Allows discard of overlapping iFrame only chunks

[stevemayhew]

Consider two I-Frame only playlists, to allow adaption based on frame rate the two playlist have different I-Frame duration.

The I-Frame only segment has a single sample (IDR), of sub-second duration (e.g. 0.0333s for 30fps source) but a duration equal to the time distance between the I-Frames.

Playlist P1 -- 6 seconds between IDR samples, and Playlist P2 -- 4 seconds between IDR samples:

+-----+-----+-----+
|  1  |  2  |  3  |
+-----+-----+-----+
+---+---+---+---+---+
| 1 | 2 | 3 | 4 | 5 |
+---+---+---+---+---+

If the AdaptiveTrackSelection changes from P2 after loading segments from P1 there appears to be a sample overlap that would trigger shouldSpliceIn to be set on the iFrame MediaChunk when no sample splicing is required.

+-----+---
|  1  |  2    --- loaded in P1
+-----+---
        +---+---+---+
        | 3 | 4 | 5 |  -- switched to P2
        +---+---+---+

Because the startTimeUs of P2 segment 3 is less then the endTimeUs of P1 segment 3 it appears samples would overlap, as there is only one sample in each segment this is not possible. Setting this shouldSpliceIn flag incorrectly prevents pruning the buffered chunks.

[stevemayhew]

@tonihei Not sure where this fits in with the plan to eliminate the fact that you cannot discard chunks that have been spliced , https://github.com/google/ExoPlayer/blob/aedde2de396995e4354f3110cc37e86b48525892/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsSampleStreamWrapper.java#L1253 Can I see the internal bug internal b/159904763 somewhere to better understand?

Hopefully the commit comment makes the use case clear. Basically if lower frame rate iFrame only stream is buffered (for higher speed playback) then you need to be able to discard that in favor of a higher frame rate track (for slower speed playback).

The current logic for detecting overlay falsely marks the two chunks as having overlapping samples. This change fixes that.

[tonihei]

Thanks for the PR and the well-written problem description!

We should be able to merge this, but please see my suggestion to make it more targeted and to cover more edge cases.

Not sure where this fits in with the plan to eliminate the fact that you cannot discard chunks that have been spliced

I think this is independent of this feature I'd say. Allowing to restore spliced-in chunks is good, but avoiding the splice is probably even better.

Can I see the internal bug internal b/159904763 somewhere to better understand?

Well, no, you can't see it directly, because it's internal :) But there is also nothing to see because it just says:

Allow to restore spliced in sample metadata In HLS, we sometimes need to splice in chunks into the SampleQueue. This prevents us from discarding chunks upstream where another chunk potentially removed sample data from the new last chunk in the queue. However, we just remove the metadata and not the sample data itself. This means if we somehow keep the metadata of the "removed" samples, we would be able to restore a spliced-in chunk completely.

We also haven't planned to implement this any time soon. Happy to review PRs though if you think you'd need this feature.

[stevemayhew]

Thanks for the PR and the well-written problem description!

We should be able to merge this, but please see my suggestion to make it more targeted and to cover more edge cases.

Your welcome, let's hold off on the merge until I add your suggestions and some test cases. This will be a little bit as we are moving our codebase to 2.15 right now (which will make this commit much easier to back port directly to dev-v2).

So, I owe you that change and some test cases (which also will be easy with this being a static method now)

Not sure where this fits in with the plan to eliminate the fact that you cannot discard chunks that have been spliced

I think this is independent of this feature I'd say. Allowing to restore spliced-in chunks is good, but avoiding the splice is probably even better.

Thanks, avoiding the splice is all we really need, as the discard is essential for proper shifting of trickplay tracks.

[stevemayhew]

I think we can make this condition more targeted to also work in cases where not both chunks are from a trick-play tracks.

Maybe, I added one test case (that is @Ignored and does not pass with current logic, or the suggested logic.

And there is also the edge case of trick play tracks whose start time is further in the past than the current trick play track.

Not sure the edge case you are seeing, but I'm sure it exists. Complicated logic! (which is why I thought test cases were a good idea)

I'm still not sure I see the use case for ever splicing when switching to/from an iFrame only track... But perhaps I'm still not really getting it.

Consider the exiting trick-play use case, here the idea would be to show the exact same position as the last trick-play frame rendered in the normal track. Consider 6 second segments with 3 second GOP:

+--+-x+--+--+--+--+
|1a|1b|2a|2b|3a|3b|    -- playlist 2 (iFrame only)
+--+-x+--+--+--+--+

+-----+-----+-----+
|  1  |  2  |  3  |    -- playlist 1 (normal)
+-----+-----+-----+

The x marks current playback position in the iFrame only track, note the IDR frame 1b is showing for 2 seconds (scale to the speed of course). I'm not sure how an adpative track selection to playlist 1, spliced or not spliced, would yield the desired visual result?

The UX for trick play exit use case would seek to the last rendered frame's position so visually normal playback starts at the same frame last rendered. To do this the segment load would be playlist 1 segment 1 and seek would discard (decode only) up to the position (likely an matching IDR).

Thanks for your time and input on this, really appreciated.

[tonihei]

I'm still not sure I see the use case for ever splicing when switching to/from an iFrame only track...

I can see this being used when an adaptive track selection tries to play i-frame-only tracks for high speeds and switches back to normal tracks for lower speeds.

My general thinking was that it would be nice to cover all potential input values now instead of only a subset of potential values. But I can see how your change is achieving this as well because it's just stricter than the other suggestion and doesn't leave out any cases. So feel free to ignore if you like!

I added one test case (that is @ignored and does not pass with current logic, or the suggested logic.

If we don't support this functionality, then we can also this test I'd say. Thanks for trying it out though and adding new test classes!

One final remark - do you see any chance you can rebase this change to the main branch of the Media3 repo? This greatly simplifies the merging process for us. If that's not feasible, we can still import it here if needed.

[stevemayhew]

I'm still not sure I see the use case for ever splicing when switching to/from an iFrame only track...

I can see this being used when an adaptive track selection tries to play i-frame-only tracks for high speeds and switches back to normal tracks for lower speeds.

So the splice will effectively discard the downloaded "lower res" media like evaluateQueueSize() with discard does in favor of the adaptive switch occurs? If so, yes that would be nice to have.

My general thinking was that it would be nice to cover all potential input values now instead of only a subset of potential values. But I can see how your change is achieving this as well because it's just stricter than the other suggestion and doesn't leave out any cases. So feel free to ignore if you like!

I wholeheartedly agree, once we have the use case and test case to match happy to add this, better to do it now in one shot and cover all the cases.

One final remark - do you see any chance you can rebase this change to the main branch of the Media3 repo? This greatly simplifies the merging process for us. If that's not feasible, we can still import it here if needed.

Yes, that should be easy enough. Once we finalize the logic and test cases and review I'll rebase/squash to a single commit and then I can rebase to main for media3, would I open a pull request there too?

[tonihei]

I can rebase to main for media3, would I open a pull request there too

Yes, by "rebase" I meant open a new PR in the media3 repository. The code should be exactly the same expect for the package names.

[icbaker]

Closing all PRs on this deprecated project. We are now unable to merge PRs from here. If you would like us to consider this change again please file a new PR on the media3 project: https://github.com/androidx/media/pulls