Fix dead skip-unavailable-track logic - #493
Conversation
The skip-unavailable-track handling in PlaybackNavigationService never ran. Two reasons: 1. bookToChapterMediaItems called setTag(book) without setUri. Media3 only builds a LocalConfiguration, which holds the tag, when a uri is present, so the tag was discarded at construction and the DetailedItem guard was always null. 2. The player reports the MediaItem of the MediaSource that LissenMediaSourceFactory returns, not the one it was given, since getMediaItemAt reads Timeline.Window.mediaItem. The factory forwarded only mediaMetadata, so mediaId and the FILE_SEGMENTS extras were lost and isTrackAvailable returned false for every track. Carry the chapter item's identity onto the sources the factory builds, and drop the redundant tag guard along with the no-op setTag. Both parts are needed together: restoring only the tag would let the guard pass while isTrackAvailable still failed, pausing playback on every chapter transition.
| val mediaItem = chapterMediaItem(arrayListOf(FileClip("file-1", 0.0, 30.0))) | ||
| lissenMediaSourceFactory.createMediaSource(mediaItem) | ||
|
|
||
| assertEquals(mediaItem.mediaId, capturedItem.captured.mediaId) |
There was a problem hiding this comment.
Maybe we need to implement more of the mocked mediaSourceFactory to make the test clearer?
@Before
fun setUp() {
mediaSourceFactory = mockk(relaxed = true)
every {
mediaSourceFactory.createMediaSource(any())
} answers {
val expectedMediaItem = firstArg<MediaItem>()
mockk<MediaSource>(relaxed = true) {
every { mediaItem } returns expectedMediaItem.buildUpon().build()
}
}
lissenMediaSourceFactory = LissenMediaSourceFactory(mediaSourceFactory)
}
@Test
fun media_id_and_request_metadata_preserved_for_single_segment_chapter() {
val mediaItem = chapterMediaItem(arrayListOf(FileClip("file-1", 0.0, 30.0)))
val reportedItem = lissenMediaSourceFactory.createMediaSource(mediaItem).mediaItem
assertEquals(mediaItem.mediaId, reportedItem.mediaId)
assertEquals(mediaItem.requestMetadata, reportedItem.requestMetadata)
assertEquals(mediaItem.mediaMetadata, reportedItem.mediaMetadata)
}
|
Probably even more changes are needed. Right now in #496 it turns out we depend on the fact that If someone changes the DefaultMediaSourceFactory (an implementation detail) to something else, we still test for the required condition to hold. @skorokithakis I can provide a PR to your branch if needed. |
|
@golinski that would be great, as I'm not intimately familiar with the internals here and I don't know what the right thing to do is. |
|
PR here skorokithakis#2 |
The LLM flagged this and I figured I'd clean up. Details follow:
The skip-unavailable-track handling in
PlaybackNavigationService.onPositionDiscontinuitynever ran. Two independent reasons:1. The
DetailedItemtag never existed.PlaybackService.bookToChapterMediaItemscalls.setTag(book)but neversetUri. In Media3,Builder.setTagonly writes intoLocalConfiguration, andbuild()creates one only whenuri != null:So the tag was discarded at construction and
currentMediaItem?.localConfiguration?.tag as? DetailedItemwas always null, returning early every time.2.
LissenMediaSourceFactoryalso droppedmediaIdandrequestMetadata.BasePlayer.getCurrentMediaItem()andgetMediaItemAt(i)both readgetCurrentTimeline().getWindow(i, window).mediaItem, which comes from theMediaSourcethe factory returned, not from the item passed tosetMediaItems.ClippingMediaSourcedelegatesgetMediaItem()to its child, andConcatenatingMediaSource2's window uses the item fromBuilder.setMediaItem(). The factory forwarded onlymediaMetadata, somediaIdand theFILE_SEGMENTSextras were lost, andisTrackAvailablereturnedfalsefor every track.The existing
mediaMetadataforwarding is what keptPlaybackSynchronizationService.getProgressworking viaCHAPTER_START_MS, which is good evidence this behaviour was already hit once empirically.Both parts have to be fixed together. Restoring only the tag would let the guard pass while
isTrackAvailablestill reported everything unavailable, sofindAvailableTrackIndexwould returnnulland the player wouldpause()+cancelSynchronization()on every chapter transition.Changes
LissenMediaSourceFactorycarries the chapter item's identity onto the sources it builds: the single-segment item is nowmediaItem.buildUpon().setUri(...), and the originalmediaItemis passed straight toConcatenatingMediaSource2.Builder.setMediaItem. This also removes the hand-rolled rebuild that was there before, so the factory is slightly smaller than it was.PlaybackNavigationServicedrops theDetailedItemguard. It was a redundant "is this one of our items?" check, the value was never used, and themediaIdparse inisTrackAvailablealready serves that purpose.PlaybackServicedrops the no-op.setTag(book).Scope and behaviour
LissenMediaProvider.provideFileUrionly fails when force-cache is on and the file is absent from disk, so streaming users keepisTrackAvailable == trueand see no change. Only offline users with partly downloaded books get the newly live skip behaviour.Chapters with zero file segments become
SilenceMediaSource, whosemediaIdis fixed, so they read as unavailable and get skipped. That seems right for an empty chapter.getMediaItemAt(i).mediaIdis nowchapter:<bookId>:<n>rather than the empty default, so it becomes visible to media controllers.MediaLibrarySessionCallbackandMediaRepositorydo not readmediaIdfrom the player, so no regression is expected there.Deliberately left alone, to keep this focused:
onTimelineChanged, not a discontinuity, so it never reaches this callback.onPositionDiscontinuitystill ignoresreason, so seek-triggered skipping stays in place.FORWARD, is untouched. It is unreachable without repeat mode.File.exists()inisTrackAvailableis untouched. Worst case is one stat per clip during a skip search.Testing
:app:testDebugUnitTest,:app:assembleDebug,:app:assembleDebugAndroidTestand:app:lintKotlinall pass locally; 858 unit tests, no failures.Two tests added to
LissenMediaSourceFactoryTestcovering the single-segment and multi-segment paths. The instrumented tests were not executed locally, as no emulator was available, so they run for the first time in CI here.