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

Revert "Give blink::CueTimeline a timer for precisely handling cue events" #25170

Merged
merged 1 commit into from Aug 21, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

This reverts commit cb9cdc6693ba2ac64292494cca7ad28a6a00f084.

Reason for revert: This CL is likely the cause of build failure for Builder Linux Tests (dbg)(1) since https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests%20%28dbg%29%281%29/91043. Revert to see if that fix the build

Original change's description:

Give blink::CueTimeline a timer for precisely handling cue events

Cue-related events (enter, exit, cuechange) are fired up to 250ms late,
and there is a similar latency in their display which is quite
noticeable.

The underlying problem is that we only update cues alongside the
timeupdate event, which is fixed at 4 Hz.

My solution is to add a separate timer to CueTimeline
(CueTimeline::cue_event_timer_) which is given a timeout for the
next cue event based on the current playback position and rate. This
allows for significantly more precise cue timing accuracy without a
significant performance penalty.

Additionally, several web tests were written with the expectation that
the 'time marches on' algorithm is run before video playback begins
(ie, upon loading text track cues). This behavior is not in accordance
with the spec (as outlined in crbug/1050767), so this CL fixes those
expectations and adds a new test to prevent regressing.

Bug: 576310, 1050767
Change-Id: I675f5f030a68ba9cee10e12b3e79a9b174048193
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2008079
Commit-Queue: Will Cassella <cassew@google.com>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#800148}

TBR=fs@opera.com,alancutter@chromium.org,mlamouri@chromium.org,cassew@google.com

Change-Id: I0563b173344cef976c16b4f2851b45762a67843f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 576310
Bug: 1050767
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368614
Reviewed-by: Maggie Cai <mxcai@chromium.org>
Commit-Queue: Maggie Cai <mxcai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800432}

…ents"

This reverts commit cb9cdc6693ba2ac64292494cca7ad28a6a00f084.

Reason for revert: This CL is likely the cause of build failure for Builder Linux Tests (dbg)(1) since https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests%20%28dbg%29%281%29/91043. Revert to see if that fix the build

Original change's description:
> Give blink::CueTimeline a timer for precisely handling cue events
>
> Cue-related events (enter, exit, cuechange) are fired up to 250ms late,
> and there is a similar latency in their display which is quite
> noticeable.
>
> The underlying problem is that we only update cues alongside the
> `timeupdate` event, which is fixed at 4 Hz.
>
> My solution is to add a separate timer to CueTimeline
> (`CueTimeline::cue_event_timer_`) which is given a timeout for the
> next cue event based on the current playback position and rate. This
> allows for significantly more precise cue timing accuracy without a
> significant performance penalty.
>
> Additionally, several web tests were written with the expectation that
> the 'time marches on' algorithm is run before video playback begins
> (ie, upon loading text track cues). This behavior is not in accordance
> with the spec (as outlined in crbug/1050767), so this CL fixes those
> expectations and adds a new test to prevent regressing.
>
> Bug: 576310, 1050767
> Change-Id: I675f5f030a68ba9cee10e12b3e79a9b174048193
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2008079
> Commit-Queue: Will Cassella <cassew@google.com>
> Reviewed-by: Fredrik Söderquist <fs@opera.com>
> Cr-Commit-Position: refs/heads/master@{#800148}

TBR=fs@opera.com,alancutter@chromium.org,mlamouri@chromium.org,cassew@google.com

Change-Id: I0563b173344cef976c16b4f2851b45762a67843f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 576310
Bug: 1050767
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368614
Reviewed-by: Maggie Cai <mxcai@chromium.org>
Commit-Queue: Maggie Cai <mxcai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800432}
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

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

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 5f8ee81 into master Aug 21, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-d0f0696d13 branch August 21, 2020 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants