Blacklist audio only playlists on probe if already playing video #1257

gesinger · 2017-09-14T21:11:35Z

Description

When a master playlist doesn't contain codec info, we have to wait until we probe to know if a rendition is audio only. Because we can't switch from audio and video to audio only, blacklist that playlist if we're already playing video.

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

shahlabs · 2017-09-15T14:41:34Z

test/segment-loader.test.js

+      assert.equal(errors.length, 0, 'no errors');
+    });
+
+    QUnit.test('does not error when going from audio only to avoid and video',


It should be audio and video

forbesjo · 2017-09-15T19:53:56Z

Looks good to me

mjneil · 2017-09-18T21:07:04Z

test/segment-loader.test.js

+      assert.equal(errors.length, 0, 'no errors');
+    });
+
+    QUnit.test('does not error when going from audio only to audio and video',


Do we support this?

If I switch from audio only to audio/video rendition it gives me error as VIDEOJS: ERROR: DOMException: Failed to set the 'duration' property on 'MediaSource': The 'updating' attribute is true on one or more of this MediaSource's SourceBuffers.

I'm not sure if we support it, but I put the test in since we don't want to throw the error there for now (until we re-consider). I can remove the test case if we want, until we are sure about behavior.

Seems from @shahlabs ' testing we may want to consider an error anyway.

Well according to the MSE spec, going from audio only to audio + video will be dependent on browser

For example, a user agent may throw a QuotaExceededError exception if the media element has reached the HAVE_METADATA readyState. This can occur if the user agent's media engine does not support adding more tracks during playback.
https://w3c.github.io/media-source/#dom-mediasource-addsourcebuffer

So if we want to support this, we should make sure which browsers support it, and handle the error in browsers that dont. I think its preferable to just not allow switching between audio only and video+audio and vice versa.

Might be worth seeing what happens when going from video only to audio+video as well, though I'm not sure if that is something anyone is trying to do in their manifests.

mjneil · 2017-09-19T21:56:42Z

src/segment-loader.js

+    // don't support switching from audio and video to audio only. If the codecs were in
+    // the master manifest, then the renditions incompatible with the first rendition
+    // would already be blacklisted.
+    if (this.isUsingVideo_ &&


WRT the other discussion on going from audio only to video+audio

If we decide that we want to support that, this will NOT trigger an error in the event that you start audio only, switch to video+audio, and then try to switch back to audio only since this.isUsingVideo_ will be set to false from the initial audio only timing source

mjneil · 2017-09-20T19:20:28Z

src/segment-loader.js

+
+  if (!startingMedia.containsAudio && startingMedia.containsVideo &&
+      newSegmentMedia.containsAudio && !newSegmentMedia.containsVideo) {
+    return 'Only audio found in segment when we expected video only.' +


a bit misleading saying we expected "video only" as we can go from starting with video only to video + audio (sort of, if starting was video only because there was a separate audio segment loader and not because there was just only video stream)

mjneil · 2017-09-20T19:27:30Z

src/segment-loader.js

+    return 'Neither audio nor video found in segment.';
+  }
+
+  if (startingMedia.containsAudio && startingMedia.containsVideo &&


I think we could simplify this and still give informative messages

if (startingMedia.containsVideo && !newSegmentMedia.containsVideo) { return 'Only audio found in segment when we expected video.' + ' We can\'t switch to audio only from a stream that had video' + ' To get rid of this message, please add codec information to the manifest.'; } if (!startingMedia.containsVideo && newSegmentMedia.containsVideo) { return 'Video found in segment when we expected only audio.' + ' We can\'t switch to a Video stream from a stream that had only audio' + ' To get rid of this message, please add codec information to the manifest.'; }

mjneil · 2017-09-20T20:44:01Z

test/segment-loader.test.js

+               ' manifest.',
+               'error when muxed to audio only');
+
+  startingMedia = { containsAudio: true, containsVideo: true };


Maybe add a test that has startingMedia = { containsAudio: true, containsVideo: false }; and newSegmentMedia = { containsAudio: false, containsVideo: false }; just to confirm it also returns 'Neither audio nor video found in segment.'

Looks like you forgot to updated containsVideo here, this test is exactly the same as the one on line 55. Also would be nice to move the two tests for 'Neither audio nor video found in segment.' next to each other just for reading through, but not necessary

mjneil · 2017-09-20T20:45:12Z

test/segment-loader.test.js

@@ -603,5 +675,92 @@ QUnit.module('SegmentLoader', function(hooks) {
      assert.ok(!playlistUpdated.segments[0].end,
                'no end info for first segment of new playlist');
    });
+
+    QUnit.test('errors when trying to switch from audio and video to audio only',


Maybe another test like this but switching from audio only to video and audio

mjneil · 2017-09-20T20:46:24Z

test/segment-loader.test.js

@@ -20,6 +23,75 @@ const ogAddSegmentMetadataCue_ = SegmentLoader.prototype.addSegmentMetadataCue_;

 SegmentLoader.prototype.addSegmentMetadataCue_ = function() {};

+QUnit.module('SegmentLoader Isolated Functions');
+
+QUnit.test('illegalMediaSwitch detects illegal media switches', function(assert) {


I think it would be good to adda test that passes type === 'audio' since the audio segment loader will also be calling this function

mjneil · 2017-09-20T20:47:24Z

src/segment-loader.js


-    this.syncController_.probeSegmentInfo(segmentInfo);
+    let illegalMediaSwitchError =


mjneil · 2017-09-20T21:09:18Z

src/segment-loader.js

+  // Although these checks should most likely cover non 'main' types, for now it narrows
+  // the scope of our checks.
+  if (loaderType !== 'main' || !startingMedia || !newSegmentMedia) {
+    return;


Since you make use of the return value, it might be a good idea to explicitly return null or false

Blacklist audio only playlists on probe if already playing video

32981ce

shahlabs reviewed Sep 15, 2017

View reviewed changes

forbesjo self-requested a review September 15, 2017 19:54

forbesjo approved these changes Sep 15, 2017

View reviewed changes

Fix spelling mistake in audio to audio and video test

e77139c

mjneil reviewed Sep 18, 2017

View reviewed changes

mjneil reviewed Sep 19, 2017

View reviewed changes

Check more cases for illegal media switches in main segment loader

b22b867

mjneil reviewed Sep 20, 2017

View reviewed changes

Simplify messaging and logic in illegalMediaSwitch

153b47a

mjneil reviewed Sep 20, 2017

View reviewed changes

src/segment-loader.js Outdated

this.syncController_.probeSegmentInfo(segmentInfo);

let illegalMediaSwitchError =

Copy link

Contributor

mjneil Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

Cover a couple more test cases for illegal media switches

9b42106

mjneil reviewed Sep 20, 2017

View reviewed changes

Return null when no error for illegalMediaSwitch

8cd9efa

mjneil approved these changes Sep 20, 2017

View reviewed changes

gesinger merged commit 3444822 into videojs:master Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blacklist audio only playlists on probe if already playing video #1257

Blacklist audio only playlists on probe if already playing video #1257

gesinger commented Sep 14, 2017

shahlabs Sep 15, 2017

forbesjo commented Sep 15, 2017

mjneil Sep 18, 2017

shahlabs Sep 18, 2017

gesinger Sep 19, 2017

mjneil Sep 19, 2017

mjneil Sep 19, 2017

mjneil Sep 20, 2017

mjneil Sep 20, 2017

mjneil Sep 20, 2017

mjneil Sep 20, 2017

mjneil Sep 20, 2017

mjneil Sep 20, 2017

mjneil Sep 20, 2017

mjneil Sep 20, 2017


		this.syncController_.probeSegmentInfo(segmentInfo);
		let illegalMediaSwitchError =

Blacklist audio only playlists on probe if already playing video #1257

Blacklist audio only playlists on probe if already playing video #1257

Conversation

gesinger commented Sep 14, 2017

Description

Requirements Checklist

Choose a reason for hiding this comment

forbesjo commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment