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?
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.