fix: null check sidx on sidxmapping, check that end > start on remove (…

…#1121)
videojs · Apr 23, 2021 · 92f1333 · 92f1333
1 parent 3c9f721
commit 92f1333
Show file tree

Hide file tree

Showing 4 changed files with 113 additions and 2 deletions.
diff --git a/src/dash-playlist-loader.js b/src/dash-playlist-loader.js
@@ -140,7 +140,8 @@ export const updateMaster = (oldMaster, newMaster, sidxMapping) => {
     if (playlist.sidx) {
       const sidxKey = generateSidxKey(playlist.sidx);
 
-      if (sidxMapping && sidxMapping[sidxKey]) {
+      // add sidx segments to the playlist if we have all the sidx info already
+      if (sidxMapping && sidxMapping[sidxKey] && sidxMapping[sidxKey].sidx) {
         addSidxSegmentsToPlaylist(playlist, sidxMapping[sidxKey].sidx, playlist.sidx.resolvedUri);
       }
     }

diff --git a/src/segment-loader.js b/src/segment-loader.js
@@ -1154,6 +1154,14 @@ export default class SegmentLoader extends videojs.EventTarget {
       end = this.duration_();
     }
 
+    // skip removes that would throw an error
+    // commonly happens during a rendition switch at the start of a video
+    // from start 0 to end 0
+    if (end <= start) {
+      this.logger_('skipping remove because end ${end} is <= start ${start}');
+      return;
+    }
+
     if (!this.sourceUpdater_ || !this.startingMediaInfo_) {
       this.logger_('skipping remove because no source updater or starting media info');
       // nothing to remove if we haven't processed any media

diff --git a/test/dash-playlist-loader.test.js b/test/dash-playlist-loader.test.js
@@ -7,6 +7,7 @@ import {
   filterChangedSidxMappings,
   parseMasterXml
 } from '../src/dash-playlist-loader';
+import parseSidx from 'mux.js/lib/tools/parse-sidx';
 import xhrFactory from '../src/xhr';
 import {generateSidxKey} from 'mpd-parser';
 import {
@@ -385,6 +386,67 @@ QUnit.test('updateMaster: updates minimumUpdatePeriod', function(assert) {
   );
 });
 
+QUnit.test('updateMaster: requires sidxMapping.sidx to add sidx segments', function(assert) {
+  const prev = {
+    playlists: [{
+      uri: '0',
+      id: 0,
+      segments: [],
+      sidx: {
+        resolvedUri: 'https://example.com/foo.mp4',
+        uri: 'foo.mp4',
+        duration: 10,
+        byterange: {
+          offset: 2,
+          length: 4
+        }
+      }
+    }],
+    mediaGroups: {
+      AUDIO: {},
+      SUBTITLES: {}
+    }
+  };
+  const next = {
+    playlists: [{
+      uri: '0',
+      id: 0,
+      segments: [],
+      sidx: {
+        resolvedUri: 'https://example.com/foo.mp4',
+        uri: 'foo.mp4',
+        duration: 10,
+        byterange: {
+          offset: 2,
+          length: 4
+        }
+      }
+    }],
+    mediaGroups: {
+      AUDIO: {},
+      SUBTITLES: {}
+    }
+  };
+  const sidxMapping = {};
+  const key = generateSidxKey(prev.playlists[0].sidx);
+
+  sidxMapping[key] = {sidxInfo: {uri: 'foo', key}};
+
+  assert.deepEqual(
+    updateMaster(prev, next, sidxMapping),
+    null,
+    'no update'
+  );
+
+  sidxMapping[key].sidx = parseSidx(sidxResponse().subarray(8));
+
+  const result = updateMaster(prev, next, sidxMapping);
+
+  assert.ok(result, 'result returned');
+  assert.equal(result.playlists[0].segments.length, 1, 'added one segment from sidx');
+
+});
+
 QUnit.test('compareSidxEntry: will not add new sidx info to a mapping', function(assert) {
   const playlists = {
     0: {

diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js
@@ -2633,8 +2633,48 @@ QUnit.module('SegmentLoader', function(hooks) {
       });
     });
 
-    QUnit.test('triggers appenderror when append errors', function(assert) {
+    QUnit.test('does not remove when end <= start', function(assert) {
+      let audioRemoves = 0;
+      let videoRemoves = 0;
+
+      return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => {
+        const playlist = playlistWithDuration(40);
+
+        loader.playlist(playlist);
+        loader.load();
+        this.clock.tick(1);
+
+        loader.sourceUpdater_.removeAudio = (start, end) => {
+          audioRemoves++;
+        };
+        loader.sourceUpdater_.removeVideo = (start, end) => {
+          videoRemoves++;
+        };
+
+        assert.equal(audioRemoves, 0, 'no audio removes');
+        assert.equal(videoRemoves, 0, 'no video removes');
+
+        standardXHRResponse(this.requests.shift(), muxedSegment());
+        return new Promise((resolve, reject) => {
+          loader.one('appended', resolve);
+          loader.one('error', reject);
+        });
+      }).then(() => {
+        loader.remove(0, 0, () => {});
+        assert.equal(audioRemoves, 0, 'no audio remove');
+        assert.equal(videoRemoves, 0, 'no video remove');
+
+        loader.remove(5, 4, () => {});
+        assert.equal(audioRemoves, 0, 'no audio remove');
+        assert.equal(videoRemoves, 0, 'no video remove');
 
+        loader.remove(0, 4, () => {});
+        assert.equal(audioRemoves, 1, 'valid remove works');
+        assert.equal(videoRemoves, 1, 'valid remove works');
+      });
+    });
+
+    QUnit.test('triggers appenderror when append errors', function(assert) {
       return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => {
         return new Promise((resolve, reject) => {
           loader.one('appenderror', resolve);