fix: Handle audio only audio track change correctly #1100

brandonocasey · 2021-03-15T20:32:30Z

Allows audio only playlists to change audio tracks. IE an audio only playlist can change from english to spanish correctly now. Would be good to get videojs/mpd-parser#123 in with this.

gkatsev

Dont really have much to add. I think it's worth having @gesinger look over this.

package.json

src/playlist-selectors.js

gkatsev · 2021-03-29T22:27:58Z

src/playlist-selectors.js

+  // if playlist is audio only, select between currently active audio group playlists.
+  if (Playlist.isAudioOnly(master)) {
+    playlists = masterPlaylistController.getAudioTrackPlaylists_();
+    options.audioOnly = true;
+  }


gesinger

We should probably add some audio only related tests.

src/playlist-selectors.js

src/master-playlist-controller.js

src/media-groups.js

gesinger · 2021-03-30T16:24:08Z

src/media-groups.js

+      mediaType.logger_(`track change. Switching master audio from ${lastTrack.id} to ${activeTrack.id}`);
+      masterPlaylistLoader.pause();
+      mainSegmentLoader.resetEverything();
+      mpc.fastQualityChange_();


fastQualityChange should reset everything unless the media is the same. And if the media is the same, would we ever resume playback (the loader would pause but fastQualityChange would return early)?

We will only get in here if the track id changed. which will cause us to select from a new set of playlists in fastQualityChange which will mean that the playlists should never be the same.

updated the code to check that the playlist will actually change.

src/media-groups.js

src/playlist-selectors.js

src/master-playlist-controller.js

gesinger · 2021-04-01T20:15:48Z

src/media-groups.js

-        if (sourceType === 'vhs-json' && properties.playlists) {
+        if (audioOnlyMaster) {
+          logger_(`AUDIO group '${groupId}' label '${variantLabel}' is a master playlist`);
+          properties.masterPlaylist = true;


When reading from the top I was a bit confused until I saw that masterPlaylist was a boolean. Maybe isMasterPlaylist?

src/media-groups.js

src/playlist.js

test/rendition-mixin.test.js

gesinger

Looks good. I think some of the new and modified functions may need some new tests.

codecov · 2021-04-05T16:41:15Z

Codecov Report

Merging #1100 (a9bfc51) into main (9b116ce) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1100      +/-   ##
==========================================
+ Coverage   86.13%   86.29%   +0.15%     
==========================================
  Files          38       38              
  Lines        8966     9068     +102     
  Branches     2017     2052      +35     
==========================================
+ Hits         7723     7825     +102     
  Misses       1243     1243

Impacted Files	Coverage Δ
src/master-playlist-controller.js	`94.31% <100.00%> (+0.18%)`	⬆️
src/media-groups.js	`98.90% <100.00%> (+0.15%)`	⬆️
src/playlist-selectors.js	`85.23% <100.00%> (+0.40%)`	⬆️
src/playlist.js	`94.50% <100.00%> (+1.29%)`	⬆️
src/rendition-mixin.js	`94.87% <100.00%> (+0.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b116ce...a9bfc51. Read the comment docs.

brandonocasey · 2021-04-05T20:48:35Z

test/media-groups.test.js

-);
+};
+
+QUnit.module('MediaGroups', function() {


made all media groups tests live under the same base module.

brandonocasey · 2021-04-05T20:49:00Z

test/media-groups.test.js

-  const tracks = settings.mediaTypes[type].tracks;
-  const activeTrack = MediaGroups.activeTrack[type](type, settings);
-  const activeGroup = MediaGroups.activeGroup(type, settings);
+  ['AUDIO', 'SUBTITLES'].forEach(function(groupType) {


move all activeGroup tests to here and added a few for cases we were missing.

brandonocasey · 2021-04-05T20:49:47Z

test/media-groups.test.js

+        'sets the correct active playlist loader'
+      );
+
+      mediaType.lastGroup_ = null;


add tests to onGroupChanged for isMasterPlaylist type group changes.

gesinger · 2021-04-05T21:22:47Z

src/master-playlist-controller.js

-    if (!master && !master.mediaGroups && !master.mediaGroups.AUDIO) {
-      return [];
+    if (!master || !master.mediaGroups || !master.mediaGroups.AUDIO) {
+      return master && master.playlists;


If there's no master, is it OK that this returns undefined? Would it be better to return []?

The comment above may need to be changed too otherwise. And would returning master.playlists end up returning non audio only tracks?

I will make it return an empty array without a master. It will could end up returning non-audio playlists but if we don't have audio groups and this function is called, which should only happen if we detect that the playlist is audio only, Then we should just return the main playlists.

gesinger · 2021-04-05T21:27:00Z

src/playlist.js

+    if (AUDIO) {
+      let audioGroupFound = false;
+
+      for (const groupName in AUDIO) {
+        if (audioGroupFound) {
+          break;
+        }
+        for (const label in AUDIO[groupName]) {
+          const variant = AUDIO[groupName][label];
+
+          // playlist is in an audio group it is audio only
+          if (playlistMatch(playlist, variant)) {
+            audioGroupFound = true;
+            break;
+          }
+        }
+      }
+
+      if (audioGroupFound) {
+        continue;
+      }
+    }
+
+    // if we make it here this playlist isn't audio and we
+    // are not audio only
+    return false;
+  }


If we can, since this ends up in a nesting of three for loops, it may be better to move this to a separate function to check if there's an audio playlist match for a given playlist.

added as someAudioVariant a function similar to Array.some.

brandonocasey · 2021-04-06T15:42:10Z

test/media-groups.test.js

+        'no resync changing to group with no playlist loader'
+      );
+
+      mediaType.lastGroup_ = null;


For most of this test we have to add this line before calling onGroupChanged to mimic previous behavior, where we didn't care about group changes.

brandonocasey · 2021-04-06T15:43:50Z

test/media-groups.test.js

+        'sets the correct active playlist loader'
+      );
+
+      tracks.fr.enabled = false;


This test previously relied on weird logic to change tracks, now we actually change the tracks around before each step.

brandonocasey · 2021-04-06T15:45:18Z

test/media-groups.test.js

+    });
+  };
+
+  QUnit.test('onTrackChanged with isMasterPlaylist', function(assert) {


The previous onTrackChanged test was very brittle, so I didn't want to add anything else to it. I instead added the this test here that tests the new isMasterPlaylist functionality.

This reverts commit 94bdda3.

test/media-groups.test.js

gkatsev · 2021-04-06T17:50:29Z

test/playlist.test.js

+    a.resolvedUri += '?nope';
+    assert.equal(Playlist.playlistMatch(a, b), true, 'uri match');
+
+    a.uri += '?nope';
+
+    assert.equal(Playlist.playlistMatch(a, b), false, 'no match');


should query parameters result in a uri match?

I see, resolveUri query param succeeds because the uri still matches but if both have a query param it fails.

gkatsev · 2021-04-06T17:51:29Z

test/playlist.test.js

@@ -1317,4 +1317,95 @@ QUnit.module('Playlist', function() {
      'no segments, live delay can\'t be calculated'
    );
  });
+
+  QUnit.test('playlistMatch', function(assert) {


if playlistMatch returns a boolean, might be better to use assert.true and assert.false. Same for isAudioOnly.

i'll switch to that.

Co-authored-by: Gary Katsevman <gary.katsevman@gmail.com>

brandonocasey force-pushed the fix/audio-only-groups branch 2 times, most recently from 161c9cc to b92f5e2 Compare March 17, 2021 19:59

gkatsev reviewed Mar 29, 2021

View reviewed changes

fix: Handle audio only playlists without main playlists

50615df

brandonocasey force-pushed the fix/audio-only-groups branch from 5f200f6 to 50615df Compare March 30, 2021 15:24

fix jsdocs

7270777

gesinger reviewed Mar 30, 2021

View reviewed changes

brandonocasey added the needs updates label Mar 30, 2021

brandonocasey and others added 5 commits March 30, 2021 13:57

code review

b48ee53

Merge branch 'main' into fix/audio-only-groups

1c6b5af

fixes from code review

2da9d29

test fixes

7ad23c7

more test fixes

c957dde

gesinger reviewed Apr 1, 2021

View reviewed changes

code review

8005060

gesinger reviewed Apr 5, 2021

View reviewed changes

fix tests

812205a

brandonocasey added 6 commits April 5, 2021 12:55

Merge remote-tracking branch 'origin/main' into fix/audio-only-groups

d0353d2

add tests for isAudioOnly/playlistMatch

7ce84dc

test audio only selector

827edce

media group test improvements

ceece05

Merge remote-tracking branch 'origin/main' into fix/audio-only-groups

01396be

test more code

83b3e7c

brandonocasey force-pushed the fix/audio-only-groups branch from ae29e4d to 83b3e7c Compare April 5, 2021 20:25

test getAudioTrackPlaylists_

fba0523

brandonocasey commented Apr 5, 2021

View reviewed changes

ie 11 fix

4a4813f

gesinger reviewed Apr 5, 2021

View reviewed changes

code review

46b00e7

brandonocasey commented Apr 6, 2021

View reviewed changes

brandonocasey added 3 commits April 6, 2021 11:50

improve coverage

c1b4501

switch instantly to alternative main audio

94bdda3

Revert "switch instantly to alternative main audio"

5a485ab

This reverts commit 94bdda3.

gkatsev previously approved these changes Apr 6, 2021

View reviewed changes

Update test/media-groups.test.js

5691211

Co-authored-by: Gary Katsevman <gary.katsevman@gmail.com>

brandonocasey dismissed gkatsev’s stale review via 5691211 April 6, 2021 17:54

switch to assert true/false

a9bfc51

gkatsev approved these changes Apr 6, 2021

View reviewed changes

brandonocasey merged commit 6d83de3 into main Apr 6, 2021

brandonocasey deleted the fix/audio-only-groups branch April 6, 2021 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Handle audio only audio track change correctly #1100

fix: Handle audio only audio track change correctly #1100

brandonocasey commented Mar 15, 2021 •

edited

Loading

gkatsev left a comment

gkatsev Mar 29, 2021

gesinger left a comment

gesinger Mar 30, 2021

brandonocasey Mar 30, 2021

brandonocasey Mar 30, 2021

gesinger Apr 1, 2021

gesinger left a comment

codecov bot commented Apr 5, 2021 •

edited

Loading

brandonocasey Apr 5, 2021

brandonocasey Apr 5, 2021

brandonocasey Apr 5, 2021

gesinger Apr 5, 2021

brandonocasey Apr 6, 2021

gesinger Apr 5, 2021

brandonocasey Apr 6, 2021

brandonocasey Apr 6, 2021 •

edited

Loading

brandonocasey Apr 6, 2021

brandonocasey Apr 6, 2021

gkatsev Apr 6, 2021

gkatsev Apr 6, 2021

gkatsev Apr 6, 2021

brandonocasey Apr 6, 2021

fix: Handle audio only audio track change correctly #1100

fix: Handle audio only audio track change correctly #1100

Conversation

brandonocasey commented Mar 15, 2021 • edited Loading

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gesinger left a 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

gesinger left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 5, 2021 • edited Loading

Codecov Report

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

brandonocasey Apr 6, 2021 • edited Loading

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

brandonocasey commented Mar 15, 2021 •

edited

Loading

codecov bot commented Apr 5, 2021 •

edited

Loading

brandonocasey Apr 6, 2021 •

edited

Loading