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

stick to new dash manifest uri after http 302 redirection #7056

Closed
wants to merge 613 commits into from

Conversation

davibe
Copy link
Contributor

@davibe davibe commented Mar 6, 2020

Related to discussion in #6907

AquilesCanta and others added 30 commits January 24, 2020 11:11
PiperOrigin-RevId: 291340508
CeaUtil depends on TrackOutput so should live in the extractors package.

To avoid having Cea708Decoder depend on extractors, this change also
moves the initialization data building/parsing to CodecSpecificDataUtil.

PiperOrigin-RevId: 291348317
It could either live in .util or .extractor, but since it's not needed
outside the extractor package in core (and the FLAC extension), and
FlacStreamMetadataTest uses a FLAC asset, it seems preferable to move it
into the extractor package.

PiperOrigin-RevId: 291372032
PiperOrigin-RevId: 291378636
PiperOrigin-RevId: 291394101
PiperOrigin-RevId: 291397882
PiperOrigin-RevId: 291401328
The tests for the extractor module rely on some core classes.

PiperOrigin-RevId: 291688932
OkHttp 3.12.7 introduced a regression, which was fixed in 3.12.8.

PiperOrigin-RevId: 291695577
There are existing bugs that need this flag to know whether the
current information in the window is still a placeholder or can
already be relied on for further calculation.

This flag will probably only ever be set in DummyTimeline, so it's
not added to the window.set method to avoid updating all clients.

Issue:google#5924
PiperOrigin-RevId: 291705637
We currently only keep the requested next content start position while
we are playing ads. However, we should also keep at least before a content
period is fully prepared to not loose the information about the user intent.

PiperOrigin-RevId: 291705752
Issue: google#6910
PiperOrigin-RevId: 291721229
This is used by broadcast channels to provide interactive contents.
PiperOrigin-RevId: 291750515
This seeker is only seeking to frames that have already been read by the
extractor for playback. Seeking to not-yet-read frames will be
implemented in another change.

Issue: google#6787
PiperOrigin-RevId: 291888899
This also makes them look more like the equivalent tests for
the list / array cases.

PiperOrigin-RevId: 291890150
It doesn't seem worth keeping the cap, since the device
will presumably stop receiving major version updates at
some point anyway.

Issue: google#6899
PiperOrigin-RevId: 291899439
SubtitleView now becomes a ViewGroup that owns a SubtitleTextView. It
handles some common styling defaults, but delegates the underlying
values down into SubtitleTextView through the SubtitleView.Output
interface.

When I add a SubtitleWebView this will also be a ViewGroup containing
a WebView and will implement SubtitleView.Output and convert Cue styling
into HTML & CSS.

PiperOrigin-RevId: 291903294
PiperOrigin-RevId: 291905440
We currently apply new parameter checkpoints from an absolute media
time and then substract the current media start time again to retrieve
the media time offset for this playback parameter checkpoint.

However, the media start time may change when unexpected discontinuities
happen (the start time doesn't actually change, but we change it to
correct for this discontinuity). This then invalidates the absolute
media time in the playback parameter checkpoints (which should have been
corrected as well).

Avoid this problem by also only applying the new start position
from the checkpoint. We don't have to save the start position anymore
because it will cancel itself out.

Also add some documentation and code clarification for improved
readability.

PiperOrigin-RevId: 291923069
We currently have 3 states, but the NOT_SET state isn't needed
anymore. We can therefore replace the IntDef by a simple boolean.

PiperOrigin-RevId: 291926049
PiperOrigin-RevId: 291927263
Calling setViewType with the same view type as the
subtitleView is using would throw InvalidArgumentException
instead of being a noop.

PiperOrigin-RevId: 291937202
Fix outline style subtitle

When a ForegroundColorSpan changes the foreground color, it is also applied
to the outline painter. In order to keep the correct color, one needs to
filter out theses span. We do this with a new cue that is our text source
for the specific edgeLayout.

Take care to only apply background color once

Test: Manual check - Subtitle view can show custom color subtitles from specific Subtitle
Renderer and outline is shown correctly using user defined color.
Without this, tapping the main video playback doesn't bring up the
controls.

PiperOrigin-RevId: 291943063
PiperOrigin-RevId: 292099759
tonihei and others added 13 commits February 28, 2020 18:41
The positions were interchangeably used with window and period
positions. This change more clearly ensures that all positions in the
AdPlaybackState are based on periods and that we use the right adjustments
for all usages.

PiperOrigin-RevId: 297811633
AnalyticsCollector keeps a list of existing MediaPeriodInfo that need
to be updated to new Timelines when they arrive. This already
happens in all cases except that the playingMediaPeriod wasn't updated
when it didn't change during the timeline update.

PiperOrigin-RevId: 297812038
When a new Timeline arrives in the Player, we check whether we can keep
existing MediaPeriods. This check currently involves a condition that
checks if the MediaPeriod is already prepared. The only reason we do
that is to avoid calling MediaPeriod.seekToUs, which is not allowed
on an unprepared MediaPeriod.

It's better to keep the MediaPeriod to prevent restarting the
preparation process. The prepration check can move further down to the
place right before we would call seekToUs.

PiperOrigin-RevId: 297812584
This while loop started with the second period in the queue and the
first one was always ignored.

PiperOrigin-RevId: 297812937
This ensures all player interactions in tests automatically verify that
timestamps calculations are done correctly.

PiperOrigin-RevId: 297813324
This is more accurate since it's just a placeholder and none of the
values is provided by the media.

It also allows to fix a problem in ClippingMediaSource where we
couldn't detect a clipping error because we didn't know if the
timeline is a placeholder or not.

Issue:google#5924
PiperOrigin-RevId: 297813606
We should only ignore seek to the current position if we are
currently READY or BUFFERING. Also, pending initial seek positions
should only be saved while we have an empty timeline, independent of
the player state.

Issue:google#6886
PiperOrigin-RevId: 297813854
The default will soon change to Looper mode PAUSED. Some parts of our code
relies on the legacy behaviour when setting set SystemClock and expecting
pending messages to be delivered. With the new mode, we need to explicitly
request to idle the main looper so that pending messages can be delivered.

PiperOrigin-RevId: 297814964
PiperOrigin-RevId: 297865356
Issue: google#5978
PiperOrigin-RevId: 297876336
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@davibe
Copy link
Contributor Author

davibe commented Mar 6, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Mar 6, 2020
@ojw28 ojw28 self-assigned this Mar 9, 2020
@@ -818,6 +818,12 @@ protected void releaseSourceInternal() {
}
}
}
boolean isRedirect = !loadable.getUri().equals(this.manifestUri);
Copy link
Contributor

@ojw28 ojw28 Mar 9, 2020

Choose a reason for hiding this comment

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

As per my comment in #6907, this change will need updating to let manifest.location take priority. You should also do the isSameUriInstance check as above. So I think you probably want to replace L810 to L826 with something that looks more like (disclaimer: untested):

Uri newManifestUri = manifest.location != null ? manifest.location : loadable.getUri();
synchronized (manifestUriLock) {
  // This condition checks that replaceManifestUri wasn't called between the start and end of
  // this load. If it was, we ignore newManifestUri and prefer the manual replacement.
  @SuppressWarnings("ReferenceEquality")
  boolean isSameUriInstance = loadable.dataSpec.uri == manifestUri;
  if (isSameUriInstance) {
    manifestUri = newManifestUri;
  }
}

@ojw28
Copy link
Contributor

ojw28 commented Mar 9, 2020

As per our contribution guidelines, pull requests should be made to the dev-v2 branch. Please close this one and send a new one to that branch instead. See also the inline comment on the code. Thanks!

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 9, 2020
@davibe davibe closed this Mar 9, 2020
@google google locked and limited conversation to collaborators May 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.