-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 jump-back of needed segment time for text stream after first update of media state failed with unavailable segment #839
Fix jump-back of needed segment time for text stream after first update of media state failed with unavailable segment #839
Conversation
…m's media state failed with unavailable segment
I'm not convinced that this is the correct solution to your problem. resumeAt is meant to override the playhead time when we are switching periods. It's an indirect solution to #715, and was introduced in 2cc68b2. Once we have consumed resumeAt, it should be reset to 0. It is used like Can you produce a failing unit test that demonstrates the bug you're trying to fix? That might help us understand and fix your issue. |
I think resumeAt is used to mark the segment time that need to be append to the buffer (aka timeNeeded) rather than the playhead. Please correct me if wrong. So setting the resumeAt to |
@albertcsm You are correct that it indicates the next segment to download; but @joeyparrish is also correct about needing to reset it once we use it. Once we have downloaded a segment, we will download the next segment, not look back at the playhead time. The problem (I think) is that we are resetting the value too soon in the method. Your logs show that when we tried to fetch the segment from the next Period, it wasn't available yet. So we wait a second and try again. The problem is that now the I think the solution should be to move the setting it to 0 to the end of the method where we actually append the segment. |
@TheModMaker I agree it is better to move the "setting it to 0" to the end, would you like me to update the fix? |
I think we will handle the fix so I can include some tests. Thanks for finding this. |
Based on PR #839 We use |resumeAt| to handle switching between Periods that do and do not have text streams. It is used to override what time we need to start buffering from. Once we have used it (i.e. started streaming), we should reset it (to ensure it doesn't break seeking). There was a bug where if the stream wasn't available yet, it we would reset |resumeAt| but not start streaming. Meaning on the next update, we would check at the playhead time instead of the next Period time. Now we reset |resumeAt| only after we know we have started streaming. Closes #839 Change-Id: Ibd3ce680cec129719869c8d4a7dda409b573a17f
The fix for this has been cherry-picked and will be released in v2.1.5. |
Ocassionally, when playing a multiple-period DASH with text stream on and off, I got "Assertion failed: All MediaStates should need the same Period be performing updates." and player stuck after a few seconds.
The log show that there is a jump-back of the "timeNeeded" for text stream. The jump-back was due to resetting of the "resumeAt" variable between updates of the media state. After removing the line that resets "mediaState.resumeAt", the issue was resolved.