Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test case for fix for issue #7975 #9174

Closed
wants to merge 2 commits into from

Conversation

stevemayhew
Copy link
Contributor

@stevemayhew stevemayhew commented Jul 9, 2021

First steps for fixing issue #7975 a unit test case[s] that show the bug.

The first commit, b9d959c is a functional test, if the fix goes outside of MaskingMediaSource itself then we'll need this test case

The next commit, ac044ae60e is a unit test case forMaskingMediaSource itself, if the code change to fix involves how the position override works in this class then this test case should be all we need.

Looking at the logic in line 2575

        } else {
          newPeriodUid = periodPosition.first;
          newContentPositionUs = periodPosition.second;
          // Use explicit initial seek as new target live offset.
          setTargetLiveOffset = true;
        }

The initial seek is resolved here, against the PlaylistTimeline this happens when the initial MedaiItem[s] are set so we have no idea even the duration of the window for the live playlist.

Also, I believe using this as the target live offset may be a mistake, the usual use case for setting a position in a live playlist with an initial seek is a SOCU (start-over-catchup) where live is buffered to a rolling buffer, in this case you want to treat it more like VOD (the rolling buffer playlist is typically labeled "EVENT" type until the program ends at which point an end tag is added.

So, there are these basic use cases for live:

  1. No initial seek/position set -- default to the playlist requested live offset (default or EXT-X-START)
  2. Initial position (usually 0) requested -- start a SOCU (here the playlist duration will be the entire event up to present time, the playlist live point is irrelevant)
  3. Saved position ( 0 - duration ) -- return to a saved position in a SOCU playlist. Here the user has navigated away from playback but returns via a "My Shows" list to the in-progress SOCU recording

The most strait forward fix I see is to make the initial preparePositionUs in the MaskingMediaPeriod / MaskingMediaSource have an unset value unless an initial seek or other explicit user action set it. This frees up 0 from serving as the default value as well as a possible user choice.

@tonihei and @marcbaechinger you both have way more invested in this code and no doubt a bunch of refactoring plans I'm unaware of, so I don't want to add more test cases to code you may be refactoring away. As long as the first commit (the functional test) passes with 0 as the seek value the requirements are met and bug #7975 can likely be closed.

The most direct approach simulates what happens in the EPII when the error occurs, that is a the live windowDefaultStartPosition overrides the initial seek request when MaskingMediaSource is in play.

In a subsequent commit, I'll introduce a path to unit test this with tests for `MaskingMediaSource` itself.
@google-cla google-cla bot added the cla: yes label Jul 9, 2021
@stevemayhew stevemayhew marked this pull request as draft July 9, 2021 23:04
@cyberwick011

This comment has been minimized.

Unit test equivalent of the functional test in ExoPlayerTest
depending on the scope of the fix either this test or the ExoPlayerTest
should be kept.

This test shows flaw in the assumption that only a non-zero initial seek value
is a valid override for the window default position.

Value for `startPositionUs` in this chmmit is non-zero, so the test passes.
If the value is 0 it will fail.
@icbaker
Copy link
Collaborator

icbaker commented Apr 15, 2024

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

@icbaker icbaker closed this Apr 15, 2024
@google google locked and limited conversation to collaborators Jun 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants