fix: do not wait for audio appends for muxed segments #894
Conversation
src/media-segment-request.js
Outdated
| @@ -289,7 +289,7 @@ const transmuxAndNotify = ({ | |||
|
|
|||
| if (probeResult) { | |||
| trackInfoFn(segment, { | |||
| hasAudio: probeResult.hasAudio, | |||
| hasAudio: !isMuxed && probeResult.hasAudio, | |||
There was a problem hiding this comment.
It seems a bit strange to me that we are reporting that we don't have audio even though we do. Would it be better to perform the check for isMuxed and hasAudio and separating the behavior at a higher level, so we always report what we do see in the result?
There was a problem hiding this comment.
It is a bit strange, I agree, but I don't know if we want to do that or not. I am open to a refactor but lets go over the reasons I have to keep it right now.
The reason we have it setup like this is that we will only ever append to and create a videoBuffer in these situations as videoBuffer will have both codecs: video/mp4;codecs="avc1.4d400d,mp4a.40.2".
This means that our videoBuffer is more of a combined buffer. The segment loader, and anyone else shouldn't care that the segment has audio, since its just a big bundle of data that will only ever be treated as video. We won't ever see any audio data and the audioBuffer won't ever be created. All data will be spit out by the muxer will reported as combined and changed to video
If we want to make such a change I have the code done. I'm just not sure what is best here.
There was a problem hiding this comment.
We should add in @gkatsev too, but I'm inclined to say that if we make changes like this, we should probably do it now. It's hard to read the code as hasAudio having the meaning that it's audio only, rather than a component of muxed.
There was a problem hiding this comment.
are you saying to rename hasAudio to something like audioOnlyMedia?
There was a problem hiding this comment.
Probably more of what Brandon prepared in the link, that hasAudio and hasVideo remain descriptors of if the media has audio and video, and isMuxed is passed along to describe whether the content is muxed together or not.
|
|
||
| if (hasVideo && hasAudio && !this.audioDisabled_) { | ||
| if (hasVideo && hasAudio && !this.audioDisabled_ && !isMuxed) { |
There was a problem hiding this comment.
Should this check for the opposite? Since if the track has audio and video and audio is not disabled and isMuxed then we should be using the combined buffered.
There was a problem hiding this comment.
So in the case of isMuxed we will only ever have a videoBuffer so we only want to check videoBuffered. We could forgo this check here and rely on sourceUpdater_. Since sourceUpdater already checks what buffers exist before trying to called buffered on them.
There was a problem hiding this comment.
If we have alt audio available but muxed at first (an option of demuxed), there will be both audio and video buffers (e.g., the first bipbop in the index page).
There was a problem hiding this comment.
Yeah so we will have to check isMuxed here, as in the case of isMuxed we only care about the videoBuffer.
There was a problem hiding this comment.
Ah, so even if there are both audio and video buffers, but the stream is muxed and we're not using an alt audio playlist, then we are only appending to the video buffer, but appending both audio and video? Is there a valid case then for ever using the buffered intersection in a segment loader then?
There was a problem hiding this comment.
Thats a good question and the answer might actually be no, maybe a TODO/issue that we need to open.
There was a problem hiding this comment.
We only set isMuxed when we have a mixed fmp4/ts playlist where the fmp4 part is muxed. In that case we have to mux together video and audio to get it to keep playing.
Normally for muxed content we split it into separate audio and video in the muxer and append to different buffers. In the normal case we will hit these lines.
There was a problem hiding this comment.
Ah, so the isMuxed is from the result of the transmuxer, telling us whether we've muxed the content together or not. Got it, thanks!
|
|
||
| if (hasVideo && hasAudio && !this.audioDisabled_) { | ||
| if (hasVideo && hasAudio && !this.audioDisabled_ && !isMuxed) { |
There was a problem hiding this comment.
Ah, so the isMuxed is from the result of the transmuxer, telling us whether we've muxed the content together or not. Got it, thanks!
Co-authored-by: Garrett Singer <gesinger@gmail.com>
Description
Noticed that our fmp4/ts mixed content is failing. This is happening because we fire
trackinfoon a change when we did not before. The trackinfo that we fire for the ts segment causes us to stall palyback because we wait foraudio, but only ever receivervideo/combinedback from the transmuxer. The fix for muxed fmp4 was only reportinghasVideo. The fix for ts is exactly the same, we should not reporthasAudioif the audio and video are muxed together or else we will wait for audio appends that will never come.