-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: Fix missing audio streams #5869
Conversation
184e25f
to
c030a20
Compare
Incremental code coverage: 100.00% |
const getAudioId = (stream) => { | ||
const validAudioIds = []; | ||
const validAudioStreamsMap = new Map(); | ||
const getAudioGroupId = (stream) => { | ||
return stream.language + (stream.channelsCount || 0) + | ||
(stream.audioSamplingRate || 0) + stream.roles.join(',') + | ||
stream.label + stream.groupId + stream.fastSwitching; |
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.
It's kind of weird how this function is now called getAudioGroupId
, and yet one of the many factors that goes into it is stream.groupId
.
I'm having a hard time coming up with a better name, admittedly. getAudioDuplicateId
or something?
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.
Changed to getAudioId
const includedStream = audioStreamsMap.get(id); | ||
if (includedStream.bandwidth > stream.bandwidth) { | ||
audioStreamsMap.set(id, stream); | ||
const previousStream = validAudioStreams[validAudioStreams.length - 1]; |
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.
Ok, I'm trying to understand the exact logic of the change.
Our previous behavior was to present the lowest-bandwidth stream for each groupId.
Now, within each groupId, we have at most one stream per bandwidth. And we also filter out every stream that does not share a codec with the lowest-bandwidth stream in that groupId (which we did not have to do since previously we only had one stream per groupId anyway, so no need to worry about adapting between codecs).
Is that correct?
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.
it's right
Fixes #5858