fix(exoplayer): match side-loaded subtitles after media3 rewrites track ids
Plex sidecar subtitles are attached as MediaItem.SubtitleConfiguration and tagged `external_<n>`, then recovered from the Format id the track selector reports. Since media3 1.3.0, DefaultMediaSourceFactory always merges side-loaded subtitles with the primary source and MergingMediaPeriod rewrites every child format id to "<periodIndex>:<originalId>", so the tag arrives as "1:external_0" - measured on device - or "0:1:external_0" behind the container-sidecar merge. The prefix test therefore never matched and every sidecar reached Dart as an embedded track with no URI. A Plex sidecar's only identity is its stream key, which the app carries in that URI, so both matchers failed on it: a server-selected sidecar could never resolve and left subtitle selection pending, and a manually chosen one could not be mapped back to a stream id to write to the server. The already-attached branch of addSubtitleTrack compared the raw id too, so re-selecting a loaded sidecar silently did nothing. Route every write and readback of the tag through ExternalSubtitleIds, which matches the final id segment, and cover it with an instrumentation test that side-loads a subtitle through the real media3 media-source factory. Also stop claiming a saved track selection when no server stream was identified - there is no local store, so that path silently dropped the user's choice. close #1713
This commit is contained in:
+152
@@ -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<ExoPlayer>()
|
||||
val errorRef = AtomicReference<Throwable>()
|
||||
val textFormatId = AtomicReference<String>()
|
||||
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
|
||||
}
|
||||
}
|
||||
@@ -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<String, Any?>(
|
||||
"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)
|
||||
|
||||
@@ -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 `"<periodIndex>:<originalId>"`. 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
|
||||
}
|
||||
}
|
||||
@@ -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"))
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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'
|
||||
|
||||
@@ -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<void> Function({required int partId, required String trackType, int? streamID});
|
||||
Future<void> 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<void> _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;
|
||||
|
||||
@@ -194,7 +194,7 @@ Future<void> _drainAsync() async {
|
||||
}
|
||||
}
|
||||
|
||||
Future<void> _noopPersister({required int partId, required String trackType, int? streamID}) async {}
|
||||
Future<void> _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);
|
||||
});
|
||||
});
|
||||
|
||||
// ============================================================
|
||||
|
||||
Reference in New Issue
Block a user