Skip to content

Commit

Permalink
Merge pull request #11051 from TiVo:p-fix-for-issue-11050
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 518953648
  • Loading branch information
@tianyif
tianyif committed Mar 30, 2023
2 parents 1d0230a + f2cf82c commit e811749
Showing 1 changed file with 11 additions and 6 deletions.
17 changes: 11 additions & 6 deletions library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1248,7 +1248,8 @@ private void seekToInternal(SeekPosition seekPosition) throws ExoPlaybackExcepti
/* newPeriodId= */ periodId,
/* oldTimeline= */ playbackInfo.timeline,
/* oldPeriodId= */ playbackInfo.periodId,
/* positionForTargetOffsetOverrideUs= */ requestedContentPositionUs);
/* positionForTargetOffsetOverrideUs= */ requestedContentPositionUs,
/* forceSetTargetOffsetOverride= */ true);
}
} finally {
playbackInfo =
Expand Down Expand Up @@ -1898,7 +1899,8 @@ timeline, rendererPositionUs, getMaxRendererReadPositionUs())) {
/* oldPeriodId= */ playbackInfo.periodId,
/* positionForTargetOffsetOverrideUs */ positionUpdate.setTargetLiveOffset
? newPositionUs
: C.TIME_UNSET);
: C.TIME_UNSET,
/* forceSetTargetOffsetOverride= */ false);
if (periodPositionChanged
|| newRequestedContentPositionUs != playbackInfo.requestedContentPositionUs) {
Object oldPeriodUid = playbackInfo.periodId.periodUid;
Expand Down Expand Up @@ -1936,7 +1938,8 @@ private void updatePlaybackSpeedSettingsForNewPeriod(
MediaPeriodId newPeriodId,
Timeline oldTimeline,
MediaPeriodId oldPeriodId,
long positionForTargetOffsetOverrideUs)
long positionForTargetOffsetOverrideUs,
boolean forceSetTargetOffsetOverride)
throws ExoPlaybackException {
if (!shouldUseLivePlaybackSpeedControl(newTimeline, newPeriodId)) {
// Live playback speed control is unused for the current period, reset speed to user-defined
Expand Down Expand Up @@ -1966,8 +1969,9 @@ private void updatePlaybackSpeedSettingsForNewPeriod(
int oldWindowIndex = oldTimeline.getPeriodByUid(oldPeriodId.periodUid, period).windowIndex;
oldWindowUid = oldTimeline.getWindow(oldWindowIndex, window).uid;
}
if (!Util.areEqual(oldWindowUid, windowUid)) {
// Reset overridden target live offset to media values if window changes.
if (!Util.areEqual(oldWindowUid, windowUid) || forceSetTargetOffsetOverride) {
// Reset overridden target live offset to media values if window changes or if seekTo
// default live position.
livePlaybackSpeedControl.setTargetLiveOffsetOverrideUs(C.TIME_UNSET);
}
}
Expand Down Expand Up @@ -2090,7 +2094,8 @@ private void maybeUpdateReadingPeriod() throws ExoPlaybackException {
/* newPeriodId= */ readingPeriodHolder.info.id,
/* oldTimeline= */ playbackInfo.timeline,
/* oldPeriodId= */ oldReadingPeriodHolder.info.id,
/* positionForTargetOffsetOverrideUs= */ C.TIME_UNSET);
/* positionForTargetOffsetOverrideUs= */ C.TIME_UNSET,
/* forceSetTargetOffsetOverride= */ false);

if (readingPeriodHolder.prepared
&& readingPeriodHolder.mediaPeriod.readDiscontinuity() != C.TIME_UNSET) {
Expand Down

11 comments on commit e811749

@stevemayhew
Copy link
Contributor

Choose a reason for hiding this comment

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

@tianyif I'm a bit confused by this "merge" as it looks like it changed commit f2cf82c
This makes it difficult for us to mange our change in our git when it comes back to us from upstream merges.

If you could explain this, as it is different from what we tested.

@tianyif
Copy link
Contributor Author

@tianyif tianyif commented on e811749 Apr 4, 2023 •
edited

Choose a reason for hiding this comment

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

@stevemayhew, the additional changes regarding the corresponding pull request could be made based on our internal code review.

@icbaker
Copy link
Collaborator

@icbaker icbaker commented on e811749 Apr 5, 2023

Choose a reason for hiding this comment

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

@marcbaechinger merged this internally and @tonihei reviewed it, so they're probably best placed to comment on what changed (@tianyif only pushed it externally, which is why it looks like they 'committed' it to this repo).

@stevemayhew Unfortunately when changes are made to a PR during internal review, the resulting git history contains an 'evil merge' containing those changes (rather than making the changes in discrete commits). This is a result of the way Google's internal VCS and git (and GitHub) interact when handling PRs specifically.

@tonihei
Copy link
Collaborator

@tonihei tonihei commented on e811749 Apr 5, 2023

Choose a reason for hiding this comment

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

If I remember correctly, we only changed the parameter name from positionRequestedBySeek to forceSetTargetOffsetOverride (because it's better to name parameters for what they do not for when they are set).

But, yeah, as @icbaker already explained, these review changes are often unavoidable. There are also automatic code formatting adjustments. So even without functional differences, there might be a small diff to the original PR.

@stevemayhew
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, we only changed the parameter name from positionRequestedBySeek to forceSetTargetOffsetOverride (because it's better to name parameters for what they do not for when they are set).

That’s perfectly fine, this is what a pull request review is for, and the proper place to have the discussion.
Our internal pull requests reviews always create a series of commits with the review updates, the end result is
then squashed to a single commit then "merged" with a rebase into our mainline.
This makes the change easy to track and reduces the rats nest of vacuous branches.

But, yeah, as @icbaker already explained, these review changes are often unavoidable...

Sure, we support and recognize that review changes are unavoidable, that is the whole point of the
pull request. It's not the changes we take issue with, but how and where they are made must preserve
the integrity of the original source commit.

@icbaker Yes, the "evil merge" is the main issue here (though Evil is a strong word, I'm sure @tianyif had no malicious intent ;-) ).
We track our changes coming back from upstream by matching the upstream commit subject with our own commits, this is non trivial for us to maintain. This simple process becomes impossible when the committed code is scattered across multiple git patches and merge commits.

While our installed base may be small compared to Google, we do bring over a million IPTV users, across multiple
Google certified Android streamer devices, unmanaged Android'ish devices, and Android mobile devices
all consuming live and start over/catchup content served by a broad range of CDN/origin/Transcode vendors
as well as third party content providers.

We are currently committed to sharing our changes and bug fixes to the Google public open source including
whatever it takes to keep this a community effort. In my opinion the best place to continue this discussion and
come to consensus on best practices (including, for example auto formatters and an Android Studio code style template) is the CONTRIBUTING guidelines.

I would be more then happy to help with this effort, reviewing changes or suggesting changes to this document.

@tonihei
Copy link
Collaborator

@tonihei tonihei commented on e811749 Apr 6, 2023

Choose a reason for hiding this comment

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

First of all, we really appreciate pull requests and upstreaming changes. Because, as you say, the library overall becomes better if it's a community effort and we definitely want to get further PRs in the future!

Having said that, we also discussed the tooling problem today and ways we can avoid the "evil merge" (this is apparently a term of art and doesn't necessarily imply evil intentions ;) ).

For a bit more background, our current process and tooling works like this:

  • We use an internal version of Copybara to automatically import PRs into a proposed change to Google's mono-repo.
  • The internal repo has no branches and every single change is directly committed to the single main branch.
  • The proposed change must be reviewed by another team member before being submitted to the main code branch. Any further comment from this internal review is directly addressed in the internal pending change proposal. The review and the submission also enforces automatic code formatting.
  • Once the internal change is submitted (as a single commit, comprised of all PR commits and any further internal changes), the next push back to GitHub via Copybara will publish this as the "merge" commit you've seen. The PR itself will also be marked as merged by Copybara.

Our current proposal is roughly:

  • We still import the original PR as a proposed change to the internal codebase. We also still assign an internal reviewer as required by the internal guidelines for code changes.
  • NEW: We will then address any review comment, potential follow-up changes or formatting adjustments in a slightly more complicated manner:
    • The suggested change is committed as a git commit to the PR branch.
    • The Copybara import is run again (replacing the code in the pending internal change).
  • This cycle can be repeated multiple times so that the final internal proposed change looks exactly like the PR on GitHub.
  • Submitting this change and reupload as a merge commit will work as before. The end result should be regular git merge without any surprising changes.

Does that sound sensible to you as well? It generates a bit more work on our side, but would align better with established git practices. We are also not entirely sure if it works exactly as we think it should work, but we are happy to try this process for the next PR.

@stevemayhew
Copy link
Contributor

Choose a reason for hiding this comment

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

@tonihei thanks, yes that does sound like that could work better. It does sound like a lot of work, both workflows. We appreciate it!

We all see the advantages of something like mono-repo (Mainly cross-team collaboration and visibility). From the POV of the ExoPlayer open-source community, the mono-repo is https://github.com/google/ExoPlayer
I'm sure we can come up with a workflow that honors the requirements of both sides.

Two things we would like from your team:

  1. Coding standard formatting guidelines (Preferably published as an Android Studio coding-style, I'd put it in the CONTRIBUTING notes).
  2. Preserve the original commit accepted and smashed from the reviewed PR (title, text, etc)

Even if you make changes post PR review and merge as a separate commit that is acceptable, as long as the original commit is preserved. Would be nice to reference the original commit in these internal review changes so we can see the history of what happened.

The first would help the community submit better code (I tell my team Google owns the style, follow what you see in the code... But obviously far better if this is mechanically done for us by the IDE). The second allows us to find our original change, reducing merge conflicts when we incorporate upstream releases. This helps us stay on the leading edge of ExoPlayer releases (a good thing for all of us!)

Thanks again!

@icbaker
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Coding standard formatting guidelines (Preferably published as an Android Studio coding-style, I'd put it in the CONTRIBUTING notes).

I've updated Media3'sCONTRIBUTING.md with a link to the Google Java Style Guide (which covers a mixture of 'automatable' (e.g. formatting) and 'human enforced' rules (e.g. identifier naming)), and google-java-format (which has instructions about various IDE integrations): androidx/media@f799766

2. Preserve the original commit accepted and smashed from the reviewed PR (title, text, etc)

I'm experimenting with the flow proposed by @tonihei above on androidx/media#313 - when that gets merged we can take a look at the resulting commit and see how much less 'evil' we've managed to make it :)

@icbaker
Copy link
Collaborator

Choose a reason for hiding this comment

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

@stevemayhew Can you take a look at this merge commit and see if it looks better? In this case the fixes I made during review were added as additional commits on the PR, so hopefully the resulting merge commit is much cleaner: androidx/media@fab134f

@stevemayhew
Copy link
Contributor

Choose a reason for hiding this comment

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

@icbaker Yes, tracing the path is easier when the pull request is cleanly just a simple merge. Thanks for you diligence on this!

I'm curious how you are able to push commits (the reformat) to the forked repo (https://github.com/pengbins/media/tree/fix_ts_h265reader_parse_sps). It's great, as one of the most frustrating things about reviewing external pull requests is you can't simply checkout the branch and make small changes. I suppose this could be done by cherry-picking the changes across identical but disconnected branches, but still that is messy.

Internally for us, a pull request is a code review essentially. When pull requests are branches with small changes (e.g. easy to squash, and use rebase to keep up to date with the mainline) we simply squash the pull request branch, rebase the commit on to mainline. This has several advantages:

  1. mainline history is not simply an endless stream of pull request merges
  2. the final "merged" commit on mainline is a well thought out description of the change
  3. mainline history maintains the text and author of the original change.

At any rate, not having code changes in a merge commit is a major advantage. When cherry-picking changes one must ignore merges, having actual patches as part of the merge makes this impossible.

Also, thanks for publishing the coding style, I've updated my Android Studio and instructed my team members to as well.

@icbaker
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious how you are able to push commits (the reformat) to the forked repo (https://github.com/pengbins/media/tree/fix_ts_h265reader_parse_sps). It's great, as one of the most frustrating things about reviewing external pull requests is you can't simply checkout the branch and make small changes. I suppose this could be done by cherry-picking the changes across identical but disconnected branches, but still that is messy.

I used the GiHub CLI to get a local copy of the PR branch in my local clone of the androidx/media repo:

$ gh pr checkout 313

That created a branch called fix_ts_h265reader_parse_sps and when I committed to it and git push'd the commits ended up going into the forked repo. I didn't do anything particularly special, it 'just worked' (I wasn't sure if it was going to, but thought it was worth a try). I could then git pull follow-up changes by the PR author (sometimes needing to 'force pull' with git reset --hard FETCH_HEAD).

Please sign in to comment.