Chromium Code Reviews
|
|
|
Created:
4 years, 11 months ago by Xianzhu Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, Rik, danakj, dshwang, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, slimming-paint-reviews_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDon't cache reflection drawings
This is a quick fix of under-invalidation of reflections.
BUG=495406
TEST=fast/repaint/scroll-absolute-layer-with-reflection.html
TEST=fast/repaint/scroll-fixed-layer-with-reflection.html
(Tests need https://codereview.chromium.org/1159113003/
and SlimmingPaintUnderInvalidationChecking)
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196416
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add DisplayItemCachingSkipper.h; Restore m_scopeId #
Total comments: 3
Patch Set 3 : DisplayItem enum Type : uint16_t #
Total comments: 4
Patch Set 4 : Allows only DisplayItemList to access skippedCache flag #Patch Set 5 : Why MSVC failed? #Patch Set 6 : bitfield again #Patch Set 7 : Try win #Patch Set 8 : Try win again #Patch Set 9 : Try win 3 #Patch Set 10 : Try win 4 #Patch Set 11 : Fix win (which doesn't pack bool bitfields) #
Messages
Total messages: 41 (12 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org, wkorman@chromium.org
Added wkorman@ because this CL may be related to your CL for bug 427743.
This adds quite a bit of complexity. Would it be possible to just invalidate the cache directly from the reflection painting code?
How about just always invalidating the source of a reflection when reflection is added?
On 2015/06/02 05:33:33, pdr wrote: > This adds quite a bit of complexity. Would it be possible to just invalidate the > cache directly from the reflection painting code? Invalidation during painting is not good, because when we invalidate some object, we might have issued CachedDisplayItems for it. Currently we disallow invalidation-during-painting with an ASSERT (which has discovered several bugs (crbug.com/457415)). The added beginSkippingCache/endSkippingCache may be also useful for some cases that previously we used invalidatePaintIncludingNonCompositingDescendants() for slimming paint. We can avoid the subtree walkings using the new functions.
On 2015/06/02 15:44:34, chrishtr wrote: > How about just always invalidating the source of a reflection when reflection is > added? Though adding reflection is a bug case that both this CL and invalidating reflection source can fix, fast/repaint/scroll-absolute-layer-with-reflection.html is a different failing case that can't be fixed by invalidating source. The failure happens as below: - During the first paint, the source is in-view but the reflection is out-of-view. We paint the source for itself; - Then the scrolling area is scrolled, and the reflection is in-view. We paint the source for both itself and reflection. In this sequence, we have no chance to invalidate the source. For optimal result, we might want to let the source and reflection share the paintings, but this may need a bigger change so I'd like to do it in phase 2.
On 2015/06/02 at 16:03:49, wangxianzhu wrote: > On 2015/06/02 15:44:34, chrishtr wrote: > > How about just always invalidating the source of a reflection when reflection is > > added? > > Though adding reflection is a bug case that both this CL and invalidating reflection source can fix, fast/repaint/scroll-absolute-layer-with-reflection.html is a different failing case that can't be fixed by invalidating source. > > The failure happens as below: > - During the first paint, the source is in-view but the reflection is out-of-view. We paint the source for itself; > - Then the scrolling area is scrolled, and the reflection is in-view. We paint the source for both itself and reflection. By out-of-view you mean outside of the interest rect that is 8000px high/wide? Content outside of the visible viewport is still painted. > > In this sequence, we have no chance to invalidate the source. > > For optimal result, we might want to let the source and reflection share the paintings, but this may need a bigger change so I'd like to do it in phase 2.
On 2015/06/02 16:28:50, chrishtr wrote: > On 2015/06/02 at 16:03:49, wangxianzhu wrote: > > On 2015/06/02 15:44:34, chrishtr wrote: > > > How about just always invalidating the source of a reflection when > reflection is > > > added? > > > > Though adding reflection is a bug case that both this CL and invalidating > reflection source can fix, > fast/repaint/scroll-absolute-layer-with-reflection.html is a different failing > case that can't be fixed by invalidating source. > > > > The failure happens as below: > > - During the first paint, the source is in-view but the reflection is > out-of-view. We paint the source for itself; > > - Then the scrolling area is scrolled, and the reflection is in-view. We paint > the source for both itself and reflection. > > By out-of-view you mean outside of the interest rect that is 8000px high/wide? > Content outside of the visible viewport is still painted. > Either, for different cases: - For composited scrolling, it's interest rect which is passed from cc; - For non-composited scrolling, it's the size of the scrollable area which is defined by DeprecasedPaintingLayerPainter.
On 2015/06/02 at 16:44:34, wangxianzhu wrote: > On 2015/06/02 16:28:50, chrishtr wrote: > > On 2015/06/02 at 16:03:49, wangxianzhu wrote: > > > On 2015/06/02 15:44:34, chrishtr wrote: > > > > How about just always invalidating the source of a reflection when > > reflection is > > > > added? > > > > > > Though adding reflection is a bug case that both this CL and invalidating > > reflection source can fix, > > fast/repaint/scroll-absolute-layer-with-reflection.html is a different failing > > case that can't be fixed by invalidating source. > > > > > > The failure happens as below: > > > - During the first paint, the source is in-view but the reflection is > > out-of-view. We paint the source for itself; > > > - Then the scrolling area is scrolled, and the reflection is in-view. We paint > > the source for both itself and reflection. > > > > By out-of-view you mean outside of the interest rect that is 8000px high/wide? > > Content outside of the visible viewport is still painted. > > > > Either, for different cases: > - For composited scrolling, it's interest rect which is passed from cc; > - For non-composited scrolling, it's the size of the scrollable area which is defined by DeprecasedPaintingLayerPainter. I see. Good points.
https://codereview.chromium.org/1160763005/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1160763005/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerPainter.cpp:29: #include "platform/graphics/paint/DisplayItemCacheSkipper.h" I think you accidentally left this class out of the CL. https://codereview.chromium.org/1160763005/diff/1/Source/platform/graphics/pa... File Source/platform/graphics/paint/DisplayItem.h (left): https://codereview.chromium.org/1160763005/diff/1/Source/platform/graphics/pa... Source/platform/graphics/paint/DisplayItem.h:286: struct Id { What's the point of flattening the struct? Is it not possible to pack the memory otherwise?
https://codereview.chromium.org/1160763005/diff/1/Source/platform/graphics/pa... File Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/1160763005/diff/1/Source/platform/graphics/pa... Source/platform/graphics/paint/DisplayItem.h:295: unsigned m_scopeId : 16; This seems dangerous in the future and doesn't do anything today.
https://codereview.chromium.org/1160763005/diff/1/Source/core/paint/Deprecate... File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right): https://codereview.chromium.org/1160763005/diff/1/Source/core/paint/Deprecate... Source/core/paint/DeprecatedPaintLayerPainter.cpp:29: #include "platform/graphics/paint/DisplayItemCacheSkipper.h" On 2015/06/02 17:06:56, chrishtr wrote: > I think you accidentally left this class out of the CL. Done. https://codereview.chromium.org/1160763005/diff/1/Source/platform/graphics/pa... File Source/platform/graphics/paint/DisplayItem.h (left): https://codereview.chromium.org/1160763005/diff/1/Source/platform/graphics/pa... Source/platform/graphics/paint/DisplayItem.h:286: struct Id { On 2015/06/02 17:06:56, chrishtr wrote: > What's the point of flattening the struct? Is it not possible to pack the memory > otherwise? Otherwise it would be: struct Id { ... }; bool m_skippedCache; // not part of Id Flattening the struct also makes it possible to borrow more bits from m_type (e.g. we can add bits to avoid the virtual isXXX functions). https://codereview.chromium.org/1160763005/diff/1/Source/platform/graphics/pa... File Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/1160763005/diff/1/Source/platform/graphics/pa... Source/platform/graphics/paint/DisplayItem.h:295: unsigned m_scopeId : 16; On 2015/06/02 17:16:08, pdr wrote: > This seems dangerous in the future and doesn't do anything today. Restored back to 'int'.
At the risk of beating a dead horse, a drive-by comment. https://codereview.chromium.org/1160763005/diff/20001/Source/platform/graphic... File Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/1160763005/diff/20001/Source/platform/graphic... Source/platform/graphics/paint/DisplayItem.h:297: const Type m_type : 16; Why use a bitfield here when you could simply reduce the size of the underlying type? (e.g. "enum Type : uint16_t { ... }")
https://codereview.chromium.org/1160763005/diff/20001/Source/platform/graphic... File Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/1160763005/diff/20001/Source/platform/graphic... Source/platform/graphics/paint/DisplayItem.h:297: const Type m_type : 16; On 2015/06/02 18:02:23, jbroman wrote: > Why use a bitfield here when you could simply reduce the size of the underlying > type? (e.g. "enum Type : uint16_t { ... }") Great! Didn't know this grammar before. Done.
https://codereview.chromium.org/1160763005/diff/40001/Source/platform/graphic... File Source/platform/graphics/paint/DisplayItem.cpp (right): https://codereview.chromium.org/1160763005/diff/40001/Source/platform/graphic... Source/platform/graphics/paint/DisplayItem.cpp:13: int ints[2]; // Make sure other fields are packed into two ints. So DisplayItem has not gotten bigger due to this patch?
https://codereview.chromium.org/1160763005/diff/40001/Source/platform/graphic... File Source/platform/graphics/paint/DisplayItem.cpp (right): https://codereview.chromium.org/1160763005/diff/40001/Source/platform/graphic... Source/platform/graphics/paint/DisplayItem.cpp:13: int ints[2]; // Make sure other fields are packed into two ints. On 2015/06/02 20:40:18, chrishtr wrote: > So DisplayItem has not gotten bigger due to this patch? Right.
https://codereview.chromium.org/1160763005/diff/40001/Source/platform/graphic... File Source/platform/graphics/paint/DisplayItemList.cpp (right): https://codereview.chromium.org/1160763005/diff/40001/Source/platform/graphic... Source/platform/graphics/paint/DisplayItemList.cpp:75: ASSERT(!displayItem.skippedCache()); // random code should not be setting this bit, only DisplayItemList. ?
https://codereview.chromium.org/1160763005/diff/40001/Source/platform/graphic... File Source/platform/graphics/paint/DisplayItemList.cpp (right): https://codereview.chromium.org/1160763005/diff/40001/Source/platform/graphic... Source/platform/graphics/paint/DisplayItemList.cpp:75: On 2015/06/02 20:58:37, chrishtr wrote: > ASSERT(!displayItem.skippedCache()); // random code should not be setting this > bit, only DisplayItemList. ? Added the assert. It's reasonable to allow only DisplayItemList to access the bit. Painters use DisplayItemCacheSkipper instead.
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160763005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64872)
https://codereview.chromium.org/1160763005/diff/20001/Source/platform/graphic... File Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/1160763005/diff/20001/Source/platform/graphic... Source/platform/graphics/paint/DisplayItem.h:297: const Type m_type : 16; On 2015/06/02 18:41:59, Xianzhu wrote: > On 2015/06/02 18:02:23, jbroman wrote: > > Why use a bitfield here when you could simply reduce the size of the > underlying > > type? (e.g. "enum Type : uint16_t { ... }") > > Great! Didn't know this grammar before. Done. MSVC failed to compile '+', '-' operators of the enum. Changed it back to bitfield. Added a static_assert to ensure the type fits in the bitfield.
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1160763005/#ps100001 (title: "bitfield again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160763005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1160763005/#ps200001 (title: "Fix win (which doesn't pack bool bitfields)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160763005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64906)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160763005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64943)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160763005/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196416 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
