onPollWindow_: use startTime when currentTime is not ready yet (fixes #2848)#2849
Merged
joeyparrish merged 1 commit intoshaka-project:masterfrom Sep 21, 2020
Merged
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
JakubRimal
force-pushed
the
bug-2848-availability-window
branch
from
d9fc7d7 to
ebdea3e
Compare
Contributor
Author
|
@googlebot I fixed it. |
Member
|
@JakubRimal, thanks! Is there a particular issue associated with this fix? Do all the tests still pass? |
Member
|
Doh, it's there in the title. I overlooked it! |
Contributor
Author
|
Member
|
Those test failures are concerning because we don't see the same failures. I would be interested to find out what is different about your environment so that we could improve our tests. Can you please file a new issue about that so that we can discuss? My only concern with your PR is that I would prefer to have a regression test or conformance rule (see |
Collaborator
|
All tests passed! |
joeyparrish
approved these changes
Sep 21, 2020
Member
joeyparrish
left a comment
joeyparrish
left a comment
There was a problem hiding this comment.
I will go ahead and submit this, then follow up with a regression test myself. I think it can be done fairly easily in the Playhead unit tests. Thanks!
Contributor
Author
Yes, please see #2865
Thank you very much, I was going to write you that I am afraid that I am not familiar with tests enough to be able to write it for this scenario so I am happy to hear that you'll do it and that it could be "fairly easily". 🙂 |
shaka-bot
pushed a commit
that referenced
this pull request
Sep 22, 2020
PR #2849 landed without a regression test. The test is a bit tricky, since it requires us to simulate a very odd set of circumstances which nonetheless occur on smart TVs. The video must be: - in a HAVE_METADATA state - unpaused - with currentTime set to 0 - with the initial seek ignored by the platform Furthermore, we want to suppress the seek retry behavior of the VideoWrapper class, since that makes it more difficult to detect the seek-range-related seeks in the original bug. This involves access to private members, which means we need to suppress access controls in the compiler during part of the test. Issue #2848 Issue #2748 PR #2849 Change-Id: I0427d4b6b4161cd2a147a5c297dfaf9b424c92b7
Member
|
The regression test was added in d20b9f0 if you are interested to see how it works. "Easy" is relative, I suppose. It required us to emulate a very specific scenario and then suppress some other internal mechanisms to more easily measure the effect of the bug. |
matEhickey
pushed a commit
to matEhickey/shaka-player
that referenced
this pull request
Sep 25, 2020
…ject#2849) Up until we load some content and seek to the start time, we should avoid applying the seek range. Otherwise, for a slow-loading live stream, we could end up seeking to the beginning of the seek range while waiting to begin playback at the end of the range. This doesn't seem to happen on desktops, but on some smart TVs, setting video.currentTime will not be immediately reflected, leaving a race condition that only VideoWrapper will properly resolve. Closes shaka-project#2848 Closes shaka-project#2748
matEhickey
pushed a commit
to matEhickey/shaka-player
that referenced
this pull request
Sep 25, 2020
PR shaka-project#2849 landed without a regression test. The test is a bit tricky, since it requires us to simulate a very odd set of circumstances which nonetheless occur on smart TVs. The video must be: - in a HAVE_METADATA state - unpaused - with currentTime set to 0 - with the initial seek ignored by the platform Furthermore, we want to suppress the seek retry behavior of the VideoWrapper class, since that makes it more difficult to detect the seek-range-related seeks in the original bug. This involves access to private members, which means we need to suppress access controls in the compiler during part of the test. Issue shaka-project#2848 Issue shaka-project#2748 PR shaka-project#2849 Change-Id: I0427d4b6b4161cd2a147a5c297dfaf9b424c92b7
matEhickey
pushed a commit
to matEhickey/shaka-player
that referenced
this pull request
Sep 25, 2020
…ject#2849) Up until we load some content and seek to the start time, we should avoid applying the seek range. Otherwise, for a slow-loading live stream, we could end up seeking to the beginning of the seek range while waiting to begin playback at the end of the range. This doesn't seem to happen on desktops, but on some smart TVs, setting video.currentTime will not be immediately reflected, leaving a race condition that only VideoWrapper will properly resolve. Closes shaka-project#2848 Closes shaka-project#2748
matEhickey
pushed a commit
to matEhickey/shaka-player
that referenced
this pull request
Sep 25, 2020
PR shaka-project#2849 landed without a regression test. The test is a bit tricky, since it requires us to simulate a very odd set of circumstances which nonetheless occur on smart TVs. The video must be: - in a HAVE_METADATA state - unpaused - with currentTime set to 0 - with the initial seek ignored by the platform Furthermore, we want to suppress the seek retry behavior of the VideoWrapper class, since that makes it more difficult to detect the seek-range-related seeks in the original bug. This involves access to private members, which means we need to suppress access controls in the compiler during part of the test. Issue shaka-project#2848 Issue shaka-project#2748 PR shaka-project#2849 Change-Id: I0427d4b6b4161cd2a147a5c297dfaf9b424c92b7
joeyparrish
pushed a commit
that referenced
this pull request
Sep 29, 2020
Up until we load some content and seek to the start time, we should avoid applying the seek range. Otherwise, for a slow-loading live stream, we could end up seeking to the beginning of the seek range while waiting to begin playback at the end of the range. This doesn't seem to happen on desktops, but on some smart TVs, setting video.currentTime will not be immediately reflected, leaving a race condition that only VideoWrapper will properly resolve. Closes #2848 Closes #2748
joeyparrish
added a commit
that referenced
this pull request
Sep 29, 2020
PR #2849 landed without a regression test. The test is a bit tricky, since it requires us to simulate a very odd set of circumstances which nonetheless occur on smart TVs. The video must be: - in a HAVE_METADATA state - unpaused - with currentTime set to 0 - with the initial seek ignored by the platform Furthermore, we want to suppress the seek retry behavior of the VideoWrapper class, since that makes it more difficult to detect the seek-range-related seeks in the original bug. This involves access to private members, which means we need to suppress access controls in the compiler during part of the test. Issue #2848 Issue #2748 PR #2849 Change-Id: I0427d4b6b4161cd2a147a5c297dfaf9b424c92b7
joeyparrish
pushed a commit
that referenced
this pull request
Oct 1, 2020
Up until we load some content and seek to the start time, we should avoid applying the seek range. Otherwise, for a slow-loading live stream, we could end up seeking to the beginning of the seek range while waiting to begin playback at the end of the range. This doesn't seem to happen on desktops, but on some smart TVs, setting video.currentTime will not be immediately reflected, leaving a race condition that only VideoWrapper will properly resolve. Closes #2848 Closes #2748 Backported to v2.5.x Change-Id: Id2259278ba99dfd99b17a93a94504ed0a1400238
joeyparrish
added a commit
that referenced
this pull request
Oct 1, 2020
PR #2849 landed without a regression test. The test is a bit tricky, since it requires us to simulate a very odd set of circumstances which nonetheless occur on smart TVs. The video must be: - in a HAVE_METADATA state - unpaused - with currentTime set to 0 - with the initial seek ignored by the platform Furthermore, we want to suppress the seek retry behavior of the VideoWrapper class, since that makes it more difficult to detect the seek-range-related seeks in the original bug. This involves access to private members, which means we need to suppress access controls in the compiler during part of the test. Issue #2848 Issue #2748 PR #2849 Backported to v2.5.x Change-Id: I0427d4b6b4161cd2a147a5c297dfaf9b424c92b7
github-actions
bot
added
the
status: archived
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There is a race condition: it could happen that
onPollWindow_checks falling outside the availability window beforevideoElementreturns thecurrentTimecorrectly (it returns 0 instead) so the following calculation leads to incorrect jump forward. Now it will usethis.videoWrapper_.getTime()which returnsstartTimein that case instead.