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 ,

ExoPlayer/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsSampleStreamWrapper.java

Line 1253 in aedde2d

// TODO: Keep sample metadata to allow restoring these chunks [internal b/159904763].

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]

The decision to splice in depends on whether we are guaranteed that either
(1) The next chunk is a continuation of the previous one (checked above at L217) or
(2) The next chunk starts with a keyframe (currently checked by isIndependent)
AND its first frame comes after the start time of the last frame of the previous chunk. (currently checked by segmentStartTime < previousEndTime)

I think we can make this condition more targeted to also work in cases where not both chunks are from a trick-play tracks. 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. So I think it could be something like:

// Need to splice in if the next chunk doesn't start with a keyframe.
if (!isIndependent(...) && !nextChunkIsTrickPlay) {
  return true;
}
// Need to splice in if chunks are overlapping.
return segmentStartTimeInPeriodUs < previousChunk.startTimeUs
    || (!previousChunkIsTrickPlay && segmentStartTimeInPeriodUs < previousChunk.endTimeUs);

[stevemayhew]

Yes, that would be great. Right now, the way trickplay track selection is coded, we have to cause a full new TrackSelection with either all iFrame only or all non-iFrame only tracks so this use case does not occur.

Eventually we want this dynamic based on a PlaybackParameter so we would have adaption (and of course require discard on trickPlay to non-TrickPlay track) between tracks. I want to get to adding this to the main demo and submitting pull requests for all of it, internally our first priority is to catch up our branch to nearer the latest ExoPlayer version.

[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 guess this returns true with my proposed logic because it really needs to be spliced in?

If I read the example data correctly, previousChunk has dense (non-trick-play) samples from 0 to 4 seconds and we now attempt to read an i-frame-only chunk from 2 to 4 seconds. This chunk has to be spliced in because we may have written the dense samples between 2 and 4 seconds already and need to replace them by the samples from the new chunk. I'd assume the test works if you choose a trick-play segment starting at 4 seconds?

I think the test the other way round is even more interesting (trick-play to non-trick-play) because you should be able to choose a dense track starting without splicing?

[stevemayhew]

So the splice will effectively discard the downloaded "lower res" media like evaluateQueueSize() with discard does in favor of the adaptive switch occurs?

If this is true (more dense samples from non-trickplay overwrite (replace) the less dense trick-play samples then yes, I agree I need to add a test case and support trick-play to non-trick play mode. We are handling this non-adaptively now by forcing a seek and a completely new TrackGroup (so sample queue is flushed), but it would be better if this could happen entirely in the adaptive switch. For obvious reasons you would not want to continue with a very low frame rate set of samples after switching back to normal playback.

[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