Implements SeekParameters.*_SYNC variants for HLS #9536

stevemayhew · 2021-10-06T16:52:18Z

The HLS implementation of getAdjustedSeekPositionUs() now completely supports SeekParameters.CLOSEST_SYNC and it's brotheran, assuming the HLS stream indicates segments all start with an IDR (that is EXT-X-INDEPENDENT-SEGMENTS is specified).

This fixes issue #2882 and improves (but does not completely solve #8592

stevemayhew · 2021-10-06T17:04:46Z

@ojw28 Here is the first step to solving the faster first render requested in #8592. In my testing on BRCM and AmLogic platforms it does not completely solve the issues, but we are looking at ways to intimidate the decoder into presenting a single IDR immediately.

At any rate, here's a fix for one bug on your plate :-). I am happy to split out the test cases into a set that test the logic in SeekParameters.resolveSeekPositionUs() as a separate unit-test (seemed more proper way), I did it this way to limit the change to a single module. It could also be tech-debt I'm happy to take on.

Still working on a design for the fix for #8952. Haven't forgotten about this one.

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

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

library/hls/src/test/java/com/google/android/exoplayer2/source/hls/HlsChunkSourceTest.java

ojw28 · 2021-10-14T18:23:25Z

library/hls/src/test/java/com/google/android/exoplayer2/source/hls/HlsChunkSourceTest.java

+  public void setup() throws IOException {
+    // sadly, auto mock does not work, you get NoClassDefFoundError: com/android/dx/rop/type/Type
+//    MockitoAnnotations.initMocks(this);
+    mockExtractorFactory = Mockito.mock(HlsExtractorFactory.class);


Why can't you just pass a real instance (i.e., HlsExtractorFactory.DEFAULT)? If this works then it's generally much better than mocking. In particular because this test doesn't randomly start failing due to some unrelated change that ends up meaning the mock starts to be called.

I'll give it a try, yes sadly Mockito comes with a host of issues with Android mocks in Roboelectric. Also, I'm fine with moving most of this to a test of SeekParameters directly. Then it covers all users of that code not use HLS.

Only HLS specific stuff will be empty playlist and if we can test track switching (ABR)

library/hls/src/test/java/com/google/android/exoplayer2/source/hls/HlsChunkSourceTest.java

The HLS implementation of `getAdjustedSeekPositionUs()` now completely supports `SeekParameters.CLOSEST_SYNC` and it's brotheran, assuming the HLS stream indicates segments all start with an IDR (that is EXT-X-INDEPENDENT-SEGMENTS is specified). This fixes issue google#2882 and improves (but does not completely solve google#8592

stevemayhew · 2021-10-18T17:43:36Z

@ojw28 I think all of the comments/issues have been addressed. Do let me know, thanks so much for the review and catching the bug with MediaPeriod vs Timeline / playlist relative time.

ojw28 · 2021-11-03T21:21:04Z

@stevemayhew - Are you certain you pushed all of your changes? Looking at your comment starting "I deleted the comment", it appears the comment is still there. That's not particularly important in itself, but it's very hard to tell whether there are also other changes that haven't been pushed?

I'm not sure if you got into a bad state and were forced into it, but force pushing a single commit on top of a previous review makes it incredibly hard to work out what changed. Please just push additional commits, if possible.

stevemayhew · 2021-11-03T21:30:16Z

@ojw28 Yes I updated the commit and did a force-push. Sure, in the future I'll make new commits each time for reviews you are on. Some people don't like lots of commits on a pull request branch, but I can see addressing review comments it is nice to see the diffs.

I do like when people interactive rebase to a single change before the final merge, that makes a cleaner history. But next time, I'll have you let me know when you are ready to merge then I can do that before the final merge.

Thanks for the review... BTW, the change does speed up paused seeks on i-Frame only playlist a bit. I've got some other things I'm working on for #8592 as well.

Once it is all a bit more baked I can add a track selector setting for iFrame only mode and add the scrub seek to the demo app.

ojw28 · 2021-11-03T21:39:30Z

So is the comment not being deleted just an omission? I guess I saw that along with the "done" comment, and was then unsure whether there are other accidental omissions too (and can't easily see a diff to check). If you could confirm that everything is as it should be, and that the comment not being deleted was just a singular omission, that would be great. Thanks!

ojw28 · 2021-11-03T21:40:04Z

In case it's not completely clear what I'm referring to, I'm referring to this (comment in the code is still there, but comment in the discussion thread says the comment in the code has been deleted):

https://github.com/google/ExoPlayer/pull/9536/files#diff-a500e9307d881d3b7b6a92999132c39808322d24b6f6a4a57a1c0d03cd6cf513R260

stevemayhew · 2021-11-04T02:27:30Z

Let me double check it all over and diff against our version (which is very close, but based from release 2.12) just to be double sure.

The comment was not removed, it is misleading.

The resolved sync point position will be valid in any variant, however it may not match a segment boundary. If for example there were sub-segment duration EXT-X-GAP segments in only one playlist such as to cause an disparate list of segments inter-variant.

This actually will cause the MSN based algorithm later in this class to pick in incorrect segment on track switch, so probably better just to add a bug

The comment "all should be same" is not correct, it is extremely likely they will be the same but not assured. In either case the seek position will work, it just may not land exactly on a sync point in every variant.

stevemayhew · 2021-11-05T20:12:51Z

@ojw28 I double checked all the diffs match up with what we have in our 2.12 version. The unit test cases all run.

I updated the comment so it is correct now, or I can remove it completely too if that is preferred. Also, let me know if you want me to squash the commits into one once we agree the review is complete and it is ready to merge. Certainly makes it easier to merge then bunches of edit commits from review comments. In the future I will not do this until it is ready to merge. Sorry again for the confusion.

ojw28 · 2021-11-15T13:40:28Z

Thanks! We'll get this merged.

Also, let me know if you want me to squash the commits into one once we agree the review is complete and it is ready to merge. Certainly makes it easier to merge then bunches of edit commits from review comments.

The tooling & internal review process we use gives us with a flat view anyway, so it doesn't really make any difference to us. The only benefit of squashing is that it makes the commit history look a little neater in the end. On the flip side, anyone wanting to revisit the review of this PR later and understand any of the discussion threads will find it much harder if there's a squash and force push, because some of those threads wont be tied to any accessible source code any more. I think being able to revisit the PR review is probably more important, so overall I think that not squashing is probably preferable for cases like this.

google-cla bot added the cla: yes label Oct 6, 2021

andrewlewis requested a review from ojw28 October 6, 2021 17:15

ojw28 reviewed Oct 14, 2021

View reviewed changes

ojw28 self-assigned this Oct 14, 2021

ojw28 added the pending comments label Oct 14, 2021

stevemayhew force-pushed the p-fix-issue-2882 branch from 06321b4 to d3bba3b Compare October 18, 2021 17:27

stevemayhew requested a review from ojw28 October 20, 2021 20:18

stevemayhew mentioned this pull request Nov 3, 2021

Support renderer mode that only renders the first frame (for trick play) #8592

Open

ojw28 added the should merge label Nov 3, 2021

Correct comment on track selection during seek

530dd3f

The comment "all should be same" is not correct, it is extremely likely they will be the same but not assured. In either case the seek position will work, it just may not land exactly on a sync point in every variant.

ojw28 removed the pending comments label Nov 15, 2021

kim-vde merged commit 4a69e16 into google:dev-v2 Nov 26, 2021

google locked and limited conversation to collaborators Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements SeekParameters.*_SYNC variants for HLS #9536

Implements SeekParameters.*_SYNC variants for HLS #9536

stevemayhew commented Oct 6, 2021

stevemayhew commented Oct 6, 2021

ojw28 Oct 14, 2021

stevemayhew Oct 14, 2021

stevemayhew commented Oct 18, 2021

ojw28 commented Nov 3, 2021

stevemayhew commented Nov 3, 2021

ojw28 commented Nov 3, 2021

ojw28 commented Nov 3, 2021 •

edited

Loading

stevemayhew commented Nov 4, 2021

stevemayhew commented Nov 5, 2021

ojw28 commented Nov 15, 2021 •

edited

Loading

Implements SeekParameters.*_SYNC variants for HLS #9536

Implements SeekParameters.*_SYNC variants for HLS #9536

Conversation

stevemayhew commented Oct 6, 2021

stevemayhew commented Oct 6, 2021

ojw28 Oct 14, 2021

Choose a reason for hiding this comment

stevemayhew Oct 14, 2021

Choose a reason for hiding this comment

stevemayhew commented Oct 18, 2021

ojw28 commented Nov 3, 2021

stevemayhew commented Nov 3, 2021

ojw28 commented Nov 3, 2021

ojw28 commented Nov 3, 2021 • edited Loading

stevemayhew commented Nov 4, 2021

stevemayhew commented Nov 5, 2021

ojw28 commented Nov 15, 2021 • edited Loading

ojw28 commented Nov 3, 2021 •

edited

Loading

ojw28 commented Nov 15, 2021 •

edited

Loading