Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2687073003:
Make disabling 2d canvas GPU acceleration for getImageData less aggressive. (Closed)
|
Created:
3 years, 10 months ago by xiangze.zhang Modified:
3 years, 10 months ago Reviewers:
Justin Novosad CC:
chromium-reviews, krit, dshwang, drott+blinkwatch_chromium.org, ajuma+watch-canvas_chromium.org, Stephen Chennney, blink-reviews-html_chromium.org, pdr+graphicswatchlist_chromium.org, jbroman, blink-reviews-platform-graphics_chromium.org, dglazkov+blink, Rik, f(malita), blink-reviews, kinuko+watch, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis, Jin Yang Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake disabling 2d canvas GPU acceleration for getImageData less aggressive.
Make canvas switch out of gpu acceleration only when getImageData is
called in successive frames. This will avoid disabling gpu acceleration
when getImageData is only called once or is called very rarely.
BUG=690122
Review-Url: https://codereview.chromium.org/2687073003
Cr-Commit-Position: refs/heads/master@{#451998}
Committed: https://chromium.googlesource.com/chromium/src/+/c36340833135933c5934ece592fb41b82743e2d8
Patch Set 1 #Patch Set 2 : Make the solution more general #Patch Set 3 : Add test #
Total comments: 6
Patch Set 4 : Address comments #
Messages
Total messages: 27 (14 generated)
Description was changed from ========== Make disabling 2d canvas GPU acceleration for getImageData less aggressive. Make canvas switch out of gpu acceleration only when getImageData is called in successive frames. This will avoid disabling gpu acceleration when getImageData is only called once or is called very rarely. BUG= ========== to ========== Make disabling 2d canvas GPU acceleration for getImageData less aggressive. Make canvas switch out of gpu acceleration only when getImageData is called in successive frames. This will avoid disabling gpu acceleration when getImageData is only called once or is called very rarely. BUG= ==========
xiangze.zhang@intel.com changed reviewers: + junov@chromium.org
PTAL
On 2017/02/10 07:49:25, xiangze.zhang wrote: > PTAL I agree with what you are doing here, but I would like the solution to be a bit more general. Instead of counting consecutive frames with calls to getImageData, we should be counting consecutive frames with gpu readbacks. gpu readbacks may be the result of calling getImageData, and also HTMLCanvasElement.toBlob and HTMLCanvasElement.toDataURL Also, you need a test for this new behavior. There is already a test in CanvasRenderingContext2DTest.cpp named GetImageDataDisablesAcceleration. I suggest that you modify that test to make it trigger your new logic. To run the CanvasRenderingContext2DTest unit tests locally you need to do this: ninja -C out/<...> blink_tests out/<...>/webkit_unit_tests --gtest_filter="CanvasRenderingContext2DTest*"
Description was changed from ========== Make disabling 2d canvas GPU acceleration for getImageData less aggressive. Make canvas switch out of gpu acceleration only when getImageData is called in successive frames. This will avoid disabling gpu acceleration when getImageData is only called once or is called very rarely. BUG= ========== to ========== Make disabling 2d canvas GPU acceleration for getImageData less aggressive. Make canvas switch out of gpu acceleration only when getImageData is called in successive frames. This will avoid disabling gpu acceleration when getImageData is only called once or is called very rarely. BUG=690122 ==========
On 2017/02/15 20:23:44, Justin Novosad wrote: > On 2017/02/10 07:49:25, xiangze.zhang wrote: > > PTAL > > I agree with what you are doing here, but I would like the solution to be a bit > more general. Instead of counting consecutive frames with calls to getImageData, > we should be counting consecutive frames with gpu readbacks. gpu readbacks may > be the result of calling getImageData, and also HTMLCanvasElement.toBlob and > HTMLCanvasElement.toDataURL There are valid use cases where CanvasRenderingContext2D.getImageData is used among other canvas2d APIs in the requestAnimationFrame loop. However HTMLCanvasElement.toBlob/toDataURL are used to output canvas data to other format. I don't know if there are cases that HTMLCanvasElement.toBlob/toDataURL are called in every frame. I have made the code more general and made the switching of surface happen during finalizeFrame(), so it'll be easy to add other cases. Maybe we should consider HTMLCanvasElement.toBlob/toDataURL later when it is needed? > Also, you need a test for this new behavior. There is already a test in > CanvasRenderingContext2DTest.cpp named GetImageDataDisablesAcceleration. I > suggest that you modify that test to make it trigger your new logic. > To run the CanvasRenderingContext2DTest unit tests locally you need to do this: > > ninja -C out/<...> blink_tests > out/<...>/webkit_unit_tests --gtest_filter="CanvasRenderingContext2DTest*" I have modified the test GetImageDataDisablesAcceleration to verify the new logic.
The CQ bit was checked by junov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This is great. I just have a few minor corrections to suggest. FYI: An example of a use cases where toDataURL is called on each frame is applications that stream canvas-rendered animations over the network. https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h (right): https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:81: GPUReadbackMinSuccessiveFrames = 3, This parameter is non-trivial to understand, please add a brief comment to explain the logic that it controls. https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:102: m_gpuReadbackInvoked(false), For clarity, let's call this m_gpuReadbackInvokedInCurrentFrame https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageBuffer.h (right): https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageBuffer.h:200: mutable int m_gpuReadbackSuccessiveFrames; This one does not need to be mutable
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h (right): https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:81: GPUReadbackMinSuccessiveFrames = 3, On 2017/02/16 15:27:20, Justin Novosad wrote: > This parameter is non-trivial to understand, please add a brief comment to > explain the logic that it controls. Done. https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:102: m_gpuReadbackInvoked(false), On 2017/02/16 15:27:20, Justin Novosad wrote: > For clarity, let's call this m_gpuReadbackInvokedInCurrentFrame Done. https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageBuffer.h (right): https://codereview.chromium.org/2687073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageBuffer.h:200: mutable int m_gpuReadbackSuccessiveFrames; On 2017/02/16 15:27:20, Justin Novosad wrote: > This one does not need to be mutable Done.
lgtm
The CQ bit was checked by xiangze.zhang@intel.com
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) 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_chromeos_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_rel_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 xiangze.zhang@intel.com
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiangze.zhang@intel.com
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": 60001, "attempt_start_ts": 1487760194380060,
"parent_rev": "fa5fec25bef17c2b0855201a3b0e1dfa848eb50e", "commit_rev":
"c36340833135933c5934ece592fb41b82743e2d8"}
Message was sent while issue was closed.
Description was changed from ========== Make disabling 2d canvas GPU acceleration for getImageData less aggressive. Make canvas switch out of gpu acceleration only when getImageData is called in successive frames. This will avoid disabling gpu acceleration when getImageData is only called once or is called very rarely. BUG=690122 ========== to ========== Make disabling 2d canvas GPU acceleration for getImageData less aggressive. Make canvas switch out of gpu acceleration only when getImageData is called in successive frames. This will avoid disabling gpu acceleration when getImageData is only called once or is called very rarely. BUG=690122 Review-Url: https://codereview.chromium.org/2687073003 Cr-Commit-Position: refs/heads/master@{#451998} Committed: https://chromium.googlesource.com/chromium/src/+/c36340833135933c5934ece592fb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c36340833135933c5934ece592fb...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2714103002/ by kbr@chromium.org. The reason for reverting is: Caused http://crbug.com/695471 -- regressions of 2D/WebGL canvas test on Linux AMD. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
