fix(DASH): Fix dynamic manifests from edgeware

[avelad]

Fixes #4913

[avelad]

@joeyparrish If you want to test it, the stream needs a VPN for Bolivia or Spain

[github-actions]

Incremental code coverage: 100.00%

[avelad]

I need to test more, I think it can cause a regression in some case.

[joeyparrish]

Yeah, now that I look again, I think this isn't correct. For multi-period live, we need to fit segments in all but the last period. See the comment above.

[avelad]

Yes, I saw the regression with DAI contents

[baffinch]

How about :

      // Unless that live content is multi-period; it's safe to fit every period
      // but the last one, since only the last period might receive new
      // segments.
-    const shouldFit = periodEnd != Infinity;
+    const shouldFit = context.periodInfo.isLastPeriod? !context.dynamic : periodEnd!=Infinity;

if (not last period) {behave as currently}
if (last period) {fit if not static}

This should only change the behaviour for a 'last period', 'dynamic' schedule with 'defined duration' - i.e. the specific case in #4913 (and exactly as stated in the comment)?

[baffinch]

How about :

      // Unless that live content is multi-period; it's safe to fit every period
      // but the last one, since only the last period might receive new
      // segments.
-    const shouldFit = periodEnd != Infinity;
+    const shouldFit = context.periodInfo.isLastPeriod? !context.dynamic : periodEnd!=Infinity;

if (not last period) {behave as currently}
if (last period) {fit if not static}

This should only change the behaviour for a 'last period', 'dynamic' schedule with 'defined duration' - i.e. the specific case in #4913 (and exactly as stated in the comment)?

[avelad]

@baffinch On a theoretical level I think it could work, but let me do some tests (between today and tomorrow)

[baffinch]

Just checking the setting of context.periodInfo.isLastPeriod dash_parser.js (643) ->
isLastPeriod: periodDuration == null || !next,
it is either 'the period does not have defined duration, or there is not a 'next' period
-> In the case of #4913, it is 'there is not a next' coming from dash_parser.js (563) :

    for (let i = 0; i < periodNodes.length; i++) {
      const elem = periodNodes[i];
      const next = periodNodes[i + 1];

-> where next==undefined

This seems pretty safe, unless anyone supplies periods in the 'wrong' physical order in the mpd const periodNodes = XmlUtils.findChildren(mpd, 'Period'); -> but I assume that would already fail badly at dash_parser.js (574), so it definitely wouldn't be a regression.

[avelad]

@baffinch I've seen your condition keep failing, but I've seen one that works fine in all cases.

[baffinch]

Thanks @avelad! - That change works perfectly for our cases (dynamic, one period, duration defined, incomplete and the transition into static, one period, duration defined, approx complete).

[baffinch]

This definitely fixes our stream

[joeyparrish]

If isLastPeriod is false, periodEnd should never be Infinity. When do we have a non-last period that has an infinite duration?

If dynamic is false, periodEnd should never be Infinity in that case, either. When do we have static (VOD) content with an infinite duration?

If this change fixes the problem, it hints at another calculation gone wrong, IMO, which makes me think this isn't fixing the root cause.

[baffinch]

I think the logic has ended up as a 'not not'. The situation is easier to describe as 'why shouldn't we fit?'

1. If periodEnd == Infinity -> we can not fit ever
2. If periodEnd != Infinity -> we should not fit if it is the last period, and the manifest is dynamic (because the final period is incomplete, so the last segment will always be fitted as 'too long')

This could be defined as something like :

const dontFit = (periodEnd == Infinity) || (isLastPeriod && context.dynamic);

const shouldFit = !dontFit
// !(A || B) = !A && !B (De Morgan)
const shouldFit = (periodEnd != Infinity) && !(isLastPeriod && context.dynamic);
// !(A && B) = !A || !B (De Morgan again)
const shouldFit = (periodEnd != Infinity) && (!isLastPeriod || !context.dynamic);

// which is equivalent to the proposed change : 
const shouldFit = periodEnd != Infinity && (!isLastPeriod || !context.dynamic);

Expanding each case :

periodEnd!=Infinity	!isLastPeriod	!dynamic	=> shouldFit	notes
T	F	F	No	<= The case we're trying to fix -> last period of a dynamic manifest, that has an 'expected' duration (periodEnd) defined
T	T	F	Yes	<= A completed period, regardless of manifest type (should fit)
T	F	T	Yes	<= Last Period, static manifest (should fit)
T	T	T	Yes	<= A completed period, regardless of manifest type (should fit)
F	F	F	No	<= 'Normal' dynamic manifest case (can't fit)
F	T	F	No	<= Impossible, only last period should be infinite
F	F	T	No	<= Impossible, static manifest should not be infinite
F	T	T	No	<= Impossible, only last period should be infinite

As you say, it contains impossible cases - but they don't matter as all the valid cases are handled correctly

[joeyparrish]

That table doesn't look quite correct to me, or perhaps I'm just getting some details mixed up in reading it, but I appreciate the truth-table approach. Let me try to revise it, to make the variables positive, and to focus on the ideal rather than current code. (X = "don't care", in case anyone reading is not familiar with the convention.) Apologies if this is equivalent to your table and I failed to understand it, but making this one is my process to grasp this issue.

dynamic	periodEnd == Infinity	isLastPeriod	ideal "shouldFit"	current "shouldFit"	notes
F	F	X	T	T	typical VOD
F	T	X	X	F	VOD with infinite periods - should not happen
T	F	F	T	T	typical live, not the most recent period
T	F	T	T	T	live, but the most recent period has a fixed duration (can this happen?) / alternately, IPR (in-progress recordings), which are dynamic but finite / your case with edgeware, correct?
T	T	F	X	F	live, but older period that is infinite - should not happen
T	T	T	F	F	typical live, the most recent period, which is infinite

I filled in the "ideal" column above before considering the "current" column, then added that afterward and found that the current code matches my expectations for what is correct/ideal.

In the case of "dynamic + finite + last period", it makes sense to me to fit segments. If the period has a duration, you clip off the ends of segments that overhang that period, whether or not it's the last period. If the period's duration may grow or change, it shouldn't have a finite duration at all. Does that make sense?

1. Did I understand correctly which case is yours? (dynamic, finite, last period?)
2. Am I mistaken about what is correct/ideal for that case? (Or any other?) If so, why?

[baffinch]

1. You completely understood - our case is 'dynamic', 'finite', 'last period'. It happens in two 'real' cases - IPR, as you say, but it is also used by edgeware for SOV (startover), where the startover is bounded to a scheduled event that has not completed yet.
2. I think this is exactly the point of debate so I'll try to elaborate :

If the period has a duration, you clip off the ends of segments that overhang that period, whether or not it's the last period.
-> I would agree with that, except that in our case it does not apply - as it is a dynamic manifest, all the segments are not defined yet, so our final segment is short (often by up to an hour) of the period end, not overhanging it. The code for fit (segment_index.js ; 301 ; fit() ) does not consider if it is shortening or lengthening the last segment, it just sets it to the end of the period :

    // Adjust the last SegmentReference.
    const lastReference = this.references[this.references.length - 1];
    this.references[this.references.length - 1] =
        new shaka.media.SegmentReference(
            lastReference.startTime,
            /* endTime= */ windowEnd,...

If the period's duration may grow or change, it shouldn't have a finite duration at all
-> I agree with this also, except that it is also not our case - the SOV and IPR are scheduled against discrete events on an LTV schedule, so they do have a pre-defined duration, even if the segments are not all processed/available yet.

---

I checked with our web team, and this stream did previously work with shaka player, although on a release from 2020, so I looked into the history. The problem the noted (with the seekRange, used to construct a progress/trickplay bar, I think) in #4913 - was previously raised with a similar issue in 2016, in #423
"If we set the MPD to the full duration of the recording, and we don't have enough segments to fill it, fitSegmentReferences will extend our last reference to the length of the MPD. That wouldn't be an issue, except getLiveEdge uses maxSegmentDuration in its calculation, and so our live edge (and availability end) calculation gets messed up."
getSeekRangeEnd() which is the incorrect return in our case, is also based on getLiveEdge_(), and eventually on the 'max segment duration'

This was eventually resolved in a refactor : #477
c7e73e0
3cad924
-> where the logic on whether to fit was changed to be based only on the 'dynamic' flag :
lib/dash/mpd_utils.js ; fitSegmentReferences ; line 314 ; if (dynamic)...

There were a set of changes to this over time, notably to reference the last period :
d510795
lib/dash/mpd_utils.js ; fitSegmentReferences ; line 324 ;
Removed -> if (dynamic)
Added -> if (!fitLast)
Where fitLast was defined as (lib/dash/segment_base.js ; line 126)

var fitLast = !context.dynamic || !context.periodInfo.isLastPeriod;

The condition was moved around the code a bit, but the logic still remained the same (May 30, 2018) :
b212ef9
Removed -> if (!context.dynamic || !context.periodInfo.isLastPeriod) {
Added -> let shouldFit = !context.dynamic || !context.periodInfo.isLastPeriod;

    if (shouldFit) {
      segmentIndex.fit(context.periodInfo.duration);
    }

The major change to the logic came in the work for player v3 : 'Flatten Periods' (Apr 9, 2020)
e24fec4
lib/dash/segment_template.js
Removed -> const shouldFit = !context.dynamic || !context.periodInfo.isLastPeriod;
Added -> const shouldFit = periodEnd != Infinity;
... which is where the behaviour changed, but only for the case 'dynamic', 'finite', 'last period'

---

Adding this logic, for 'Before Flatten Periods change', as 'player v2' to the truth table :

dynamic	periodEnd Infinity	isLastPeriod	player v2	ideal "shouldFit"	current "shouldFit"	notes
F	F	X	T	T	T	typical VOD
F	T	X	T	X	F	VOD with infinite periods - should not happen
T	F	F	T	T	T	typical live, not the most recent period
T	F	T	F	?	T	<- This case
T	T	F	T	X	F	live, but older period that is infinite - should not happen
T	T	T	F	F	F	typical live, the most recent period, which is infinite

So the only case (that is not a 'should not happen') that was changed with that logic change is this one - so effectively I'm trying to argue that e24fec4 is a regression.

I would be just as happy with a change back to

const shouldFit = !context.dynamic || !context.periodInfo.isLastPeriod;

as the proposed change to

const shouldFit = periodEnd != Infinity && (!isLastPeriod || !context.dynamic);

... the effect on all 'real' cases would be the same.

[joeyparrish]

The code for fit (segment_index.js ; 301 ; fit() ) does not consider if it is shortening or lengthening the last segment

Ah! Of course! I should have remembered that (or re-read the code). You are definitely correct. We don't want to expand the last segment of IPR or any equivalent of that.

Thank you for being patient with me and helping me understand!

The final truth-table should be:

dynamic	periodEnd Infinity	isLastPeriod	shouldFit	notes
F	F	X	T	typical VOD
F	T	X	X (T)	VOD with infinite periods - should not happen
T	F	F	T	typical live, not the most recent period
T	F	T	F	This case, also IPR
T	T	F	X (T)	live, but older period that is infinite - should not happen
T	T	T	F	typical live, the most recent period, which is infinite

And I think this really needs to be a comment in the code, to help preserve the correct behavior across future refactors.

I think the old version is more readable than the new proposal:

const shouldFit = !context.dynamic || !context.periodInfo.isLastPeriod;

But I think this is even clearer, especially with a comment:

// We never fit the final period of dynamic content, which could be infinite live
// (with no limit to fit to) or IPR (which would expand the most recent segment
// to the end of the presentation).
const shouldFit = !(context.dynamic && context.periodInfo.isLastPeriod);

[joeyparrish]

I'm going to take the liberty of editing the PR. Thanks for the discussion!

[joeyparrish]

With my proposed logic in place, there's a test failure that involves one of those "impossible" cases: "rejects periods after one without duration". The test fails because we fail an assertion in fit() that protects against the impossible.

That test was added in 2015 with the v2 DASH parser, and I don't think it adds any value now. So I will delete it.

[joeyparrish]

@avelad, I'll let you merge if you approve of the modifications I've made to your PR.

[avelad]

@joeyparrish I'll review it asap.

[baffinch]

Thanks to both of you!

[joeyparrish]

I've started cherry-picking for bugfix releases, and I hope to get this PR into those releases. I'll do all the other prep work, but we have a Cast deadline Tuesday (US/Pacific time) that I need to get ahead of, so please review today if you can.