Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
731933004:
Get rid of LayerPainter::paintTransformedLayerIntoFragments(). (Closed)
|
Created:
6 years, 1 month ago by mstensho (USE GERRIT) Modified:
6 years, 1 month ago Reviewers:
chrishtr CC:
blink-reviews, blink-reviews-paint_chromium.org, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionGet rid of LayerPainter::paintTransformedLayerIntoFragments().
Now we have a more unified code path for multicol and non-multicol.
Note that we no longer clip against the "paint dirty rect" here.
That was wrong anyway, since the "paint dirty rect" is in physical
pixels, while what it was clipped against (parent clip rectangle) was
not transformed. Instead of adding code that transforms or
inverse-transforms one of the rectangles, we can just as well trust
paintFragmentWithPhase() to do proper clipping when we finally get there.
We also make an effort not to meddle with layers that are not within
paintingInfo.rootLayer, since that's going to produce incorrect results
anyway.
R=chrishtr@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185777
Patch Set 1 #
Total comments: 2
Patch Set 2 : code review #
Total comments: 11
Patch Set 3 : rebase master #Patch Set 4 : If there's no parent layer to worry about, we can skip some work. Also added a FIXME. #Patch Set 5 : code review. #
Total comments: 6
Patch Set 6 : Code review. #Patch Set 7 : Add FIXME for fragmentOffset parameter to ClipRecorder(). #
Messages
Total messages: 26 (7 generated)
https://codereview.chromium.org/731933004/diff/1/Source/core/paint/LayerPaint... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/1/Source/core/paint/LayerPaint... Source/core/paint/LayerPainter.cpp:94: LayerFragments fragments; Factor all this code into a new method paintFragmentsForLayerByApplyingTransform, this method is getting too big.
https://codereview.chromium.org/731933004/diff/1/Source/core/paint/LayerPaint... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/1/Source/core/paint/LayerPaint... Source/core/paint/LayerPainter.cpp:94: LayerFragments fragments; On 2014/11/17 23:27:49, chrishtr wrote: > Factor all this code into a new method > paintFragmentsForLayerByApplyingTransform, this method is getting too big. What I was going to do next (see the first FIXME) was to move everything into paintLayerByApplyingTransform(). But having two methods would probably be even cleaner. Done, but I went for different names. I renamed old paintLayerByApplyingTransform() to paintFragmentByApplyingTransform(), and moved everything else to a new method simply named paintLayerWithTransform(). Also renamed the parameter translationOffset to fragmentTranslation and removed the default value. Let me know what you think.
https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:394: RenderLayer* parentLayer = &m_renderLayer == paintingInfo.rootLayer ? 0 : m_renderLayer.parent(); This logic is new? Fixes a bug? https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:413: paginationLayer->collectFragments(fragments, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, cacheSlot, IgnoreOverlayScrollbarSize, respectOverflowClip, 0, paintingInfo.subPixelAccumulation, &transformedExtent); Add a RenderLayer::collectFragments method that hides the logic from lines 405-420. Also, the existing RenderLayer::colectFragments collects all fragments below paintingInfo.rootLayer, right? Why does this need to happen when painting layers that are not the root, doesn't it cause redundant painting? Is this avoided by having early returns in RenderLayer::colectFragments? https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:433: if (clipRectForFragment.hasRadius()) You're going to need to rebase on top of my latest patch.
https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:394: RenderLayer* parentLayer = &m_renderLayer == paintingInfo.rootLayer ? 0 : m_renderLayer.parent(); On 2014/11/18 18:36:18, chrishtr wrote: > This logic is new? Fixes a bug? Yes, it used to only check m_renderLayer.parent(). I found it ugly that we risked jumping over paintingInfo.rootLayer that way, and walk all the way to RenderView in search of the "root" layer (which we just skipped), so I changed it. But I don't think it fixes anything, since RenderLayerClipper::backgroundClipRect() catches the situation and returns an infinite clip rect (with no radius). This means that needsToClip() will always be false, so that we won't create a clip recorder (which would have used the parent of the root layer if we actually got there -- scary). So, thanks to something rather indistinguishable from luck, my change probably doesn't fix anything at the moment, but I'd feel safer if we made sure to not jump over the "root" layer and stumble all the way to the RenderView. There's similar stuff all over the place in RenderLayerClipper, such as backgroundClipRect(), which I pointed out above, but also getClipRects(), calculateRects() and calculateClipRects(). They all make sure to stop at the "root". Everything should stop at the "root" layer. RenderLayer::convertToLayerCoords() also does this, although there it's called ancestorLayer (a better name, IMHO). So I prefer to do the same here, but if you prefer to keep it as it was, I'll back it out. The m_renderLayer.parent() check looked heavily misguided to me, but then I found this, from 2008: http://trac.webkit.org/changeset/31007 Not sure if that means anything in current Blink, but in any case, my change shouldn't make any difference in that regard. https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:413: paginationLayer->collectFragments(fragments, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, cacheSlot, IgnoreOverlayScrollbarSize, respectOverflowClip, 0, paintingInfo.subPixelAccumulation, &transformedExtent); On 2014/11/18 18:36:18, chrishtr wrote: > Add a RenderLayer::collectFragments method that hides the logic from lines > 405-420. OK, this isn't very pretty. Need to think a little more about it. This code also potentially violates what I wrote in the review comment above regarding not walking past the "root" layer. > Also, the existing RenderLayer::colectFragments collects all fragments below > paintingInfo.rootLayer, right? It just collects the fragments for the layer passed. If you pass the pagination layer (like here), it will collect all columns. > Why does this need to happen when painting layers > that are not the root, doesn't it cause redundant painting? Is this avoided by > having early returns in RenderLayer::colectFragments? No redundant painting that I'm aware of. https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:431: if (parentLayer && needsToClip(paintingInfo, clipRectForFragment)) { All the clipping stuff above for multicol should be conditioned on parentLayer. Fixed that. https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:433: if (clipRectForFragment.hasRadius()) On 2014/11/18 18:36:18, chrishtr wrote: > You're going to need to rebase on top of my latest patch. Done.
https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:394: RenderLayer* parentLayer = &m_renderLayer == paintingInfo.rootLayer ? 0 : m_renderLayer.parent(); On 2014/11/19 19:23:01, mstensho wrote: > On 2014/11/18 18:36:18, chrishtr wrote: > > This logic is new? Fixes a bug? > > Yes, it used to only check m_renderLayer.parent(). I found it ugly that we > risked jumping over paintingInfo.rootLayer that way, and walk all the way to > RenderView in search of the "root" layer (which we just skipped), so I changed > it. But I don't think it fixes anything, since > RenderLayerClipper::backgroundClipRect() catches the situation and returns an > infinite clip rect (with no radius). This means that needsToClip() will always > be false, so that we won't create a clip recorder (which would have used the > parent of the root layer if we actually got there -- scary). > > So, thanks to something rather indistinguishable from luck, my change probably > doesn't fix anything at the moment, but I'd feel safer if we made sure to not > jump over the "root" layer and stumble all the way to the RenderView. > > There's similar stuff all over the place in RenderLayerClipper, such as > backgroundClipRect(), which I pointed out above, but also getClipRects(), > calculateRects() and calculateClipRects(). They all make sure to stop at the > "root". > > Everything should stop at the "root" layer. RenderLayer::convertToLayerCoords() > also does this, although there it's called ancestorLayer (a better name, IMHO). > > So I prefer to do the same here, but if you prefer to keep it as it was, I'll > back it out. > > The m_renderLayer.parent() check looked heavily misguided to me, but then I > found this, from 2008: http://trac.webkit.org/changeset/31007 > Not sure if that means anything in current Blink, but in any case, my change > shouldn't make any difference in that regard. I don't think it's good practice to change more than the minimum necessary amount of code when factoring functions. It's hard enough as-is to verify nothing was missed. You can do that in a patch before or after instead. https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:413: paginationLayer->collectFragments(fragments, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, cacheSlot, IgnoreOverlayScrollbarSize, respectOverflowClip, 0, paintingInfo.subPixelAccumulation, &transformedExtent); On 2014/11/19 19:23:01, mstensho wrote: > On 2014/11/18 18:36:18, chrishtr wrote: > > Add a RenderLayer::collectFragments method that hides the logic from lines > > 405-420. > > OK, this isn't very pretty. Need to think a little more about it. This code also > potentially violates what I wrote in the review comment above regarding not > walking past the "root" layer. > > > Also, the existing RenderLayer::colectFragments collects all fragments below > > paintingInfo.rootLayer, right? > > It just collects the fragments for the layer passed. If you pass the pagination > layer (like here), it will collect all columns. > > > Why does this need to happen when painting layers > > that are not the root, doesn't it cause redundant painting? Is this avoided by > > having early returns in RenderLayer::colectFragments? > > No redundant painting that I'm aware of. Let's put in a big FIXME to look closely at this code and the code in RenderLayer and fix any issues in it & refactor to make it obvious from code structure what it does and that it's correct. Code as is is fine for this refactor CL.
https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:394: RenderLayer* parentLayer = &m_renderLayer == paintingInfo.rootLayer ? 0 : m_renderLayer.parent(); On 2014/11/19 19:34:51, chrishtr wrote: > On 2014/11/19 19:23:01, mstensho wrote: > > On 2014/11/18 18:36:18, chrishtr wrote: > > > This logic is new? Fixes a bug? > > > > Yes, it used to only check m_renderLayer.parent(). I found it ugly that we > > risked jumping over paintingInfo.rootLayer that way, and walk all the way to > > RenderView in search of the "root" layer (which we just skipped), so I changed > > it. But I don't think it fixes anything, since > > RenderLayerClipper::backgroundClipRect() catches the situation and returns an > > infinite clip rect (with no radius). This means that needsToClip() will always > > be false, so that we won't create a clip recorder (which would have used the > > parent of the root layer if we actually got there -- scary). > > > > So, thanks to something rather indistinguishable from luck, my change probably > > doesn't fix anything at the moment, but I'd feel safer if we made sure to not > > jump over the "root" layer and stumble all the way to the RenderView. > > > > There's similar stuff all over the place in RenderLayerClipper, such as > > backgroundClipRect(), which I pointed out above, but also getClipRects(), > > calculateRects() and calculateClipRects(). They all make sure to stop at the > > "root". > > > > Everything should stop at the "root" layer. > RenderLayer::convertToLayerCoords() > > also does this, although there it's called ancestorLayer (a better name, > IMHO). > > > > So I prefer to do the same here, but if you prefer to keep it as it was, I'll > > back it out. > > > > The m_renderLayer.parent() check looked heavily misguided to me, but then I > > found this, from 2008: http://trac.webkit.org/changeset/31007 > > Not sure if that means anything in current Blink, but in any case, my change > > shouldn't make any difference in that regard. > > I don't think it's good practice to change more than the minimum necessary > amount of code when factoring functions. It's hard enough as-is to verify > nothing was missed. > > You can do that in a patch before or after instead. Done. https://codereview.chromium.org/731933004/diff/20001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:413: paginationLayer->collectFragments(fragments, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, cacheSlot, IgnoreOverlayScrollbarSize, respectOverflowClip, 0, paintingInfo.subPixelAccumulation, &transformedExtent); On 2014/11/19 19:34:51, chrishtr wrote: > On 2014/11/19 19:23:01, mstensho wrote: > > On 2014/11/18 18:36:18, chrishtr wrote: > > > Add a RenderLayer::collectFragments method that hides the logic from lines > > > 405-420. > > > > OK, this isn't very pretty. Need to think a little more about it. This code > also > > potentially violates what I wrote in the review comment above regarding not > > walking past the "root" layer. > > > > > Also, the existing RenderLayer::colectFragments collects all fragments below > > > paintingInfo.rootLayer, right? > > > > It just collects the fragments for the layer passed. If you pass the > pagination > > layer (like here), it will collect all columns. > > > > > Why does this need to happen when painting layers > > > that are not the root, doesn't it cause redundant painting? Is this avoided > by > > > having early returns in RenderLayer::colectFragments? > > > > No redundant painting that I'm aware of. > > Let's put in a big FIXME to look closely at this code and the code in > RenderLayer and fix any issues in it & refactor to make it obvious from code > structure what it does and that it's correct. Code as is is fine for this > refactor CL. Done.
https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:357: ClipRect clipRect(LayoutRect::infiniteRect()); What about intersecting with paintingInfo.paintDirtyRect here and after line 363, like it was in the old code? https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:392: clipRectForFragment.moveBy(fragment.paginationOffset); Ideally, you would adjust for paginationOffset inside ClipRecorder, since it's being passed there anyway. Maybe also pass the clipRectForFragment to the ClipRecorder to make the optimization on lines 394-395 available for all caller uniformly?
https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:357: ClipRect clipRect(LayoutRect::infiniteRect()); On 2014/11/20 01:46:31, chrishtr wrote: > What about intersecting with paintingInfo.paintDirtyRect here and after line > 363, like it was in the old code? Can't do it this early for multicol. The clip rectangle is in flow thread coordinates, while the paint dirty rect is in visual coordinates. For multicol it's replaced with intersecting with fragment.backgroundRect, after having translated for a given column. Should really do the same for non-multicol. I'll set backgroundRect for non-multicol as well. Done. https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:392: clipRectForFragment.moveBy(fragment.paginationOffset); On 2014/11/20 01:46:31, chrishtr wrote: > Ideally, you would adjust for paginationOffset inside ClipRecorder, since it's > being passed there anyway. > > Maybe also pass the clipRectForFragment to the ClipRecorder to make the > optimization on lines 394-395 available for all caller uniformly? Before creating a ClipRecorder, we call needsToClip(). We need the final clipRectForFragment before we can call that one. But yeah, it does feel a bit weird for ClipRecorder() to take a visual clip rectangle (translated to the current column), and, at the same time take take a separate translation parameter. Something to consider cleaning up in a separate CL?
lgtm https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:392: clipRectForFragment.moveBy(fragment.paginationOffset); On 2014/11/20 09:37:55, mstensho wrote: > On 2014/11/20 01:46:31, chrishtr wrote: > > Ideally, you would adjust for paginationOffset inside ClipRecorder, since it's > > being passed there anyway. > > > > Maybe also pass the clipRectForFragment to the ClipRecorder to make the > > optimization on lines 394-395 available for all caller uniformly? > > Before creating a ClipRecorder, we call needsToClip(). We need the final > clipRectForFragment before we can call that one. > > But yeah, it does feel a bit weird for ClipRecorder() to take a visual clip > rectangle (translated to the current column), and, at the same time take take a > separate translation parameter. Something to consider cleaning up in a separate > CL? Yes. Probably worth a FIXME.
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/731933004/120001
https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/731933004/diff/80001/Source/core/paint/LayerP... Source/core/paint/LayerPainter.cpp:392: clipRectForFragment.moveBy(fragment.paginationOffset); On 2014/11/20 18:18:19, chrishtr wrote: > On 2014/11/20 09:37:55, mstensho wrote: > > On 2014/11/20 01:46:31, chrishtr wrote: > > > Ideally, you would adjust for paginationOffset inside ClipRecorder, since > it's > > > being passed there anyway. > > > > > > Maybe also pass the clipRectForFragment to the ClipRecorder to make the > > > optimization on lines 394-395 available for all caller uniformly? > > > > Before creating a ClipRecorder, we call needsToClip(). We need the final > > clipRectForFragment before we can call that one. > > > > But yeah, it does feel a bit weird for ClipRecorder() to take a visual clip > > rectangle (translated to the current column), and, at the same time take take > a > > separate translation parameter. Something to consider cleaning up in a > separate > > CL? > > Yes. Probably worth a FIXME. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/31924)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/731933004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/31937)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/731933004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/31993)
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/731933004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=185777 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
