Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700833002:
Simplify rate limiter logic in Canvas2DLayerBridge (Closed)
|
Created:
3 years, 10 months ago by Justin Novosad Modified:
3 years, 10 months ago Reviewers:
xlai (Olivia) CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, ajuma+watch-canvas_chromium.org, Stephen Chennney, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, Justin Novosad, blink-reviews-platform-graphics_chromium.org, Rik, f(malita), blink-reviews, kinuko+watch, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify rate limiter logic in Canvas2DLayerBridge
This CL is a follow-up to https://codereview.chromium.org/2653933003/.
It refactors the rate-limiter code to make it stop using it own task
observer, in favor of piggy-backing on top of the finalize frame signal
BUG=653548, 673469
Review-Url: https://codereview.chromium.org/2700833002
Cr-Commit-Position: refs/heads/master@{#452071}
Committed: https://chromium.googlesource.com/chromium/src/+/9ac0ae9669662fadaf27f247d0c46d6e14e075fe
Patch Set 1 #Patch Set 2 : add trace #
Total comments: 6
Patch Set 3 : make more readable #
Messages
Total messages: 20 (11 generated)
The CQ bit was checked by junov@chromium.org to run a CQ dry run
junov@chromium.org changed reviewers: + xlai@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2700833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2700833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:144: m_previousFrameWasPresented( Will it be better to rename this as "m_nextFrameNeedsToWait" and initialize it to "false"? Or perhaps other better name. https://codereview.chromium.org/2700833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:895: m_rateLimiter->reset(); Are you sure that this is the only place when the animation is consumed and committed to the compositor? https://codereview.chromium.org/2700833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:1056: m_previousFrameWasPresented = false; If the previous frame was successfully committed (and is true), why calling another finalizeFrame() will make it false? Also the initial call to finalizeFrame() also make it false?
https://codereview.chromium.org/2700833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2700833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:144: m_previousFrameWasPresented( On 2017/02/17 19:39:53, xlai (Olivia) wrote: > Will it be better to rename this as "m_nextFrameNeedsToWait" and initialize it > to "false"? Or perhaps other better name. Acknowledged. https://codereview.chromium.org/2700833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:895: m_rateLimiter->reset(); On 2017/02/17 19:39:53, xlai (Olivia) wrote: > Are you sure that this is the only place when the animation is consumed and > committed to the compositor? For non-offscreen gpu-accelerated 2D canvases, it is. https://codereview.chromium.org/2700833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:1056: m_previousFrameWasPresented = false; On 2017/02/17 19:39:53, xlai (Olivia) wrote: > If the previous frame was successfully committed (and is true), why calling > another finalizeFrame() will make it false? Also the initial call to > finalizeFrame() also make it false? The basic idea is to activate rate limiting when finalize frame gets called more than once between commits. I'm going to rename things and add comments to make this more readable.
Description was changed from ========== Simplify rate limiter logic in Canvas2DLayerBridge This CL is a follow-up to https://codereview.chromium.org/2653933003/. It refactors the rate-limiter code to make it stop using it own task observer, in favor of piggy-backing on top of the finalize frame signal BUG=653548 ========== to ========== Simplify rate limiter logic in Canvas2DLayerBridge This CL is a follow-up to https://codereview.chromium.org/2653933003/. It refactors the rate-limiter code to make it stop using it own task observer, in favor of piggy-backing on top of the finalize frame signal BUG=653548,673469 ==========
New Patch
Looks better. lgtm.
The CQ bit was checked by junov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by junov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487776903382210,
"parent_rev": "0a2373993fd8c9b7eafb1993e905c9b4386aad9a", "commit_rev":
"9ac0ae9669662fadaf27f247d0c46d6e14e075fe"}
Message was sent while issue was closed.
Description was changed from ========== Simplify rate limiter logic in Canvas2DLayerBridge This CL is a follow-up to https://codereview.chromium.org/2653933003/. It refactors the rate-limiter code to make it stop using it own task observer, in favor of piggy-backing on top of the finalize frame signal BUG=653548,673469 ========== to ========== Simplify rate limiter logic in Canvas2DLayerBridge This CL is a follow-up to https://codereview.chromium.org/2653933003/. It refactors the rate-limiter code to make it stop using it own task observer, in favor of piggy-backing on top of the finalize frame signal BUG=653548,673469 Review-Url: https://codereview.chromium.org/2700833002 Cr-Commit-Position: refs/heads/master@{#452071} Committed: https://chromium.googlesource.com/chromium/src/+/9ac0ae9669662fadaf27f247d0c4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9ac0ae9669662fadaf27f247d0c4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
