sub: fix subs being invisible after track reinits during pause #10176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We need to account for the edge case where both the stream is eof and the reader_head is NULL. In this case we still need to return 0 (wait) if the lazy stream requires it (i.e. if we’re force reading subs). This may occur when for example - the sub demuxer queue contains a packet, so it’s marked as eof by read_packet - then, dequeue_packet is called twice in quick succession, so that after the first call, the readerhead is NULL and the stream is still eof because the demuxer thread hasn’t had time to respond to the readerhead advancement and read in a new packet.
Previously, subs were updated before the data for the current pts was been demuxed, which means they were not decoded and displayed until the next update (e.g. when the next frame was rendered). This wasn’t an issue during playback (since the next frame follows quickly) but it was during pause. Thus, when track switching during pause we need to block until the relevant data is available (or until we know for sure there isn’t any).
Lypheo
changed the title
Traneptora
reviewed
Jul 17, 2022
| @@ -634,7 +634,7 @@ void reinit_sub_all(struct MPContext *mpctx); | |||
| void uninit_sub(struct MPContext *mpctx, struct track *track); | |||
| void uninit_sub_all(struct MPContext *mpctx); | |||
| void update_osd_msg(struct MPContext *mpctx); | |||
| bool update_subtitles(struct MPContext *mpctx, double video_pts); | |||
| bool update_subtitles(struct MPContext *mpctx, double video_pts, bool force_read_ahead); | |||
There was a problem hiding this comment.
Does this change the libmpv ABI?
Traneptora
reviewed
Jul 17, 2022
| // so we need to loop the call. We also need to set | ||
| // force_read_ahead to make sure we wait until the | ||
| // demuxer has caught up to the current position. | ||
| if (mpctx->paused) |
There was a problem hiding this comment.
Why not while(!update_subtitles(mpctx, mpctx->playback_pts, mpctx->paused) && mpctx->paused);
Traneptora
reviewed
Jul 17, 2022
| @@ -81,7 +81,7 @@ void uninit_sub_all(struct MPContext *mpctx) | |||
| } | |||
|
|
|||
| static bool update_subtitle(struct MPContext *mpctx, double video_pts, | |||
| struct track *track) | |||
| struct track *track, bool force_read_ahead) | |||
There was a problem hiding this comment.
Is there any particular reason we need this argument and can't just use mpctx->paused as the value of this bool instead?
|
Superseded by a323dfa |
|
Dudemanguy singlehandedly saving mpv 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the issue where switching subtitles or dis- and reenabling subs during pause causes the subs to disapper until the next frame is rendered (i.e. until playback is resumed or a seek/frame step is issued). I am not confident my solution is ideal (for example I am not sure if there isn’t a place where you could just call update_subtitles at a later point in time when the demuxer is guaranteed to be caught up), but it works and has negligible performance impact.