-
Notifications
You must be signed in to change notification settings - Fork 426
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
fix: Handle audio only audio track change correctly #1100
Conversation
161c9cc
to
b92f5e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont really have much to add. I think it's worth having @gesinger look over this.
src/playlist-selectors.js
Outdated
// if playlist is audio only, select between currently active audio group playlists. | ||
if (Playlist.isAudioOnly(master)) { | ||
playlists = masterPlaylistController.getAudioTrackPlaylists_(); | ||
options.audioOnly = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
5f200f6
to
50615df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add some audio only related tests.
src/media-groups.js
Outdated
mediaType.logger_(`track change. Switching master audio from ${lastTrack.id} to ${activeTrack.id}`); | ||
masterPlaylistLoader.pause(); | ||
mainSegmentLoader.resetEverything(); | ||
mpc.fastQualityChange_(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the code to check that the playlist will actually change.
src/media-groups.js
Outdated
if (sourceType === 'vhs-json' && properties.playlists) { | ||
if (audioOnlyMaster) { | ||
logger_(`AUDIO group '${groupId}' label '${variantLabel}' is a master playlist`); | ||
properties.masterPlaylist = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When reading from the top I was a bit confused until I saw that masterPlaylist
was a boolean. Maybe isMasterPlaylist
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think some of the new and modified functions may need some new tests.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
ae29e4d
to
83b3e7c
Compare
); | ||
}; | ||
|
||
QUnit.module('MediaGroups', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made all media groups tests live under the same base module.
const tracks = settings.mediaTypes[type].tracks; | ||
const activeTrack = MediaGroups.activeTrack[type](type, settings); | ||
const activeGroup = MediaGroups.activeGroup(type, settings); | ||
['AUDIO', 'SUBTITLES'].forEach(function(groupType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move all activeGroup
tests to here and added a few for cases we were missing.
'sets the correct active playlist loader' | ||
); | ||
|
||
mediaType.lastGroup_ = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add tests to onGroupChanged for isMasterPlaylist
type group changes.
src/master-playlist-controller.js
Outdated
if (!master && !master.mediaGroups && !master.mediaGroups.AUDIO) { | ||
return []; | ||
if (!master || !master.mediaGroups || !master.mediaGroups.AUDIO) { | ||
return master && master.playlists; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/playlist.js
Outdated
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added as someAudioVariant
a function similar to Array.some
.
'no resync changing to group with no playlist loader' | ||
); | ||
|
||
mediaType.lastGroup_ = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
'sets the correct active playlist loader' | ||
); | ||
|
||
tracks.fr.enabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test previously relied on weird logic to change tracks, now we actually change the tracks around before each step.
}); | ||
}; | ||
|
||
QUnit.test('onTrackChanged with isMasterPlaylist', function(assert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test/playlist.test.js
Outdated
a.resolvedUri += '?nope'; | ||
assert.equal(Playlist.playlistMatch(a, b), true, 'uri match'); | ||
|
||
a.uri += '?nope'; | ||
|
||
assert.equal(Playlist.playlistMatch(a, b), false, 'no match'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should query parameters result in a uri match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, resolveUri query param succeeds because the uri still matches but if both have a query param it fails.
@@ -1317,4 +1317,95 @@ QUnit.module('Playlist', function() { | |||
'no segments, live delay can\'t be calculated' | |||
); | |||
}); | |||
|
|||
QUnit.test('playlistMatch', function(assert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if playlistMatch returns a boolean, might be better to use assert.true
and assert.false
. Same for isAudioOnly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll switch to that.
Co-authored-by: Gary Katsevman <[email protected]>
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.