Allow adjusting sideloaded subtitle time offsets during playback - #3370
Allow adjusting sideloaded subtitle time offsets during playback#3370fluffypony wants to merge 4 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
CLA signed🫡 |
| /** | ||
| * Updates the offset that is applied to all timestamps coming from the wrapped source. | ||
| * | ||
| * <p>Must be called on the playback thread. |
There was a problem hiding this comment.
I wonder if there is any value left for a stand-alone TimeOffsetMediaSource if the update needs to happen on the playback thread anyway, which implies it needs to cooperation of another wrapping media source as proposed in the SideloadedSubtitlesMediaSource. Would it make sense to integrate all of this logic into SideloadedSubtitlesMediaSource directly?
There was a problem hiding this comment.
Yeah that's fair - my understanding is that a package-private class has exactly one caller, so it doesn't warrant standing alone necessarily. The only reason it exists separately is that the offset has to be applied per subtitle child inside the merge, while the MediaItem-update handling has to sit outside it, and I was thinking to use two classes to keep those jobs apart.
Lemme go with your idea tho - the per-child wrapper becomes a private inner class of SideloadedSubtitlesMediaSource, and while I'm at it I'll move the assembly there too - then the constructor can take the content source plus the plain subtitle sources and build the MergingMediaSource and wrappers itself, which gets all the offset knowledge out of DefaultMediaSourceFactory. TimeOffsetMediaPeriod stays where it is since MergingMediaPeriod also uses it.
Will push an update shortly.
| * The offset that is added to the timestamps of the cues in this subtitle track, in | ||
| * microseconds. See {@link Builder#setTimeOffsetUs(long)} for details. | ||
| */ | ||
| @UnstableApi public final long timeOffsetUs; |
There was a problem hiding this comment.
This looks like a nice way to integrate it and allow the dynamic updates, thanks for the proposal.
@icbaker Do you have any additional API thoughts around this?
|
Some demand context from another client family, in case it is useful while this is in review. The Jellyfin Android TV maintainer stated yesterday that they will not implement subtitle offset in the app and consider this an upstream matter: jellyfin/jellyfin-androidtv#2479 (comment)
That makes it the third client in the Jellyfin family in the same position, alongside the Findroid and Moonfin cases already in your description. Two attempts have been made in that repo and both died: the request has been open since February 2023, and the most recent implementation attempt leaned on One scoping note, offered as information rather than as a request to widen the PR. That client delivers server provided external subtitles as Subtitle tracks embedded in the container take a different path and would not be. For that library the sideloaded case is the common one for out of sync subtitles, since those are the tracks fetched separately, so the sideloaded scope covers most of the real complaints. I run a Jellyfin server with a Fire TV Stick client and a good supply of genuinely out of sync subtitles. If it is useful I am happy to build this branch against that client and report back on how the offset behaves with real content, including the mid playback change and the re-timing of the cue already on screen. Just say the word if that would help, and tell me if there is a particular case you want exercised. |
|
Correcting my offer above: the Fire TV Stick I mentioned is not a device I control, so I cannot run this branch on it. I should not have offered that. Apologies for the noise. What I did manage to do is build the branch and integrate it into a real client, so here is what came out of that, in case any of it is useful. I built One practical note for anyone else trying this against an app: the branch is based on and the same for To be clear about what I did not do: I have not run this at playback time, so I am not reporting anything about runtime behaviour, the mid playback offset change, or the track toggle. The above is build and integration only. |
Fixes #1976 - the original request is google/ExoPlayer#854, open since 2015.
The short version: this adds
MediaItem.SubtitleConfiguration.timeOffsetUs(@UnstableApi), an offset added to the cue timestamps of a sideloaded subtitle track - positive shows cues later, negative earlier - and it can be changed during playback without interrupting anything, which is what subtitle sync UIs actually need.How it works:
DefaultMediaSourceFactorywraps each sideloaded subtitle source in a package-privateTimeOffsetMediaSourcewhen subtitles are parsed during extraction (the default). The deprecated legacy decoding path ignores the offset; the javadoc says so.replaceMediaItemmachinery: a new package-privateSideloadedSubtitlesMediaSourcewrapper implementscanUpdateMediaItem/updateMediaItem, accepts updates where only the time offsets changed, and pushes the new offsets down to the live media periods. Same patternClippingMediaSourceuses for dynamic clip updates. Anything structural (adding, removing or re-labelling subtitle tracks) returns false, so the player falls back to a normal item replacement - previously areplaceMediaItemwith changed subtitle configurations was silently accepted and changed nothing.TextRendereronly reads ahead 1s). To re-time the cue that's already on screen, the app disables and re-enables the text track: the subtitle period is single-track, so re-selection forces an internal seek and the cues around the current position get re-read with the new offset. This is the approach ojw28 suggested in the original issue back in 2015, translated to the current parse-during-extraction pipeline - the offset lives at the media source level, not in the renderer, for the reasons icbaker laid out there.TimeOffsetMediaPeriodgainsupdateTimeOffsetUs; beyond that and the factory wiring there are no changes to existing classes, and no new publicMediaSourceAPI.Tests: bundle/equals round-trips for the new field,
TimeOffsetMediaPeriodoffset updates,canUpdateMediaItemsemantics inDefaultMediaSourceFactoryTest, and three end-to-end Robolectric playback tests - positive shift, negative shift, and a mid-playback offset change that asserts the exact re-timed cue output and that the player never re-buffers.Why this matters: every player with user-facing subtitle sync (VLC, mpv, Kodi) has this, and third-party ExoPlayer apps keep asking for it. The Jellyfin family is a good example: Findroid and Moonfin ship subtitle offset controls for their mpv backend but had to disable the feature on ExoPlayer entirely - in the Findroid maintainer's words, "this does not work on ExoPlayer because I can't find a native way to do it" (jarnedemeulemeester/findroid#1113). The known workaround (a copy of
TextRendererthat shiftspositionUs) was ruled out in the original issue and breaks in the general case, so nobody can carry it upstream.Happy to rename the field or move the API surface around if you'd prefer a different shape - naming aside, the mechanics here try to follow the guidance the team already gave in ExoPlayer#854.