Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727223002:
Round the subpixel accumulation used for composited scrolling content (Closed)
DescriptionRound the subpixel accumulation used for composited scrolling content
When painting the scrolling content layer background the composited
layer for the background does not include the fractional part of the
subpixel accumulation in the layer position. In the test case included
we would not fully paint the background due to this incorrect paint
offset.
BUG=692486
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Patch Set 1 #Patch Set 2 : Adjust offset passed to paintBoxDecorationBackgroundWithRect. #Patch Set 3 : Round the adjusted offset when painting into the composited scrolling contents layer. #Patch Set 4 : Remove the fractional subpixel accumulation from the adjusted offset on composited scrolling conten… #Patch Set 5 : Round the subpixel accumulation in PaintLayerPainter for scrolling contents. #
Total comments: 1
Patch Set 6 : Add test for scrolling contents drawn content. #
Total comments: 2
Patch Set 7 : Move subpixel offset conditional into CompositedLayerMapping #Patch Set 8 : Add checks at alternate callsites to PaintLayerPaint::paintLayerContents. #Patch Set 9 : Merge and fix #
Total comments: 2
Patch Set 10 : Merge and remove subpixel adjustment for transformed fragments. #
Total comments: 2
Messages
Total messages: 43 (20 generated)
Description was changed from ========== Ignore the paintOffset when painting scrolling content layer backgrounds When painting the scrolling content layer background the composited layer for the background does not include the fractional layer paint offset of the layout box. BUG=692486 ========== to ========== Ignore the paintOffset when painting scrolling content layer backgrounds When painting the scrolling content layer background the composited layer for the background does not include the fractional layer paint offset of the layout box. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by flackr@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...
flackr@chromium.org changed reviewers: + chrishtr@chromium.org
Hey, Perhaps we want to pass in the correct paint offset but it wasn't clear to me that the caller should know that the scrolling contents layer background is a special case when we can localize that into BoxPainter like we do with the enlarged paint rect. PTAL, thanks.
What do you mean by "fractional layer paint offset"? Subpixel in some way? Isn't paintOffset a full offset in the current 2D transforom space?
Hi - do you need more feedback on this patch?
On 2017/03/08 22:47:26, chrishtr wrote: > Hi - do you need more feedback on this patch? Hi, no, sorry, I got delayed by writing feedback. The fractional paint offset is subpixel, in this particular example it is (-0.5, 0). The -0.5 comes from the fragment offset, but I haven't looked further back than that yet.
On 2017/03/09 at 00:37:17, flackr wrote: > On 2017/03/08 22:47:26, chrishtr wrote: > > Hi - do you need more feedback on this patch? > > Hi, no, sorry, I got delayed by writing feedback. The fractional paint offset is subpixel, in this particular example it is (-0.5, 0). The -0.5 comes from the fragment offset, but I haven't looked further back than that yet. Ok I'll wait then.
https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:331: subpixelAccumulation = LayoutSize(roundedIntSize(subpixelAccumulation)); I found where we seem to first make the incorrect calculation. If we're painting composited scrolling content CompositedLayerMapping::updateScrollingLayerGeometry rounds the subpixel accumulation offset applied to the layer position (see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... ).
Description was changed from ========== Ignore the paintOffset when painting scrolling content layer backgrounds When painting the scrolling content layer background the composited layer for the background does not include the fractional layer paint offset of the layout box. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Round the subpixel accumulation used for composited scrolling content When painting the scrolling content layer background the composited layer for the background does not include the fractional part of the subpixel accumulation in the layer position. In the test case included we would not fully paint the background due to this incorrect paint offset. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2017/03/14 at 21:07:19, flackr wrote: > https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): > > https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:331: subpixelAccumulation = LayoutSize(roundedIntSize(subpixelAccumulation)); > I found where we seem to first make the incorrect calculation. If we're painting composited scrolling content CompositedLayerMapping::updateScrollingLayerGeometry rounds the subpixel accumulation offset applied to the layer position (see > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... ). I see. But it has to round to integer given the current interface for offsetFromLayoutObject... Is your latest patch set the one you think could be committed?
On 2017/03/15 21:55:09, chrishtr wrote: > On 2017/03/14 at 21:07:19, flackr wrote: > > > https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): > > > > > https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:331: > subpixelAccumulation = LayoutSize(roundedIntSize(subpixelAccumulation)); > > I found where we seem to first make the incorrect calculation. If we're > painting composited scrolling content > CompositedLayerMapping::updateScrollingLayerGeometry rounds the subpixel > accumulation offset applied to the layer position (see > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... > ). > > I see. But it has to round to integer given the current interface for > offsetFromLayoutObject... I'm not sure I follow where you're going here... > > Is your latest patch set the one you think could be committed? I just need to convince myself that the painting of the rest of the overflow scrolling content is okay not to include the subpixel accumulation offset, since it seems like patchset 5, unlike 4, would change it for all of the scrolling contents. I think it's correct and should fix a similar bug, but I'll add a test for the content position / size.
On 2017/03/16 at 18:10:31, flackr wrote: > On 2017/03/15 21:55:09, chrishtr wrote: > > On 2017/03/14 at 21:07:19, flackr wrote: > > > > > https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): > > > > > > > > https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:331: > > subpixelAccumulation = LayoutSize(roundedIntSize(subpixelAccumulation)); > > > I found where we seem to first make the incorrect calculation. If we're > > painting composited scrolling content > > CompositedLayerMapping::updateScrollingLayerGeometry rounds the subpixel > > accumulation offset applied to the layer position (see > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... > > ). > > > > I see. But it has to round to integer given the current interface for > > offsetFromLayoutObject... > > I'm not sure I follow where you're going here... Float vs int on GraphicsLayer...the thing you're bringing up on chrome-gpu. > > > > Is your latest patch set the one you think could be committed? > > I just need to convince myself that the painting of the rest of the overflow scrolling content is okay not to include the subpixel accumulation offset, since it seems like patchset 5, unlike 4, would change it for all of the scrolling contents. I think it's correct and should fix a similar bug, but I'll add a test for the content position / size. Ok.
On 2017/03/16 18:48:14, chrishtr wrote: > On 2017/03/16 at 18:10:31, flackr wrote: > > On 2017/03/15 21:55:09, chrishtr wrote: > > > On 2017/03/14 at 21:07:19, flackr wrote: > > > > > > > > https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/2727223002/diff/80001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:331: > > > subpixelAccumulation = LayoutSize(roundedIntSize(subpixelAccumulation)); > > > > I found where we seem to first make the incorrect calculation. If we're > > > painting composited scrolling content > > > CompositedLayerMapping::updateScrollingLayerGeometry rounds the subpixel > > > accumulation offset applied to the layer position (see > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... > > > ). > > > > > > I see. But it has to round to integer given the current interface for > > > offsetFromLayoutObject... > > > > I'm not sure I follow where you're going here... > > Float vs int on GraphicsLayer...the thing you're bringing up on chrome-gpu. > > > > > > > Is your latest patch set the one you think could be committed? > > > > I just need to convince myself that the painting of the rest of the overflow > scrolling content is okay not to include the subpixel accumulation offset, since > it seems like patchset 5, unlike 4, would change it for all of the scrolling > contents. I think it's correct and should fix a similar bug, but I'll add a test > for the content position / size. > > Ok. Yes, it turned out the same offset bug existed for the scrolling contents content as well. I've added a test for this and verified that everything looks good. PTAL, thanks!
https://codereview.chromium.org/2727223002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): https://codereview.chromium.org/2727223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:325: if (isPaintingScrollingContent) I think you can move the logic determining subpixel accumulation entirely up into CompositedLayerMapping, and pass it through paintingInfoArg. Then the conditional on lines 320-322 can be removed, and the code can always obey paintingInfoArg.subPixelAccumulation.
The CQ bit was checked by flackr@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...
https://codereview.chromium.org/2727223002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): https://codereview.chromium.org/2727223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:325: if (isPaintingScrollingContent) On 2017/04/07 21:03:12, chrishtr wrote: > I think you can move the logic determining subpixel accumulation entirely > up into CompositedLayerMapping, and pass it through paintingInfoArg. > Then the conditional on lines 320-322 can be removed, and the code can always > obey paintingInfoArg.subPixelAccumulation. Done. The conditional on 320 was strange since it seems that paintingInfoArg.subPixelAccumulation is passed in from CLM as the paint layers subpixel accumulation (i.e. the same value). There are a couple of other callsites where the passed in accumulation value is (0, 0), so perhaps the conditional is to make sure we respect the layer accumulation in those cases? In any event I've added checks to these callsites to see if this change would cause any accumulation changes, I suspect not, but I'm running tryjobs to see if it triggers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 flackr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
https://codereview.chromium.org/2727223002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): https://codereview.chromium.org/2727223002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:823: paint_layer_.GetCompositingState() == kPaintsIntoOwnBacking Why is this change needed?
Ping - is this CL still relevant?
Yes, this is still relevant, the bug still exists. I've removed the change in paint with transform for now. https://codereview.chromium.org/2727223002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): https://codereview.chromium.org/2727223002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:823: paint_layer_.GetCompositingState() == kPaintsIntoOwnBacking On 2017/04/21 15:50:51, chrishtr wrote: > Why is this change needed? This was to make the code behave the same as before (i.e. the removed / moved code from PaintLayerPainter::Paint) but I'm not sure if it's necessary yet. Without this change the bug is still fixed and my tests pass.
The CQ bit was checked by flackr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
Description was changed from ========== Round the subpixel accumulation used for composited scrolling content When painting the scrolling content layer background the composited layer for the background does not include the fractional part of the subpixel accumulation in the layer position. In the test case included we would not fully paint the background due to this incorrect paint offset. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Round the subpixel accumulation used for composited scrolling content When painting the scrolling content layer background the composited layer for the background does not include the fractional part of the subpixel accumulation in the layer position. In the test case included we would not fully paint the background due to this incorrect paint offset. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Round the subpixel accumulation used for composited scrolling content When painting the scrolling content layer background the composited layer for the background does not include the fractional part of the subpixel accumulation in the layer position. In the test case included we would not fully paint the background due to this incorrect paint offset. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Round the subpixel accumulation used for composited scrolling content When painting the scrolling content layer background the composited layer for the background does not include the fractional part of the subpixel accumulation in the layer position. In the test case included we would not fully paint the background due to this incorrect paint offset. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:3054: sub_pixel_accumulation = This will round off the subpixel accumulation for the entire composited scrolling layer, right? Why is that correct? Now I'm no longer sure I understand why the background is not being painted to the edge of the composited layer. Layers are positioned at snapped locations, but they have subpixel offsets to record the extra offset.
https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:3054: sub_pixel_accumulation = On 2017/05/05 01:14:09, chrishtr wrote: > This will round off the subpixel accumulation for the entire composited > scrolling layer, right? Why is that correct? Now I'm no longer sure > I understand why the background is not being painted to the edge of the > composited layer. Layers are positioned at snapped locations, but they > have subpixel offsets to record the extra offset. The composited scrolling content geometry size does not take into account the scroller's location, but the subpixel accumulation and painting code does. This leads to the 1px gap. I'm now thinking the better change is to compute the composited scrolling layer geometry with the correct size based on the layout box location and then we could respect the subpixel accumulation. I'll start doing this but let me know if you have any objections.
On 2017/05/05 at 17:02:19, flackr wrote: > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:3054: sub_pixel_accumulation = > On 2017/05/05 01:14:09, chrishtr wrote: > > This will round off the subpixel accumulation for the entire composited > > scrolling layer, right? Why is that correct? Now I'm no longer sure > > I understand why the background is not being painted to the edge of the > > composited layer. Layers are positioned at snapped locations, but they > > have subpixel offsets to record the extra offset. > > The composited scrolling content geometry size does not take into account the scroller's location, but the subpixel accumulation and painting code does. This leads to the 1px gap. I'm now thinking the better change is to compute the composited scrolling layer geometry with the correct size based on the layout box location and then we could respect the subpixel accumulation. I'll start doing this but let me know if you have any objections. Do you mean that scrolling_contents_layer_'s position does not take into account subpixel offset of the scrolling DOM element? That seems wrong I agree. graphics_layer_ does do that.
On 2017/05/05 18:01:18, chrishtr wrote: > On 2017/05/05 at 17:02:19, flackr wrote: > > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > > File > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > (right): > > > > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:3054: > sub_pixel_accumulation = > > On 2017/05/05 01:14:09, chrishtr wrote: > > > This will round off the subpixel accumulation for the entire composited > > > scrolling layer, right? Why is that correct? Now I'm no longer sure > > > I understand why the background is not being painted to the edge of the > > > composited layer. Layers are positioned at snapped locations, but they > > > have subpixel offsets to record the extra offset. > > > > The composited scrolling content geometry size does not take into account the > scroller's location, but the subpixel accumulation and painting code does. This > leads to the 1px gap. I'm now thinking the better change is to compute the > composited scrolling layer geometry with the correct size based on the layout > box location and then we could respect the subpixel accumulation. I'll start > doing this but let me know if you have any objections. > > Do you mean that scrolling_contents_layer_'s position does not take into account > subpixel offset of the scrolling DOM element? That seems wrong I agree. > graphics_layer_ does do that. Not its position, but its size doesn't take into account its position: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... For example, a scroller with width=100.5px at x=4.5px has its composited layer positioned at x=5px (so it should only be 100px wide) but calls ToLayoutBox(owning_layer_.GetLayoutObject()).PixelSnappedBorderBoxRect(); which computes the pixel snapped rect assuming it is at (0, 0) which returns 101px instead of 100px. Then the painting code paints into the layer at x=4.5px with width=100.5px which leaves the pixels at x=105 empty. I will change this to compute the size using its current position, so this will make the scroller only 100px wide when at x=4.5px, but 101px wide if it was at 4px.
On 2017/05/05 18:01:18, chrishtr wrote: > On 2017/05/05 at 17:02:19, flackr wrote: > > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > > File > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > (right): > > > > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:3054: > sub_pixel_accumulation = > > On 2017/05/05 01:14:09, chrishtr wrote: > > > This will round off the subpixel accumulation for the entire composited > > > scrolling layer, right? Why is that correct? Now I'm no longer sure > > > I understand why the background is not being painted to the edge of the > > > composited layer. Layers are positioned at snapped locations, but they > > > have subpixel offsets to record the extra offset. > > > > The composited scrolling content geometry size does not take into account the > scroller's location, but the subpixel accumulation and painting code does. This > leads to the 1px gap. I'm now thinking the better change is to compute the > composited scrolling layer geometry with the correct size based on the layout > box location and then we could respect the subpixel accumulation. I'll start > doing this but let me know if you have any objections. > > Do you mean that scrolling_contents_layer_'s position does not take into account > subpixel offset of the scrolling DOM element? That seems wrong I agree. > graphics_layer_ does do that. Not its position, but its size doesn't take into account its position: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... For example, a scroller with width=100.5px at x=4.5px has its composited layer positioned at x=5px (so it should only be 100px wide) but calls ToLayoutBox(owning_layer_.GetLayoutObject()).PixelSnappedBorderBoxRect(); which computes the pixel snapped rect assuming it is at (0, 0) which returns 101px instead of 100px. Then the painting code paints into the layer at x=4.5px with width=100.5px which leaves the pixels at x=105 empty. I will change this to compute the size using its current position, so this will make the scroller only 100px wide when at x=4.5px, but 101px wide if it was at 4px.
On 2017/05/05 at 18:30:14, flackr wrote: > On 2017/05/05 18:01:18, chrishtr wrote: > > On 2017/05/05 at 17:02:19, flackr wrote: > > > > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > > > File > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > > (right): > > > > > > > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:3054: > > sub_pixel_accumulation = > > > On 2017/05/05 01:14:09, chrishtr wrote: > > > > This will round off the subpixel accumulation for the entire composited > > > > scrolling layer, right? Why is that correct? Now I'm no longer sure > > > > I understand why the background is not being painted to the edge of the > > > > composited layer. Layers are positioned at snapped locations, but they > > > > have subpixel offsets to record the extra offset. > > > > > > The composited scrolling content geometry size does not take into account the > > scroller's location, but the subpixel accumulation and painting code does. This > > leads to the 1px gap. I'm now thinking the better change is to compute the > > composited scrolling layer geometry with the correct size based on the layout > > box location and then we could respect the subpixel accumulation. I'll start > > doing this but let me know if you have any objections. > > > > Do you mean that scrolling_contents_layer_'s position does not take into account > > subpixel offset of the scrolling DOM element? That seems wrong I agree. > > graphics_layer_ does do that. > > Not its position, but its size doesn't take into account its position: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... > > For example, a scroller with width=100.5px at x=4.5px has its composited layer positioned at x=5px (so it should only be 100px wide) but calls ToLayoutBox(owning_layer_.GetLayoutObject()).PixelSnappedBorderBoxRect(); which computes the pixel snapped rect assuming it is at (0, 0) which returns 101px instead of 100px. Then the painting code paints into the layer at x=4.5px with width=100.5px which leaves the pixels at x=105 empty. > > I will change this to compute the size using its current position, so this will make the scroller only 100px wide when at x=4.5px, but 101px wide if it was at 4px. Ah width, right. It should use pixelSnappedIntRect, right?
On 2017/05/05 18:32:35, chrishtr wrote: > On 2017/05/05 at 18:30:14, flackr wrote: > > On 2017/05/05 18:01:18, chrishtr wrote: > > > On 2017/05/05 at 17:02:19, flackr wrote: > > > > > > > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > > > > File > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > > > (right): > > > > > > > > > > > > https://codereview.chromium.org/2727223002/diff/180001/third_party/WebKit/Sou... > > > > > > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:3054: > > > sub_pixel_accumulation = > > > > On 2017/05/05 01:14:09, chrishtr wrote: > > > > > This will round off the subpixel accumulation for the entire composited > > > > > scrolling layer, right? Why is that correct? Now I'm no longer sure > > > > > I understand why the background is not being painted to the edge of the > > > > > composited layer. Layers are positioned at snapped locations, but they > > > > > have subpixel offsets to record the extra offset. > > > > > > > > The composited scrolling content geometry size does not take into account > the > > > scroller's location, but the subpixel accumulation and painting code does. > This > > > leads to the 1px gap. I'm now thinking the better change is to compute the > > > composited scrolling layer geometry with the correct size based on the > layout > > > box location and then we could respect the subpixel accumulation. I'll start > > > doing this but let me know if you have any objections. > > > > > > Do you mean that scrolling_contents_layer_'s position does not take into > account > > > subpixel offset of the scrolling DOM element? That seems wrong I agree. > > > graphics_layer_ does do that. > > > > Not its position, but its size doesn't take into account its position: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... > > > > For example, a scroller with width=100.5px at x=4.5px has its composited layer > positioned at x=5px (so it should only be 100px wide) but calls > ToLayoutBox(owning_layer_.GetLayoutObject()).PixelSnappedBorderBoxRect(); which > computes the pixel snapped rect assuming it is at (0, 0) which returns 101px > instead of 100px. Then the painting code paints into the layer at x=4.5px with > width=100.5px which leaves the pixels at x=105 empty. > > > > I will change this to compute the size using its current position, so this > will make the scroller only 100px wide when at x=4.5px, but 101px wide if it was > at 4px. > > Ah width, right. It should use pixelSnappedIntRect, right? Right, there's a bunch of functions we're calling that currently assume the location is (0, 0) which will need to be updated. Also, because we use subpixel accumulation as an offset to the composited layer location we'll have to adjust the size according to pixel snapping applied for rounded subpixel adjustment.
As per our conversation I've abandoned this approach as it seems the right thing to do is correctly pixel snap the composited scrolling contents layers according to the subpixel accumulaton. I've done this in https://chromium-review.googlesource.com/c/509175/
Description was changed from ========== Round the subpixel accumulation used for composited scrolling content When painting the scrolling content layer background the composited layer for the background does not include the fractional part of the subpixel accumulation in the layer position. In the test case included we would not fully paint the background due to this incorrect paint offset. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Round the subpixel accumulation used for composited scrolling content When painting the scrolling content layer background the composited layer for the background does not include the fractional part of the subpixel accumulation in the layer position. In the test case included we would not fully paint the background due to this incorrect paint offset. BUG=692486 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== |
