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

Removes liveOffsetTarget override on seek to live edge #11051

Merged
merged 1 commit into from Mar 31, 2023

Conversation

stevemayhew
Copy link
Contributor

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

livePlaybackSpeedControl.setTargetLiveOffsetOverrideUs(C.TIME_UNSET);
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@marcbaechinger marcbaechinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Steve! Please see my suggestion in the comment!

livePlaybackSpeedControl.setTargetLiveOffsetOverrideUs(C.TIME_UNSET);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false every place but the seekToInternal()

// 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tianyif tianyif merged commit e811749 into google:dev-v2 Mar 31, 2023
1 check passed
rohitjoins pushed a commit that referenced this pull request Apr 18, 2023
PiperOrigin-RevId: 518953648
(cherry picked from commit e811749)
icbaker pushed a commit that referenced this pull request Apr 27, 2023
icbaker pushed a commit that referenced this pull request Apr 27, 2023
PiperOrigin-RevId: 518953648
(cherry picked from commit dc3481f)
@google google locked and limited conversation to collaborators Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants