Removes liveOffsetTarget override on seek to live edge #11051

stevemayhew · 2023-03-10T00:06:16Z

Any calls that issue a seekTo() where the position is TIME_UNSET will now restore the live offset target to the MediaItem set default.

The code handles an intra-period seek in the same timeline by simply updating or removing the setTargetLiveOffsetOverrideUs(). Other cases call the original code.

This fixes issue #11050

stevemayhew · 2023-03-10T00:41:09Z

library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java

+ livePlaybackSpeedControl.setTargetLiveOffsetOverrideUs(C.TIME_UNSET);
+ }
+ }
+ }


basically this is an 'inline' of the logic from updatePlaybackSpeedSettingsForNewPeriod when the periodId does not change.

I'm curious for the source update call to updatePlaybackSpeedSettingsForNewPeriod, I'm assuming this is the possible in a DASH playlist to have multiple live periods?

Thanks for the contribution!

updatePlaybackSpeedSettingsForNewPeriod is called each time when the playing period potentially changed. This is not necessarily a multi-period stream. Currently this is called from seekToInternal (after a seek), handleMediaSourceListInfoRefreshed (after the timeline has changed), and maybeUpdateReadingPeriod when the reading period advances to the next period.

From this I rather think we should move this logic into updatePlaybackSpeedSettingsForNewPeriod. So we can call it in any case because we want to potentially reset the playback parameters in the !shouldUseLivePlaybackSpeedControl(...) case.

The only issue with this is that updatePlaybackSpeedSettingsForNewPeriod is potentially called from the other call sites with C.TIME_UNSET. When C.TIME_UNSET is passed updatePlaybackSpeedSettingsForNewPeriod currently only remove the override when the old period is different to the new period (for instance when moving to the next reading period or when the timeline change has removed the playing period).

Concretely, I think your change does actually what updatePlaybackSpeedSettingsForNewPeriod does except that L1945 [1] does only remove the override when the periods are not the same, what is not the case in the seekTo case.

Allow me to suggest the following so you can test whether this is equivalent for you:

add a parameter boolean positionRequestedBySeek to the parameter list of updatePlaybackSpeedSettingsForNewPeriod

All call sites call the method then with false except the call from seekToInternal.

Revert your change in seekToInternal and instead call updatePlaybackSpeedSettingsForNewPeriod with positionRequestedBySeek=true

change ExoPlayerImplInternal.java#L1945 to if (positionRequestedBySeek || !Util.areEqual(oldWindowUid, windowUid))

I think that should boil down to the same, avoid some duplicated code and makes sure that the !shouldUseLivePlaybackSpeedControl(..) case is covered as well.

Can you apply this and probably even more important, test this with your app/stream to see if it is equivalent to your change?

[1] https://github.com/google/ExoPlayer/blob/release-v2/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java#L1945

Thanks for the contribution!
You're welcome

updatePlaybackSpeedSettingsForNewPeriod is called each time when the playing period potentially changed.

Ah, ok as long as this alway the case then yes, it would duplicate less code to keep it all in that method. Guess we need a name like updatePlaybackSpeedSettingsForNewPeriodOrSeek (:-))

From this I rather think we should move this logic into updatePlaybackSpeedSettingsForNewPeriod. So we can call it in any case because we want to potentially reset the playback parameters in the !shouldUseLivePlaybackSpeedControl(...) case.

That sounds good, I'll rework the code to follow this pattern and submit a second commit so we can review the diffs. When it is merge ready I'll squash to a single commit.

The only issue with this is that updatePlaybackSpeedSettingsForNewPeriod is potentially called from the other call sites with C.TIME_UNSET. When C.TIME_UNSET is passed updatePlaybackSpeedSettingsForNewPeriod currently only remove the override when the old period is different to the new period (for instance when moving to the next reading period or when the timeline change has removed the playing period).

Concretely, I think your change does actually what updatePlaybackSpeedSettingsForNewPeriod does except that L1945 [1] does only remove the override when the periods are not the same, what is not the case in the seekTo case.

Correct, the main reason I pull the code out was to make it clear what is going on, and to avoid adding a parameter to
updatePlaybackSpeedSettingsForNewPeriod. If we are good for adding the parameter it will remove the duplicated logic.

Allow me to suggest the following so you can test whether this is equivalent for you:
...
I think that should boil down to the same, avoid some duplicated code and makes sure that the !shouldUseLivePlaybackSpeedControl(..) case is covered as well.

Can you apply this and probably even more important, test this with your app/stream to see if it is equivalent to your change?

Sure, happy to do that. Also, you want me to cherry-pick the commit to androidx.media or does that just make managing the two repositories more of a headache for your team?

SG, thanks! It's fine to keep it here for this PR that is a single file. But you are right, for future PRs we'd prefer androidx.media if that's possible.

marcbaechinger

Thanks Steve! Please see my suggestion in the comment!

marcbaechinger · 2023-03-17T16:10:03Z

library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java

+ livePlaybackSpeedControl.setTargetLiveOffsetOverrideUs(C.TIME_UNSET);
+ }
+ }
+ }


Thanks for the contribution!

updatePlaybackSpeedSettingsForNewPeriod is called each time when the playing period potentially changed. This is not necessarily a multi-period stream. Currently this is called from seekToInternal (after a seek), handleMediaSourceListInfoRefreshed (after the timeline has changed), and maybeUpdateReadingPeriod when the reading period advances to the next period.

From this I rather think we should move this logic into updatePlaybackSpeedSettingsForNewPeriod. So we can call it in any case because we want to potentially reset the playback parameters in the !shouldUseLivePlaybackSpeedControl(...) case.

The only issue with this is that updatePlaybackSpeedSettingsForNewPeriod is potentially called from the other call sites with C.TIME_UNSET. When C.TIME_UNSET is passed updatePlaybackSpeedSettingsForNewPeriod currently only remove the override when the old period is different to the new period (for instance when moving to the next reading period or when the timeline change has removed the playing period).

Concretely, I think your change does actually what updatePlaybackSpeedSettingsForNewPeriod does except that L1945 [1] does only remove the override when the periods are not the same, what is not the case in the seekTo case.

Allow me to suggest the following so you can test whether this is equivalent for you:

add a parameter boolean positionRequestedBySeek to the parameter list of updatePlaybackSpeedSettingsForNewPeriod

All call sites call the method then with false except the call from seekToInternal.

Revert your change in seekToInternal and instead call updatePlaybackSpeedSettingsForNewPeriod with positionRequestedBySeek=true

change ExoPlayerImplInternal.java#L1945 to if (positionRequestedBySeek || !Util.areEqual(oldWindowUid, windowUid))

I think that should boil down to the same, avoid some duplicated code and makes sure that the !shouldUseLivePlaybackSpeedControl(..) case is covered as well.

Can you apply this and probably even more important, test this with your app/stream to see if it is equivalent to your change?

[1] https://github.com/google/ExoPlayer/blob/release-v2/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java#L1945

Any calls that issue a `seekTo()` where the position is `TIME_UNSET` will now restore the live offset target to the MediaItem set default. The change simply passes a flag to `updatePlaybackSpeedSettingsForNewPeriod()` that indicates the call is from a seek operation. If this flag is set, a `positionForTargetOffsetOverrideUs` of `TIME_UNSET` unconditionally removes the `setTargetLiveOffsetOverrideUs()` This fixes issue google#11050

stevemayhew · 2023-03-17T20:35:23Z

Thanks @marcbaechinger. I tested the change and forced pushed the single commit I rebased before updating, so I picked up @tonihei 's fix for #10882. Diff's to HEAD of dev-v2 are strait forward now.

It works just the same (logic is same), but the code is cleaner all going through the same method.

I will back port his changes and this code to our 2.15.1 based release so we are running with the same code.

stevemayhew · 2023-03-17T20:36:27Z

library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java

@@ -1897,7 +1899,8 @@ timeline, rendererPositionUs, getMaxRendererReadPositionUs())) {
 /* oldPeriodId= */ playbackInfo.periodId,
 /* positionForTargetOffsetOverrideUs */ positionUpdate.setTargetLiveOffset
 ? newPositionUs
- : C.TIME_UNSET);
+ : C.TIME_UNSET,
+ /* positionRequestedBySeek */false);
 if (periodPositionChanged


false every place but the seekToInternal()

stevemayhew · 2023-03-17T20:37:52Z

library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java

- // Reset overridden target live offset to media values if window changes.
+ if (!Util.areEqual(oldWindowUid, windowUid) || positionRequestedBySeek) {
+ // Reset overridden target live offset to media values if window changes or if seekTo
+ // default live position.
 livePlaybackSpeedControl.setTargetLiveOffsetOverrideUs(C.TIME_UNSET);


on the else side of if (positionForTargetOffsetOverrideUs != C.TIME_UNSET) so we can unconditionally remove the liveOffsetOverride if this is from a seek.

PiperOrigin-RevId: 518953648 (cherry picked from commit e811749)

PiperOrigin-RevId: 518953648

PiperOrigin-RevId: 518953648 (cherry picked from commit dc3481f)

stevemayhew commented Mar 10, 2023

View reviewed changes

stevemayhew mentioned this pull request Mar 10, 2023

Seek to default live does not restore idealTargetLiveOffset #11050

Closed

1 task

rohitjoins assigned marcbaechinger Mar 10, 2023

marcbaechinger reviewed Mar 17, 2023

View reviewed changes

stevemayhew force-pushed the p-fix-for-issue-11050 branch from f932974 to 31b6166 Compare March 17, 2023 20:19

stevemayhew force-pushed the p-fix-for-issue-11050 branch from 31b6166 to f2cf82c Compare March 17, 2023 20:28

stevemayhew commented Mar 17, 2023

View reviewed changes

tianyif merged commit e811749 into google:dev-v2 Mar 31, 2023

rohitjoins pushed a commit that referenced this pull request Apr 18, 2023

Merge pull request #11051 from TiVo:p-fix-for-issue-11050

4278453

PiperOrigin-RevId: 518953648 (cherry picked from commit e811749)

icbaker pushed a commit that referenced this pull request Apr 27, 2023

Merge pull request #11051 from TiVo:p-fix-for-issue-11050

dc3481f

PiperOrigin-RevId: 518953648

icbaker pushed a commit that referenced this pull request Apr 27, 2023

Merge pull request #11051 from TiVo:p-fix-for-issue-11050

690ac23

PiperOrigin-RevId: 518953648 (cherry picked from commit dc3481f)

google locked and limited conversation to collaborators Jun 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes liveOffsetTarget override on seek to live edge #11051

Removes liveOffsetTarget override on seek to live edge #11051

stevemayhew commented Mar 10, 2023

stevemayhew Mar 10, 2023

marcbaechinger Mar 17, 2023

stevemayhew Mar 17, 2023 •

edited

marcbaechinger Mar 17, 2023

marcbaechinger left a comment

marcbaechinger Mar 17, 2023

stevemayhew commented Mar 17, 2023

stevemayhew Mar 17, 2023

stevemayhew Mar 17, 2023

Removes liveOffsetTarget override on seek to live edge #11051

Removes liveOffsetTarget override on seek to live edge #11051

Conversation

stevemayhew commented Mar 10, 2023

stevemayhew Mar 10, 2023

Choose a reason for hiding this comment

marcbaechinger Mar 17, 2023

Choose a reason for hiding this comment

stevemayhew Mar 17, 2023 • edited

Choose a reason for hiding this comment

marcbaechinger Mar 17, 2023

Choose a reason for hiding this comment

marcbaechinger left a comment

Choose a reason for hiding this comment

marcbaechinger Mar 17, 2023

Choose a reason for hiding this comment

stevemayhew commented Mar 17, 2023

stevemayhew Mar 17, 2023

Choose a reason for hiding this comment

stevemayhew Mar 17, 2023

Choose a reason for hiding this comment

stevemayhew Mar 17, 2023 •

edited