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.

Sign up for GitHub

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

Jump to bottom

Rewrite all subtitle behavior #4094

Merged

nielsvanvelzen merged 3 commits into jellyfin:master from nielsvanvelzen:subtitles-fixing

Oct 20, 2024

Member

nielsvanvelzen commented Oct 19, 2024

In #3825 I made the initial changes to no longer require external subtitles. This came with a few issues; there was no longer support for external subtitles, switching subtitles didn't work a lot of times and the app didn't track which subtitles were playing properly. This pull request aims to fix all the issues by rewriting the subtitle logic.

The new logic relies on the server providing the delivery method for subs. For example, when transcoding the delivery method is set to HLS but on direct-play it is Embedded or External. The client will then decide the appropriate method to loading subtitles based on that information.

In my testing everything appears to work correctly but I'd appreciate some additional testing for:

Playback starts with the correct subtitles when they are embedded/external/baked
Switching subtitles from baking to external/embedded

There should be sufficient logging to troubleshoot issues.

Changes

Rewrite subtitle switching
Support external subtitles
Uses the URL provided by server instead of manually constructing it
Unsupport "sub" codec as I couldn't figure out which media type in ExoPlayer it belongs to so it probably never worked before

Issues

nielsvanvelzen added the enhancement label

nielsvanvelzen added this to the v0.18.0 milestone

github-advanced-security bot found potential problems

View reviewed changes

app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackControllerHelper.kt

@@ @@ -31,6 +36,99 @@ @@
               	}
               }
+              @OptIn(UnstableApi::class)
+              @JvmOverloads
+              fun PlaybackController.setSubtitleIndex(index: Int, force: Boolean = false) {

Check warning

Code scanning / detekt

One method should have one responsibility. Long methods tend to handle many things at once. Prefer smaller methods to make them easier to understand. Warning

The function setSubtitleIndex is too long (83). The maximum length is 60.

app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackControllerHelper.kt

@@ @@ -31,6 +36,99 @@ @@
               	}
               }
+              @OptIn(UnstableApi::class)
+              @JvmOverloads
+              fun PlaybackController.setSubtitleIndex(index: Int, force: Boolean = false) {

Check warning

Code scanning / detekt

Prefer splitting up complex methods into smaller, easier to test methods. Warning

The function setSubtitleIndex appears to be too complex based on Cyclomatic Complexity (complexity: 19). Defined complexity threshold for methods is set to '15'

github-advanced-security bot found potential problems

View reviewed changes

app/src/main/java/org/jellyfin/androidtv/ui/playback/GetPlaybackInfoResponse.java


		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo, Long startPositionTicks) {
		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/ui/playback/GetPlaybackInfoResponse.java


		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo, Long startPositionTicks) {
		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/ui/playback/GetPlaybackInfoResponse.java


		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo, Long startPositionTicks) {
		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/ui/playback/GetPlaybackInfoResponse.java


		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo, Long startPositionTicks) {
		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/ui/playback/GetPlaybackInfoResponse.java


		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo, Long startPositionTicks) {
		public GetPlaybackInfoResponse(PlaybackManager playbackManager, DeviceInfo deviceInfo, ApiClient apiClient, AudioOptions options, Response<StreamInfo> response, boolean isVideo) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackController.java

                   @Nullable
                   private CustomPlaybackOverlayFragment mFragment;
                   private Boolean spinnerOff = false;
-                  private VideoOptions mCurrentOptions;
-                  private int mDefaultSubIndex = -1;
+                  protected VideoOptions mCurrentOptions;

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoManager.java

+                      return flags;
+                  }
+                  public void setMediaStreamInfo(ApiClient api, StreamInfo streamInfo) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoManager.java

+                      return flags;
+                  }
+                  public void setMediaStreamInfo(ApiClient api, StreamInfo streamInfo) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

crobibero reviewed

View reviewed changes

app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoManager.java Outdated Show resolved Hide resolved


          Rewrite all subtitle behavior

50e2a95

nielsvanvelzen force-pushed the subtitles-fixing branch from d30ff5c to 50e2a95 Compare

October 19, 2024 16:51

crobibero approved these changes

View reviewed changes

Contributor

MichaelRUSF commented Oct 20, 2024

Findings

Server: 10.10.0
Commit: b3da2ff8f494c91fb19779d64db47fff521a155e

srt:
embedded – no issue
external – no issue
no subtitles – no issue

pgs:
embedded – no issue
external – player exited, to many errors (in version 0.17, it would indicate "unable to load subtitle" and continue playback)
no subtitles – no issue

ass:
embedded – burnt in
external – burnt in
no subtitles – subtitles always appear in a plain, unstyled font, without burn-in from the server


          Always use lowercase codec when mapping to mime type

17ffdd2

Member Author

nielsvanvelzen commented Oct 20, 2024

External PGS should be fixed now. The codec->mime mapping failed because the server returns PGSSUB (fullcaps) as codec and our mapping was case sensitive. Not sure why this is the only codec that happens with.

ASS/SSA should also be fixed. When the subtitle track was default/foced and the video was direct playing the subtitle track would be embedded, I missed a case where the ExoPlayer needed to be disabled. (there as a check to only force-update the ExoPlayer subtitle track when a subtitle was selected).

Thanks for testing!


          Fix not disabling embedded subtitle tracks that require baking

3a799d2

nielsvanvelzen force-pushed the subtitles-fixing branch from c8566c7 to 3a799d2 Compare

October 20, 2024 09:01

nielsvanvelzen merged commit fbfd681 into jellyfin:master

6 checks passed

nielsvanvelzen deleted the subtitles-fixing branch

October 20, 2024 13:04

Contributor

MichaelRUSF commented Oct 20, 2024 •

edited

Loading

New findings:

ass:
If "none" is set as the default option, the plain unstyled subs will appear during the first playback (direct play). If you switch to a subtitle track while playing (burn-in) and then revert to "none," the subtitle then turns off.

pgs:
I tested a file with two embedded tracks and one external track. Only the first embedded subtitle track functions; selecting a secondary or external track results in no subtitles being display.

nielsvanvelzen mentioned this pull request

Fix PlaybackController not always disabling default track #4098

Merged

Member Author

nielsvanvelzen commented Oct 20, 2024

First one fixed with #4098. For the second one it feels like there is an issue with androidx.media3 that I need some more time to investigate. It wouldn't show subtitles for me whenever there was more then one pgs track.

Contributor

MichaelRUSF commented Oct 20, 2024

I just noticed that the index is off for PGS.

In the file:
Track 1 - non-sdh pgs
Track 2 - sdh pgs

When I select track 1, it displays the sdh pgs track (track 2). Selecting track 2 leads to no subtitles being displayed.

nielsvanvelzen added the release-highlight label

MichaelRUSF mentioned this pull request

Selecting embedded subtitle track leads to different track being displayed #4183

Closed

vttc08 mentioned this pull request

Version 0.18.x using external players (Just Player), external subtitles doesn't work #4310

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement release-highlight