fix(media): Fix playback of some multi-Period content.

When a Period has segments outside the Period, we evict them during manifest parsing. However, this eviction shouldn't affect the position of segments since these never existed. Fixes shaka-project#3230 Change-Id: I0494d9c38b51857967ed4b572475ddcc37f815a0
joeyparrish · Mar 17, 2021 · 78357ed · 78357ed
1 parent 568321c
commit 78357ed
Show file tree

Hide file tree

Showing 6 changed files with 126 additions and 5 deletions.
diff --git a/lib/dash/segment_base.js b/lib/dash/segment_base.js
@@ -186,7 +186,7 @@ shaka.dash.SegmentBase = class {
 
     segmentIndex = new shaka.media.SegmentIndex(references);
     if (fitLast) {
-      segmentIndex.fit(appendWindowStart, appendWindowEnd);
+      segmentIndex.fit(appendWindowStart, appendWindowEnd, /* isNew= */ true);
     }
     return segmentIndex;
   }

diff --git a/lib/dash/segment_list.js b/lib/dash/segment_list.js
@@ -58,6 +58,7 @@ shaka.dash.SegmentList = class {
         info.startNumber, context.representation.baseUris, info,
         initSegmentReference);
 
+    const isNew = !segmentIndex;
     if (segmentIndex) {
       segmentIndex.merge(references);
       const start = context.presentationTimeline.getSegmentAvailabilityStart();
@@ -74,7 +75,7 @@ shaka.dash.SegmentList = class {
       const periodStart = context.periodInfo.start;
       const periodEnd = context.periodInfo.duration ?
           context.periodInfo.start + context.periodInfo.duration : Infinity;
-      segmentIndex.fit(periodStart, periodEnd);
+      segmentIndex.fit(periodStart, periodEnd, isNew);
     }
 
     return {

diff --git a/lib/dash/segment_template.js b/lib/dash/segment_template.js
@@ -108,7 +108,7 @@ shaka.dash.SegmentTemplate = class {
           // Fit the new references before merging them, so that the merge
           // algorithm has a more accurate view of their start and end times.
           const wrapper = new shaka.media.SegmentIndex(references);
-          wrapper.fit(periodStart, periodEnd);
+          wrapper.fit(periodStart, periodEnd, /* isNew= */ true);
         }
 
         segmentIndex.merge(references);

diff --git a/lib/media/segment_index.js b/lib/media/segment_index.js
@@ -250,9 +250,11 @@ shaka.media.SegmentIndex = class {
    *
    * @param {number} windowStart
    * @param {?number} windowEnd
+   * @param {boolean=} isNew Whether this is a new SegmentIndex and we shouldn't
+   *   update the number of evicted elements.
    * @export
    */
-  fit(windowStart, windowEnd) {
+  fit(windowStart, windowEnd, isNew = false) {
     goog.asserts.assert(windowEnd != null,
         'Content duration must be known for static content!');
     goog.asserts.assert(windowEnd != Infinity,
@@ -275,7 +277,9 @@ shaka.media.SegmentIndex = class {
       const firstReference = this.references[0];
       if (firstReference.endTime <= windowStart) {
         this.references.shift();
-        this.numEvicted++;
+        if (!isNew) {
+          this.numEvicted++;
+        }
       } else {
         break;
       }
@@ -563,6 +567,9 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {
    * @param {!shaka.media.SegmentIndex} segmentIndex
    */
   appendSegmentIndex(segmentIndex) {
+    goog.asserts.assert(
+        this.indexes_.length == 0 || segmentIndex.numEvicted == 0,
+        'Cannot have evicted segments in non-first Periods');
     this.indexes_.push(segmentIndex);
   }
 
@@ -617,15 +624,20 @@ shaka.media.MetaSegmentIndex = class extends shaka.media.SegmentIndex {
    */
   get(position) {
     let numPassedInEarlierIndexes = 0;
+    let sawSegments = false;
 
     for (const index of this.indexes_) {
+      goog.asserts.assert(
+          !sawSegments || index.numEvicted == 0,
+          'Cannot have evicted segments in non-first Periods');
       const reference = index.get(position - numPassedInEarlierIndexes);
 
       if (reference) {
         return reference;
       }
 
       numPassedInEarlierIndexes += index.numEvicted + index.references.length;
+      sawSegments = sawSegments || index.references.length != 0;
     }
 
     return null;

diff --git a/test/dash/dash_parser_segment_base_unit.js b/test/dash/dash_parser_segment_base_unit.js
@@ -306,6 +306,46 @@ describe('DashParser SegmentBase', () => {
     expect(reference.endTime).toBe(10);  // would be 12 without PTO
   });
 
+  // https://github.com/google/shaka-player/issues/3230
+  it('works with multi-Period with eviction', async () => {
+    const source = [
+      '<MPD mediaPresentationDuration="PT75S">',
+      '  <Period duration="PT30S">',
+      '    <AdaptationSet mimeType="video/mp4">',
+      '      <Representation bandwidth="1">',
+      '        <BaseURL>http://example.com/index.mp4</BaseURL>',
+      '        <SegmentBase indexRange="30-900" />',
+      '      </Representation>',
+      '    </AdaptationSet>',
+      '  </Period>',
+      '  <Period>',
+      '    <AdaptationSet mimeType="video/mp4">',
+      '      <Representation bandwidth="1">',
+      '        <BaseURL>http://example.com/index.mp4</BaseURL>',
+      '        <SegmentBase indexRange="30-900" presentationTimeOffset="30" />',
+      '      </Representation>',
+      '    </AdaptationSet>',
+      '  </Period>',
+      '</MPD>',
+    ].join('\n');
+
+    fakeNetEngine
+        .setResponseText('dummy://foo', source)
+        .setResponseValue('http://example.com/index.mp4', indexSegment);
+
+    /** @type {shaka.extern.Manifest} */
+    const manifest = await parser.start('dummy://foo', playerInterface);
+    const video = manifest.variants[0].video;
+    await video.createSegmentIndex();  // real data, should succeed
+    goog.asserts.assert(video.segmentIndex != null, 'Null segmentIndex!');
+
+    // There are originally 5 references, but the segment that spans the Period
+    // boundary is duplicated.  In the bug, we'd stop references at the Period
+    // boundary and only have 3 references.
+    const references = Array.from(video.segmentIndex);
+    expect(references.length).toBe(6);
+  });
+
   describe('fails for', () => {
     it('unsupported container', async () => {
       const source = [

diff --git a/test/dash/dash_parser_segment_list_unit.js b/test/dash/dash_parser_segment_list_unit.js
@@ -4,7 +4,9 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+goog.require('goog.asserts');
 goog.require('shaka.test.Dash');
+goog.require('shaka.test.FakeNetworkingEngine');
 goog.require('shaka.test.ManifestParser');
 goog.require('shaka.util.Error');
 
@@ -305,5 +307,71 @@ describe('DashParser SegmentList', () => {
       await Dash.testSegmentIndex(source, references);
     });
   });
+
+  // https://github.com/google/shaka-player/issues/3230
+  it('works with multi-Period with eviction', async () => {
+    const setFormat = [
+      '    <AdaptationSet mimeType="video/mp4">',
+      '      <Representation>',
+      '        <SegmentList presentationTimeOffset="%s">',
+      '          <SegmentURL media="s1.mp4" />',
+      '          <SegmentURL media="s2.mp4" />',
+      '          <SegmentURL media="s3.mp4" />',
+      '          <SegmentURL media="s4.mp4" />',
+      '          <SegmentURL media="s5.mp4" />',
+      '          <SegmentTimeline>',
+      '            <S d="10" t="0" r="4" />',
+      '          </SegmentTimeline>',
+      '        </SegmentList>',
+      '      </Representation>',
+      '    </AdaptationSet>',
+    ].join('\n');
+    const source = [
+      '<MPD mediaPresentationDuration="PT60S">',
+      `  <BaseURL>${baseUri}</BaseURL>`,
+      '  <Period duration="PT30S">',
+      `${sprintf(setFormat, [0])}`,
+      '  </Period>',
+      '  <Period duration="PT30S">',
+      `${sprintf(setFormat, [20])}`,
+      '  </Period>',
+      '</MPD>',
+    ].join('\n');
+    const networkingEngine = new shaka.test.FakeNetworkingEngine()
+        .setResponseText('dummy://foo', source);
+
+    const dashParser = shaka.test.Dash.makeDashParser();
+
+    const playerInterface = {
+      networkingEngine: networkingEngine,
+      filter: () => {},
+      makeTextStreamsForClosedCaptions: (manifest) => {},
+      onTimelineRegionAdded: fail,  // Should not have any EventStream elements.
+      onEvent: fail,
+      onError: fail,
+      isLowLatencyMode: () => false,
+      isAutoLowLatencyMode: () => false,
+      enableLowLatencyMode: () => {},
+    };
+    const manifest = await dashParser.start('dummy://foo', playerInterface);
+    const stream = manifest.variants[0].video;
+    await stream.createSegmentIndex();
+    goog.asserts.assert(stream.segmentIndex, 'Expected index to be created');
+
+    // Don't use Dash.testSegmentIndex since it uses SegmentIndex.find, which
+    // doesn't reproduce this issue.  We want to use Array.from which uses the
+    // iterator.
+    const expected = [
+      ManifestParser.makeReference('s1.mp4', 0, 10, baseUri),
+      ManifestParser.makeReference('s2.mp4', 10, 20, baseUri),
+      ManifestParser.makeReference('s3.mp4', 20, 30, baseUri),
+
+      ManifestParser.makeReference('s3.mp4', 30, 40, baseUri),
+      ManifestParser.makeReference('s4.mp4', 40, 50, baseUri),
+      ManifestParser.makeReference('s5.mp4', 50, 60, baseUri),
+    ];
+    const actual = Array.from(stream.segmentIndex);
+    expect(actual).toEqual(expected);
+  });
 });