Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Live DASH multiperiod SSAI not working in shaka 3.x.x because segment template indexes extend forever #3354

Closed
caridley opened this issue Apr 20, 2021 · 11 comments
Assignees
Labels
priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@caridley
Copy link
Contributor

caridley commented Apr 20, 2021

Have you read the FAQ and checked for duplicate open issues?
Yes

What version of Shaka Player are you using?
3.0.10
The problem does not seem to happen with 2.5.20

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from master?
Yes with code from this commit 7af44ef from 4/19/2021

Are you using the demo app or your own custom app?
Custom app

If custom app, can you reproduce the issue using our demo app?
Yes

What browser and OS are you using?
Chrome on MAC OS
Problem is also being seen on Chromecast

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

What are the manifest and license server URIs?
Will send e-mail with info on accessing streams

What configuration are you using? What is the output of player.getConfiguration()?
See player-config.json in the attached zip file
shakaLiveDai404_TMFJ.zip

What did you do?
Watch one of our live dash DAI streams through a few ad breaks

What did you expect to happen?
Playback should progress smoothly through the ad breaks.

What actually happened?
Playback stalls at the end of the first because of a 404 response caused by making an extra segment request at the end of the ad.

Playback then resumes using segments from the period prior to the ad break, even though that period is no longer visible in the manifest. The segment request are successful because the main video streams is continuous.

Meanwhile thousands of these error messages are emitted "Assertion failed: Should not see evicted segments after available segments"

Debug logs and manifest are in attached .zip file

@caridley
Copy link
Contributor Author

I just e-mailed stream access instructions to shaka-player-issues@google.com

@caridley
Copy link
Contributor Author

caridley commented May 7, 2021

I tried adding these logging statement in generateSegmentIndexFromDuration_() in lib/dash/segment_template.js and found that the player was continuing to add segments to periods that had already ended. I suspect this could be caused by the fact that our live manifest do not have explicit period end times, but instead expect it to be inferred from the next period start time.

    for (let position = minPosition; position <= maxPosition; ++position) {
      const reference = createReference(position);
      shaka.log.info('AIDAN adding segment reference for period ' + context.period.id + 
      ' type=' + context.adaptationSet.mimeType +
      ' bandwidth=' + context.bandwidth + 
      ' position=' + position);
      references.push(reference);
    }


        while (nextPosition <= maxPosition) {
          const reference = createReference(nextPosition);
          shaka.log.info('AIDAN adding more segment reference for period ' + context.period.id + 
          ' type=' + context.adaptationSet.mimeType +
          ' bandwidth=' + context.bandwidth + 
          ' position=' + nextPosition);
          references.push(reference);
          nextPosition++;
        }

11:54:34.177 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=149000 position=2
11:54:34.178 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=899000 position=2
11:54:34.178 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=4200000 position=2
11:54:34.178 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=2100000 position=2
11:54:34.178 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=5652000 position=2
11:54:34.198 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=audio/mp4 bandwidth=96000 position=17
11:54:34.199 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=149000 position=17
11:54:34.199 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=899000 position=17
11:54:34.199 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=4200000 position=17
11:54:34.199 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=2100000 position=17
11:54:34.199 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=5652000 position=17
11:54:34.330 segment_template.js:430 AIDAN adding more segment reference for period orig_0_1-947046 type=audio/mp4 bandwidth=98800 position=947209
11:54:34.333 segment_template.js:430 AIDAN adding more segment reference for period orig_0_1-947046 type=audio/mp4 bandwidth=98800 position=947209
11:54:34.333 segment_template.js:430 AIDAN adding more segment reference for period orig_0_1-947046 type=video/mp4 bandwidth=149952 position=947209
11:54:34.333 segment_template.js:430 AIDAN adding more segment reference for period orig_0_1-947046 type=video/mp4 bandwidth=899968 position=947209
11:54:34.333 segment_template.js:430 AIDAN adding more segment reference for period orig_0_1-947046 type=video/mp4 bandwidth=1400000 position=947209
11:54:34.333 segment_template.js:430 AIDAN adding more segment reference for period orig_0_1-947046 type=video/mp4 bandwidth=2400000 position=947209
11:54:34.333 segment_template.js:430 AIDAN adding more segment reference for period orig_0_1-947046 type=video/mp4 bandwidth=3400000 position=947209
11:54:36.179 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=audio/mp4 bandwidth=96000 position=3
11:54:36.179 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=149000 position=3
11:54:36.179 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=899000 position=3
11:54:36.180 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=4200000 position=3
11:54:36.180 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=2100000 position=3
11:54:36.180 segment_template.js:430 AIDAN adding more segment reference for period ad_2-1 type=video/mp4 bandwidth=5652000 position=3
11:54:36.202 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=audio/mp4 bandwidth=96000 position=18
11:54:36.202 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=149000 position=18
11:54:36.202 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=899000 position=18
11:54:36.202 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=4200000 position=18
11:54:36.202 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=2100000 position=18
11:54:36.202 segment_template.js:430 AIDAN adding more segment reference for period ad_1-1 type=video/mp4 bandwidth=5652000 position=18

@caridley
Copy link
Contributor Author

caridley commented May 7, 2021

It looks to me like there is no way for the code that adds more references to a live stream segment index to detect a change in the period end time when the next period appears in the manifest.

@caridley
Copy link
Contributor Author

caridley commented May 7, 2021

When this problem occurs we will also see massive number of these error message: Assertion failed: Should not see evicted segments after available segments

@caridley
Copy link
Contributor Author

if we allow the player to continue after encountering the a 404 response when requesting an extra segment at the end of the first ad, the player will resume fetching and playing content from the main content from the period prior to the ad break, despite the fact that period is no longer in the manifest.

Playback of live multi-period content seems to be badly broken in shaka 3.x.x

The segment indexes need to have the period end time updated when the following period comes into view in the manifest so that they do not continue adding references for ever.

This behavior is also having an impact on application performance because a large collection or segments indexes stay alive for the duration of playback adding references and logging errors.

@caridley
Copy link
Contributor Author

caridley commented May 11, 2021

I am able to improve behavior with an extremely ugly hack that makes the updated period duration available to the index update timer.

In parsePeriods_() around line 574 in dash_parser.js I add code stash the period duration

      const period = this.parsePeriod_(context, baseUris, info);
      periods.push(period);

      window.shakaPeriodDurations[period.id] = periodDuration;

In compute computeAvailablePeriodRange() in segment_template.js I make use of the stashed period duration

const computeAvailablePeriodRange = () => {
      const updatedPeriodEnd = window.shakaPeriodDurations[context.period.id] ? 
        (periodStart + window.shakaPeriodDurations[context.period.id]) :
        periodEnd;
      return [
        Math.max(
            presentationTimeline.getSegmentAvailabilityStart(),
            periodStart),

        Math.min(
            presentationTimeline.getSegmentAvailabilityEnd(),
            updatedPeriodEnd),
      ];
    };

With these changes the segment index references for live periods no longer grow without bounds, and I no longer see a gazillion of these error messages "Assertion failed: Should not see evicted segments after available segments"

I do however see indications that multiple segment indexes are being created for the a single representation within a period. When a new period appears segment indexes for existing periods seem to be duplicated.

@caridley
Copy link
Contributor Author

I looks like the extra duplicate segment indexes are created by this line in extendExistingOutputStream_()

    // We need to create all the per-period segment indexes and append them to
    // the output's MetaSegmentIndex.
    await Promise.all(matches.map((match) => match.createSegmentIndex()));

I think these calls to createSegmentIndex should be be restricted to matches in new periods like this

    await Promise.all(matches.slice(firstNewPeriodIndex).map((match) => match.createSegmentIndex()));

@caridley caridley changed the title Extra segment request at end of ad breaks with shaka 3.0.10 Live DASH multiperiod SSAI not working in shaka 3.x.x because segment template indexes extend forever May 12, 2021
@caridley
Copy link
Contributor Author

Because the periodEnd is not being updated in the segment template index when the period end becomes known, the appendWindowEnd value is not set correctly for live multi-period streams. If a period ends partway through a segment we will end up with an overlap in the timeline. I wonder if this might be causing looping behavior that I have occasionally observed with our streams.

@caridley
Copy link
Contributor Author

@joeyparrish I believe that we have a contributors agreement in place and I would like to submit a merge request to fix this problem. I could use use some guidance on how to best provide the updated period duration and end times to the segment index. I have prototyped two approaches.

One hack in which I "published" a map of the latest period end times in window.shakaPeriodDurations.

I also tried adding a map of period duration to the dash_parser and then defined a getter for the duration field in the periodInfo and made segment_template.js compute the period end on very use

  this.periodDurations[period.id] = periodDuration;
  Object.defineProperty(info, 'duration', {
    get: () => this.periodDurations[period.id]
  });

Does this second approach seem acceptable.

@joeyparrish
Copy link
Member

Getting caught up on this thread, and then I'll try to provide the guidance you requested. Sorry for letting this one slip our attention!

@joeyparrish
Copy link
Member

found that the player was continuing to add segments to periods that had already ended. I suspect this could be caused by the fact that our live manifest do not have explicit period end times, but instead expect it to be inferred from the next period start time.

There is a check since v3.0.8 that looks like this:

        if (presentationTimeline.getSegmentAvailabilityEnd() >= periodEnd &&
            !references.length) {
          // Signal stop.
          return null;
        }

The periodEnd variable is set by context.periodInfo.duration, which is derived in lib/dash/dash_parser.js like so:

        if (nextStart != null) {
          periodDuration = nextStart - start;
        }

So by the time we generate segment references, the period duration has already been calculated based on the start of the next period in the manifest.

It looks to me like there is no way for the code that adds more references to a live stream segment index to detect a change in the period end time when the next period appears in the manifest.

This may be the root of the issue. Since the callback that generates references has captured the value of the local variable periodEnd, this would have to be replaced by a new callback to get a new value for the end of the period. Ideally, we would do something like you suggested and use some structure to allow updates to that value later.

I'm not especially keen on using Object.defineProperty for this. If SegmentTemplateInfo object is already captured by the callback, why wouldn't we just update that directly?

Just answered my own question reviewing the code. That object is recreated on each update in createStreamInfo, and the returned generateSegmentIndex function will never be called again. So updating that doesn't make much sense with the current structure.

How about this instead? We maintain and pass segmentIndexMap into each createStreamInfo call. In addition to that, we could maintain and pass a map of period IDs to ending times. That new map could be sent along to generateSegmentIndexFromDuration_, and used instead of the locally-calculated periodEnd value.

Because the periodEnd is not being updated in the segment template index when the period end becomes known, the appendWindowEnd value is not set correctly for live multi-period streams. If a period ends partway through a segment we will end up with an overlap in the timeline. I wonder if this might be causing looping behavior that I have occasionally observed with our streams.

That sounds like a reasonable conclusion. Thanks for all the hard work you've done tracking this down! We're very grateful for your help.

@joeyparrish joeyparrish added type: bug Something isn't working correctly priority: P1 Big impact or workaround impractical; resolve before feature release and removed needs triage labels May 18, 2021
@shaka-bot shaka-bot added this to the v3.2 milestone May 18, 2021
shaka-bot pushed a commit that referenced this issue Jun 10, 2021
In DASH with SegmentTimeline, we had previously assumed that the
timeline would only ever have items added to its end.  In practice,
some content adds segments to the beginning of the timeline in an
update.

When this happened, assumptions higher up the stack were broken, and
StreamingEngine began streaming from the wrong position, re-fetching
segments that were already buffered.

It is relatively simple to filter the incoming segment references to
ensure that we only ever grow our list from the end.  This fixes
assumptions in other components and resolves the re-fetching issue for
SegmentTimeline-based content.

PR #3419 will further address issues with SegmentTemplate+duration.

See also issue #3354
b/186457868

Change-Id: Ibc03a697dd6a2a6f5e0f539cb7cf94bd9a63b495
joeyparrish pushed a commit that referenced this issue Jun 16, 2021
In DASH with SegmentTemplate and a fixed duration, indexes would
continue to grow so long as they had not ended.  However, this was
based on a callback which captured an end time variable that could
become out-of-date in a multi-period live stream.

This prevents indexes from continuing to grow indefinitely by always
using the most up-to-date end time for the period.

This also fixes eviction of segments in the same scenario, which
fixes SegmentIterator access and related assertions later.

Issue #3354 (partial fix)

b/186457868

Change-Id: I2b34ee52dd12b59e1c1237258050b50e3189bee3
joeyparrish added a commit that referenced this issue Jun 16, 2021
In DASH with SegmentTimeline, we had previously assumed that the
timeline would only ever have items added to its end.  In practice,
some content adds segments to the beginning of the timeline in an
update.

When this happened, assumptions higher up the stack were broken, and
StreamingEngine began streaming from the wrong position, re-fetching
segments that were already buffered.

It is relatively simple to filter the incoming segment references to
ensure that we only ever grow our list from the end.  This fixes
assumptions in other components and resolves the re-fetching issue for
SegmentTimeline-based content.

PR #3419 will further address issues with SegmentTemplate+duration.

See also issue #3354
b/186457868

Change-Id: Ibc03a697dd6a2a6f5e0f539cb7cf94bd9a63b495
joeyparrish pushed a commit that referenced this issue Jun 16, 2021
In DASH with SegmentTemplate and a fixed duration, indexes would
continue to grow so long as they had not ended.  However, this was
based on a callback which captured an end time variable that could
become out-of-date in a multi-period live stream.

This prevents indexes from continuing to grow indefinitely by always
using the most up-to-date end time for the period.

This also fixes eviction of segments in the same scenario, which
fixes SegmentIterator access and related assertions later.

Issue #3354 (partial fix)

b/186457868

Change-Id: I2b34ee52dd12b59e1c1237258050b50e3189bee3
joeyparrish pushed a commit that referenced this issue Jun 16, 2021
In multi-period live DASH streams, the SegmentIterator would sometimes
get reset to a value pointing to the beginning of the segment
references for the period, causing StreamingEngine to re-fetch
segments it already had.

This fixes SegmentIterator's getIteratorForTime() to return null when
a reference can't be found, and fixes StreamingEngine to handle null
returns by waiting and checking again.

Closes #3354

b/186457868

Backported to v3.0.x

Change-Id: I2b34ee52dd12b59e1c1237258050b50e3189bee3
joeyparrish added a commit that referenced this issue Jun 16, 2021
In DASH with SegmentTimeline, we had previously assumed that the
timeline would only ever have items added to its end.  In practice,
some content adds segments to the beginning of the timeline in an
update.

When this happened, assumptions higher up the stack were broken, and
StreamingEngine began streaming from the wrong position, re-fetching
segments that were already buffered.

It is relatively simple to filter the incoming segment references to
ensure that we only ever grow our list from the end.  This fixes
assumptions in other components and resolves the re-fetching issue for
SegmentTimeline-based content.

PR #3419 will further address issues with SegmentTemplate+duration.

See also issue #3354
b/186457868

Change-Id: Ibc03a697dd6a2a6f5e0f539cb7cf94bd9a63b495
joeyparrish pushed a commit that referenced this issue Jun 16, 2021
In DASH with SegmentTemplate and a fixed duration, indexes would
continue to grow so long as they had not ended.  However, this was
based on a callback which captured an end time variable that could
become out-of-date in a multi-period live stream.

This prevents indexes from continuing to grow indefinitely by always
using the most up-to-date end time for the period.

This also fixes eviction of segments in the same scenario, which
fixes SegmentIterator access and related assertions later.

Issue #3354 (partial fix)

b/186457868

Change-Id: I2b34ee52dd12b59e1c1237258050b50e3189bee3
joeyparrish pushed a commit that referenced this issue Jun 16, 2021
In multi-period live DASH streams, the SegmentIterator would sometimes
get reset to a value pointing to the beginning of the segment
references for the period, causing StreamingEngine to re-fetch
segments it already had.

This fixes SegmentIterator's getIteratorForTime() to return null when
a reference can't be found, and fixes StreamingEngine to handle null
returns by waiting and checking again.

Closes #3354

b/186457868

Change-Id: I2b34ee52dd12b59e1c1237258050b50e3189bee3
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Aug 15, 2021
@shaka-project shaka-project locked and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
3 participants