Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
341213002:
Redraw only dirty area for accelerated 2D Canvas. (Closed)
|
Created:
6 years, 6 months ago by hj.r.chung Modified:
6 years, 6 months ago CC:
blink-layers+watch_chromium.org, blink-reviews, blink-reviews-html_chromium.org, blink-reviews-rendering, Rik, danakj, dglazkov+blink, krit, eae+blinkwatch, jamesr, jbroman, jchaffraix+rendering, leviw+renderwatch, pdr., rwlbuis, rune+blink, Stephen Chennney, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionRedraw only dirty area for accelerated 2D Canvas.
When 2D Canvas got it's own layer, the whole canvas was getting
updated instead of only the dirty rect.
BUG=382958
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176815
Patch Set 1 #
Total comments: 2
Patch Set 2 : New patch with comments applied #
Total comments: 2
Patch Set 3 : New patch with cleanup applied #
Messages
Total messages: 22 (0 generated)
PTAL! I was working on this issue before I spotted the bug, and am uploading just in case you haven't started. If you are already working on the bug please let me know. I'll delete this patch :)
On 2014/06/20 08:10:28, hj.r.chung wrote: > PTAL! > I was working on this issue before I spotted the bug, and am uploading just in > case you haven't started. > If you are already working on the bug please let me know. I'll delete this patch > :) Your time is excellent, I was just about to start working on this today! :-)
https://codereview.chromium.org/341213002/diff/1/Source/core/html/HTMLCanvasE... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/341213002/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:222: m_dirtyRect = rect; Storing the rect for later is a fragile approach because we are assuming that CompositedLayerMapping::contentChanged will be called after this and before the next call to this function. Couldn't you propagate the dirtyRect in CanvasRenderingContext2D::didDraw through the call to renderBox->contentChanged, which calls RenderLayer->contentChanged, which calls CompositedLayerMapping->contentChange?
https://codereview.chromium.org/341213002/diff/1/Source/core/rendering/compos... File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/341213002/diff/1/Source/core/rendering/compos... Source/core/rendering/compositing/CompositedLayerMapping.cpp:1718: return; Rather than using this awkward notification pipeline, can you refactor the code to call a function on CompositedLayerMapping that passes the dirtyRect as an actual argument? Currently, this sends the control flow through a one path and the data through another path. It would be much better to send the control and data through the same path. Also, that would save you from having to store m_dirtyRect on HTMLCanvasElement.
On 2014/06/20 14:06:53, junov wrote: > https://codereview.chromium.org/341213002/diff/1/Source/core/html/HTMLCanvasE... > File Source/core/html/HTMLCanvasElement.cpp (right): > > https://codereview.chromium.org/341213002/diff/1/Source/core/html/HTMLCanvasE... > Source/core/html/HTMLCanvasElement.cpp:222: m_dirtyRect = rect; > Storing the rect for later is a fragile approach because we are assuming that > CompositedLayerMapping::contentChanged will be called after this and before the > next call to this function. > Couldn't you propagate the dirtyRect in CanvasRenderingContext2D::didDraw > through the call to renderBox->contentChanged, which calls > RenderLayer->contentChanged, which calls CompositedLayerMapping->contentChange? Thank you for the input. I'll prepare a new patch with this in regard. I'm currently setting up a build env. at home so it might take some time
In the case where the canvas is accelerated (i.e. is drawing the canvas contents to a dedicated layer) why not send the invalidation directly to the layer in CanvasRenderingContext2D? I.e. instead of relying on the notification to invalidate the layer just call platformLayer()->invalidateRect(). There's no need to bounce all the way out to the HTMLCanvasElement and then through the composited layer mapping to find the layer handling the canvas buffer when the invalidation is starting inside the CanvasRenderingContext2D. The context owns the buffer which actually owns the layer.
PTAL! I took jamesr's approach since it looked reasonable to remove the indirections
On 2014/06/23 06:46:03, hj.r.chung wrote: > PTAL! > I took jamesr's approach since it looked reasonable to remove the indirections lgtm
On 2014/06/23 14:47:29, junov wrote: > On 2014/06/23 06:46:03, hj.r.chung wrote: > > PTAL! > > I took jamesr's approach since it looked reasonable to remove the indirections > > lgtm With this direct access to the WebLayer, it seems we could get rid of some canvas/webgl specific plumbing in RenderBox, RenderBoxModelObject, RenderLayer, GraphicsLayer and CompositedLayerMapping
On 2014/06/23 at 14:59:39, junov wrote: > With this direct access to the WebLayer, it seems we could get rid of some canvas/webgl specific plumbing in RenderBox, RenderBoxModelObject, RenderLayer, GraphicsLayer and CompositedLayerMapping Yes please!
https://codereview.chromium.org/341213002/diff/20001/Source/core/html/canvas/... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/341213002/diff/20001/Source/core/html/canvas/... Source/core/html/canvas/CanvasRenderingContext2D.cpp:1746: if (renderBox && renderBox->hasAcceleratedCompositing()) { I wonder if we can replace this call to hasAcceleratedCompositing with just checking the platformLayer() for null.
On 2014/06/23 17:08:13, abarth wrote: > On 2014/06/23 at 14:59:39, junov wrote: > > With this direct access to the WebLayer, it seems we could get rid of some > canvas/webgl specific plumbing in RenderBox, RenderBoxModelObject, RenderLayer, > GraphicsLayer and CompositedLayerMapping > > Yes please! crbug.com/387766
Please test that this works properly with reflections (I can't figure out how they work today). https://codereview.chromium.org/341213002/diff/20001/Source/core/html/canvas/... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/341213002/diff/20001/Source/core/html/canvas/... Source/core/html/canvas/CanvasRenderingContext2D.cpp:1745: RenderBox* renderBox = canvas()->renderBox(); you aren't using this any more, no need to grab it
PTAL, I've confirmed that reflection is working properly, and cleaned up no longer used code.
On 2014/06/23 17:14:45, junov wrote: > On 2014/06/23 17:08:13, abarth wrote: > > On 2014/06/23 at 14:59:39, junov wrote: > > > With this direct access to the WebLayer, it seems we could get rid of some > > canvas/webgl specific plumbing in RenderBox, RenderBoxModelObject, > RenderLayer, > > GraphicsLayer and CompositedLayerMapping > > > > Yes please! > > crbug.com/387766 oops, I just saw this comment after uploading the new patch
On 2014/06/24 at 02:46:45, heejin.r.chung wrote: > On 2014/06/23 17:14:45, junov wrote: > > On 2014/06/23 17:08:13, abarth wrote: > > > On 2014/06/23 at 14:59:39, junov wrote: > > > > With this direct access to the WebLayer, it seems we could get rid of some > > > canvas/webgl specific plumbing in RenderBox, RenderBoxModelObject, > > RenderLayer, > > > GraphicsLayer and CompositedLayerMapping > > > > > > Yes please! > > > > crbug.com/387766 > > oops, I just saw this comment after uploading the new patch You can save that for a future CL. :)
lgtm
The CQ bit was checked by heejin.r.chung@samsung.com
On 2014/06/24 04:01:05, abarth wrote: > lgtm Thank you all for the reviews!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/heejin.r.chung@samsung.com/341213002/4...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Message was sent while issue was closed.
Change committed as 176815 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
