diff --git a/android/app/src/androidTest/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIdentityTest.kt b/android/app/src/androidTest/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIdentityTest.kt new file mode 100644 index 00000000..d2d7564f --- /dev/null +++ b/android/app/src/androidTest/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIdentityTest.kt @@ -0,0 +1,152 @@ +package com.edde746.plezy.exoplayer + +import android.content.Context +import android.net.Uri +import android.os.Handler +import android.os.HandlerThread +import android.util.Log +import androidx.media3.common.C +import androidx.media3.common.MediaItem +import androidx.media3.common.MimeTypes +import androidx.media3.common.PlaybackException +import androidx.media3.common.Player +import androidx.media3.common.Tracks +import androidx.media3.exoplayer.ExoPlayer +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import java.io.File +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith + +/** + * Pins the side-loaded subtitle identity contract against the real media3 the + * app links, on a real device. + * + * `ExoPlayerCore` tags each `MediaItem.SubtitleConfiguration` with + * [ExternalSubtitleIds.idFor] and recovers it from the `Format` the track + * selector reports. media3 does not hand that id back verbatim: + * `DefaultMediaSourceFactory` merges side-loaded subtitles with the primary + * source, and `MergingMediaPeriod` prefixes every child format id with its + * period index. Matching the raw id therefore classifies every sidecar as an + * embedded track and drops its URI, which is what broke Plex sidecar + * subtitles (#1713). + * + * A JVM test can only assert the parsing rule. This asserts that the rule + * still matches what media3 actually emits. + */ +@RunWith(AndroidJUnit4::class) +class ExternalSubtitleIdentityTest { + + private companion object { + const val TAG = "ExternalSubtitleIdentityTest" + const val SRT = "1\n00:00:00,500 --> 00:00:05,000\nplezy sidecar identity\n\n" + } + + @Test + fun sideLoadedSubtitleIdSurvivesMedia3Merging() { + val instrumentation = InstrumentationRegistry.getInstrumentation() + val context = instrumentation.targetContext + val media = copyAsset(instrumentation.context, context, "ffmpeg/planar_5_1.m4a") + val subtitle = File.createTempFile("sidecar-", ".srt", context.cacheDir).apply { + writeText(SRT) + } + + val thread = HandlerThread("plezy-sidecar-identity-test").apply { start() } + val handler = Handler(thread.looper) + val settled = CountDownLatch(1) + val playerRef = AtomicReference() + val errorRef = AtomicReference() + val textFormatId = AtomicReference() + val textGroupCount = AtomicReference(0) + + handler.post { + try { + val player = ExoPlayer.Builder(context).setLooper(thread.looper).build() + playerRef.set(player) + player.addListener(object : Player.Listener { + override fun onPlayerError(error: PlaybackException) { + errorRef.set(error) + settled.countDown() + } + + override fun onTracksChanged(tracks: Tracks) { + val text = tracks.groups.filter { it.type == C.TRACK_TYPE_TEXT } + if (text.isEmpty()) return + textGroupCount.set(text.size) + textFormatId.set(text.first().mediaTrackGroup.getFormat(0).id) + settled.countDown() + } + }) + // setMediaItem (not setMediaSource) so DefaultMediaSourceFactory owns + // the subtitle configuration exactly as ExoPlayerCore.open does. + player.setMediaItem( + MediaItem.Builder() + .setUri(Uri.fromFile(media)) + .setSubtitleConfigurations( + listOf( + MediaItem.SubtitleConfiguration.Builder(Uri.fromFile(subtitle)) + .setId(ExternalSubtitleIds.idFor(0)) + .setLabel("Plezy sidecar") + .setLanguage("en") + .setMimeType(MimeTypes.APPLICATION_SUBRIP) + .setSelectionFlags(C.SELECTION_FLAG_DEFAULT) + .build() + ) + ) + .build() + ) + player.prepare() + } catch (error: Throwable) { + errorRef.set(error) + settled.countDown() + } + } + + val finished = settled.await(30, TimeUnit.SECONDS) + val released = CountDownLatch(1) + handler.post { + playerRef.get()?.release() + thread.quitSafely() + released.countDown() + } + val teardownFinished = released.await(5, TimeUnit.SECONDS) + thread.join(5_000) + media.delete() + subtitle.delete() + + assertTrue("Player teardown timed out", teardownFinished) + assertNull("Playback failed", errorRef.get()) + assertTrue("Timed out before any text track group was reported", finished) + assertEquals("Expected exactly one side-loaded text group", 1, textGroupCount.get()) + + val reportedId = textFormatId.get() + Log.i(TAG, "media3 reported side-loaded subtitle Format.id=$reportedId") + assertNotNull("Side-loaded subtitle reported a null Format.id", reportedId) + + // The contract ExoPlayerCore depends on. + assertTrue( + "Side-loaded subtitle was not recognised as external (Format.id=$reportedId)", + ExternalSubtitleIds.isExternal(reportedId) + ) + assertEquals( + "Side-loaded subtitle did not resolve to its configuration index (Format.id=$reportedId)", + 0, + ExternalSubtitleIds.indexOf(reportedId) + ) + } + + private fun copyAsset(instrumentationContext: Context, targetContext: Context, asset: String): File { + val output = File.createTempFile("sidecar-primary-", null, targetContext.cacheDir) + instrumentationContext.assets.open(asset).use { input -> + output.outputStream().use(input::copyTo) + } + return output + } +} diff --git a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerCore.kt b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerCore.kt index f063483d..bea1d10f 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerCore.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerCore.kt @@ -1877,14 +1877,15 @@ class ExoPlayerCore(private val activity: Activity) : subtitleTrackGroupMap[trackId] = trackGroup val isSelected = group.isSelected - // Detect external (side-loaded) subtitle by the ID prefix set in open() - val isExternal = format.id?.startsWith("external_") == true - val externalIndex = if (isExternal) format.id?.removePrefix("external_")?.toIntOrNull() else null + // Detect external (side-loaded) subtitle by the ID set in open(). media3 + // rewrites merged child ids, so the tag is not the whole id. + val isExternal = ExternalSubtitleIds.isExternal(format.id) + val externalIndex = ExternalSubtitleIds.indexOf(format.id) val externalUri = externalIndex?.takeIf { it in externalSubtitleUris.indices }?.let { externalSubtitleUris[it] } val isContainer = !isExternal && externalSubtitleContainerUris.isNotEmpty() val containerUri = if (isContainer) externalSubtitleContainerUris.first() else null - Log.d(TAG, "Subtitle track $groupIndex: codec=${format.codecs}, lang=${format.language}, selected=$isSelected, external=$isExternal") + Log.d(TAG, "Subtitle track $groupIndex: formatId=${format.id}, codec=${format.codecs}, lang=${format.language}, selected=$isSelected, external=$isExternal") val track = mutableMapOf( "type" to "sub", @@ -3222,7 +3223,7 @@ class ExoPlayerCore(private val activity: Activity) : (if (isDefault) C.SELECTION_FLAG_DEFAULT else 0) or (if (isForced) C.SELECTION_FLAG_FORCED else 0) val config = MediaItem.SubtitleConfiguration.Builder(Uri.parse(subUri)) - .setId("external_$index") + .setId(ExternalSubtitleIds.idFor(index)) .setLabel(title ?: "External") .setLanguage(language) .setMimeType(mimeType ?: subtitleMimeTypeForCodec(codec) ?: detectSubtitleMimeType(subUri)) @@ -3585,7 +3586,7 @@ class ExoPlayerCore(private val activity: Activity) : val existingIndex = externalSubtitleUris.indexOf(uri) val isNew = existingIndex < 0 val index = if (isNew) externalSubtitles.size else existingIndex - val formatId = "external_$index" + val formatId = ExternalSubtitleIds.idFor(index) if (isNew) { // SELECTION_FLAG_DEFAULT marks this as the preferred text track so ExoPlayer's @@ -3632,9 +3633,10 @@ class ExoPlayerCore(private val activity: Activity) : player.prepare() player.playWhenReady = savedPlayWhenReady } else { - // Already attached — select the existing track via override. + // Already attached — select the existing track via override. The + // reported id carries media3's merge prefixes, so compare the tag. val trackId = subtitleTrackGroupMap.entries - .firstOrNull { (_, group) -> group.getFormat(0).id == formatId } + .firstOrNull { (_, group) -> ExternalSubtitleIds.indexOf(group.getFormat(0).id) == index } ?.key if (trackId != null) { selectSubtitleTrack(trackId) diff --git a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIds.kt b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIds.kt new file mode 100644 index 00000000..e45bb8f8 --- /dev/null +++ b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIds.kt @@ -0,0 +1,40 @@ +package com.edde746.plezy.exoplayer + +/** + * Identity of a side-loaded subtitle across media3's media-source merging. + * + * Each `MediaItem.SubtitleConfiguration` is tagged with [idFor], and the tag is + * read back off the `Format` the track selector reports. The tag does not + * survive verbatim: `DefaultMediaSourceFactory.createMediaSource` always wraps + * the primary source plus one source per subtitle configuration in a + * `MergingMediaSource`, and since media3 1.3.0 `MergingMediaPeriod.onPrepared` + * rewrites every child format id to `":"`. A second + * merge — the one this player builds for container sidecars — prefixes it + * again. So the same configuration can surface as `external_0`, + * `1:external_0`, or `0:1:external_0`. + * + * Only the final `:`-separated segment carries the tag, which is why matching + * on the whole id silently classifies every sidecar as an embedded track. + */ +internal object ExternalSubtitleIds { + private const val PREFIX = "external_" + + /** Tag written onto the [index]th side-loaded subtitle configuration. */ + fun idFor(index: Int): String = "$PREFIX$index" + + /** Whether [formatId] identifies a side-loaded subtitle. */ + fun isExternal(formatId: String?): Boolean = tagOf(formatId) != null + + /** + * Index passed to [idFor], or null when [formatId] is not a side-loaded + * subtitle or carries a tag this build did not write. + */ + fun indexOf(formatId: String?): Int? = tagOf(formatId)?.toIntOrNull() + + private fun tagOf(formatId: String?): String? { + // substringAfterLast returns the whole string when no ':' is present, which + // is the unmerged case. + val segment = formatId?.substringAfterLast(':') ?: return null + return if (segment.startsWith(PREFIX)) segment.removePrefix(PREFIX) else null + } +} diff --git a/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIdsTest.kt b/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIdsTest.kt new file mode 100644 index 00000000..939d6b72 --- /dev/null +++ b/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExternalSubtitleIdsTest.kt @@ -0,0 +1,61 @@ +package com.edde746.plezy.exoplayer + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +class ExternalSubtitleIdsTest { + + @Test + fun unmergedTagResolves() { + assertTrue(ExternalSubtitleIds.isExternal("external_0")) + assertEquals(0, ExternalSubtitleIds.indexOf("external_0")) + assertEquals(7, ExternalSubtitleIds.indexOf("external_7")) + } + + @Test + fun mediaSourceFactoryMergePrefixResolves() { + // DefaultMediaSourceFactory always merges side-loaded subtitles with the + // primary source, so MergingMediaPeriod prefixes the period index. + assertTrue(ExternalSubtitleIds.isExternal("1:external_0")) + assertEquals(0, ExternalSubtitleIds.indexOf("1:external_0")) + assertEquals(2, ExternalSubtitleIds.indexOf("3:external_2")) + } + + @Test + fun containerSidecarOuterMergePrefixResolves() { + // The container-sidecar path wraps that merge in a second MergingMediaSource. + assertTrue(ExternalSubtitleIds.isExternal("0:1:external_0")) + assertEquals(0, ExternalSubtitleIds.indexOf("0:1:external_0")) + assertEquals(4, ExternalSubtitleIds.indexOf("0:2:external_4")) + } + + @Test + fun embeddedAndUnknownIdsAreNotExternal() { + assertFalse(ExternalSubtitleIds.isExternal(null)) + assertFalse(ExternalSubtitleIds.isExternal("1:2")) + assertFalse(ExternalSubtitleIds.isExternal("0:")) + // A container child whose own id merely ends in the tag's text. + assertFalse(ExternalSubtitleIds.isExternal("1:not_external_0")) + assertNull(ExternalSubtitleIds.indexOf("1:2")) + } + + @Test + fun malformedTagIsExternalWithoutAnIndex() { + // Still a side-loaded track, but no usable URI lookup. + assertTrue(ExternalSubtitleIds.isExternal("1:external_x")) + assertNull(ExternalSubtitleIds.indexOf("1:external_x")) + } + + @Test + fun writtenIdRoundTrips() { + for (index in 0..3) { + val id = ExternalSubtitleIds.idFor(index) + assertEquals(index, ExternalSubtitleIds.indexOf(id)) + assertEquals(index, ExternalSubtitleIds.indexOf("1:$id")) + assertEquals(index, ExternalSubtitleIds.indexOf("0:1:$id")) + } + } +} diff --git a/lib/screens/video_player_screen.dart b/lib/screens/video_player_screen.dart index b8e283db..ec50f194 100644 --- a/lib/screens/video_player_screen.dart +++ b/lib/screens/video_player_screen.dart @@ -262,8 +262,7 @@ _PlaybackOpenTiming _playbackOpenTiming({ /// change should not silently rewrite the whole series' Plex prefs. The /// explicit path for that lives in the metadata-edit UI. TrackPreferencePersister _plexTrackPersister(PlexClient? Function() resolve) { - return ({required int partId, required String trackType, int? streamID}) async { - if (streamID == null) return; + return ({required int partId, required String trackType, required int streamID}) async { final client = resolve(); if (client == null) return; await (trackType == 'audio' diff --git a/lib/services/track_manager.dart b/lib/services/track_manager.dart index 1340a804..706f3995 100644 --- a/lib/services/track_manager.dart +++ b/lib/services/track_manager.dart @@ -18,7 +18,7 @@ import '../utils/track_label_builder.dart'; /// stream indexes) or lack server-side stream selection leave this null. /// [trackType] is `'audio'` or `'subtitle'`. typedef TrackPreferencePersister = - Future Function({required int partId, required String trackType, int? streamID}); + Future Function({required int partId, required String trackType, required int streamID}); /// Manages track (audio + subtitle) lifecycle: external subtitle loading, /// automatic track selection, server preference sync, and cycling. @@ -530,7 +530,15 @@ class TrackManager { } /// Save the stream selection for the current part to the server. + /// + /// A null [streamID] means no server stream could be identified for the + /// chosen track. There is no local fallback store, so the choice is simply + /// lost — say so instead of reporting a save that never happened. Future _saveTrackPreferences({required int partId, required String trackType, int? streamID}) async { + if (streamID == null) { + appLogger.w('Not saving $trackType stream selection: no server stream matched the selected track'); + return; + } try { if (!isActive()) return; final persist = persistTrackPreference; diff --git a/test/services/track_manager_test.dart b/test/services/track_manager_test.dart index e0496d19..c9ed7153 100644 --- a/test/services/track_manager_test.dart +++ b/test/services/track_manager_test.dart @@ -194,7 +194,7 @@ Future _drainAsync() async { } } -Future _noopPersister({required int partId, required String trackType, int? streamID}) async {} +Future _noopPersister({required int partId, required String trackType, required int streamID}) async {} void main() { // The constructor doesn't touch prefs, but [dispose] / [applyTrackSelection] @@ -1515,7 +1515,7 @@ void main() { final mgr = _make( player: player, mediaInfo: info(), - persister: ({required int partId, required String trackType, int? streamID}) async { + persister: ({required int partId, required String trackType, required int streamID}) async { captured = streamID; }, ); @@ -1535,7 +1535,7 @@ void main() { final mgr = _make( player: player, mediaInfo: info(), - persister: ({required int partId, required String trackType, int? streamID}) async { + persister: ({required int partId, required String trackType, required int streamID}) async { captured = streamID; }, ); @@ -1552,7 +1552,7 @@ void main() { final mgr = _make( player: player, mediaInfo: info(), - persister: ({required int partId, required String trackType, int? streamID}) async { + persister: ({required int partId, required String trackType, required int streamID}) async { captured = streamID; }, ); @@ -1561,6 +1561,33 @@ void main() { await mgr.onSubtitleTrackChanged(const SubtitleTrack(id: 'native-without-metadata'), sourceStreamId: 32); expect(captured, 32); }); + + test('does not persist anything when the track maps to no server stream', () async { + // #1713: an unmappable native track used to reach the persister with a + // null streamID, which short-circuited before the request while the + // manager still reported a successful save. Nothing is stored locally, + // so a silent no-op loses the choice on the next start. + await SettingsService.getInstance(); + const unknown = SubtitleTrack(id: '2_9', language: 'jpn', codec: 'ass'); + final player = _FakePlayer(tracks: const Tracks(subtitle: [...playerSubs, unknown])); + var persistCalls = 0; + final mgr = _make( + player: player, + mediaInfo: info(), + persister: ({required int partId, required String trackType, required int streamID}) async { + persistCalls++; + }, + ); + addTearDown(mgr.dispose); + + await mgr.onSubtitleTrackChanged(unknown); + expect(persistCalls, 0); + + // Same manager and fixture: a mappable track still persists, so the + // assertion above is about the unmatched track, not a disabled path. + await mgr.onSubtitleTrackChanged(playerSubs[0]); + expect(persistCalls, 1); + }); }); // ============================================================