Invalidate non-composited content host when page scale factor changes
Created attachment 171355 [details] Patch
Comment on attachment 171355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171355&action=review Needs a test. > Source/WebKit/chromium/ChangeLog:11 > + a special case since it doesn't get notified by the WebCore::Page. Why is that?
In Page::setPageScaleFactor() I see a setNeedsLayout(true) followed by a recalcStyle(Node::Force) - I would definitely expect this sequence to generate quite a few invalidations on the non composited content.
(In reply to comment #3) > In Page::setPageScaleFactor() I see a setNeedsLayout(true) followed by a recalcStyle(Node::Force) - I would definitely expect this sequence to generate quite a few invalidations on the non composited content. The RenderObject do generate a bunch of invalidation, but I doubt they're not redirected to the non-composited content layer (the contents are supposed to be on RenderView layer). The only invalidation non-composited content layer gets is a rect of size of the CSS viewport, scaled by the page scale factor. I don't exactly know where does this rect come from, probably the size of RenderView(= size of CSS viewport), then wrongfully scaled with page scale factor. However even if it's correctly unscaled, it's still not suffice as the compositor can scroll the layer to expose the parts that are not repainted. We can observe that on the low-res preview when pinch zooming.
This change is hard to test -- with layout tests you don't have enough control over paint timing, and with unit tests you can't check the pixels. Moreover, the class of problem it's fixing will become moot when we transition to the page-scale-applied-in-compositor model (at which point we can test this stuff entirely in cc_unittests). So unless you can suggest some straightforward way to test it, I'd argue we should land without test.
(In reply to comment #4) > (In reply to comment #3) > > In Page::setPageScaleFactor() I see a setNeedsLayout(true) followed by a recalcStyle(Node::Force) - I would definitely expect this sequence to generate quite a few invalidations on the non composited content. > > The RenderObject do generate a bunch of invalidation, but I doubt they're not redirected to the non-composited content layer (the contents are supposed to be on RenderView layer). Where do you think they go? All RenderObject invalidations go either to the non-composited content host or to a descendant composited layer. > > The only invalidation non-composited content layer gets is a rect of size of the CSS viewport, scaled by the page scale factor. I don't exactly know where does this rect come from, probably the size of RenderView(= size of CSS viewport), then wrongfully scaled with page scale factor. However even if it's correctly unscaled, it's still not suffice as the compositor can scroll the layer to expose the parts that are not repainted. We can observe that on the low-res preview when pinch zooming. Sounds like something is getting clipped incorrectly, then. We've taken some care to make sure that invalidations that should cover areas outside the currently visible viewport aren't clipped out, so if this is working incorrectly here we should track it down and fix it.
(In reply to comment #5) > This change is hard to test -- with layout tests you don't have enough control over paint timing, and with unit tests you can't check the pixels. Moreover, the class of problem it's fixing will become moot when we transition to the page-scale-applied-in-compositor model (at which point we can test this stuff entirely in cc_unittests). So unless you can suggest some straightforward way to test it, I'd argue we should land without test. In a layout test you have 100% control over paint timing - the harness paints when you call testRunner.display() or when the test ends and at no other time.
After carefully examining the whole invalidation process, I think this is the proper fix. The story goes like this: 1. WebViewImpl::setPageScaleFactor calls Page::setPageScaleFactor, which calls document->recalcStyle(Node::Force) to update the transform. 2. During the style update, RenderObject::setStyle detects the transform change on the RenderView. Since it is only a transform change, repaint is not needed (StyleDifferenceRecompositeLayer). 3. Supposedly if the compositor wants to change the contents scale to repaint a high-res version, it is the compositor's responsibility to invalidate the layer. The compositor actually does it, but on the RenderView layer, which is a no-op since RenderView layer is non-drawing layer because its content is stolen by the non-composited content layer. 4. Nobody tells non-composited content layer to repaint --> tile busted This phenomenon is not obvious, because some area of the non-composited content layer are still invalidated for other reasons: 1. renderView->setNeedsLayout(true) This line doesn't really do any change to the layout. The purpose is only to update some internal variables. Regardless, it cause the RenderView to repaint its rect, which is totally bogus as the document node doesn't represent any contents, and its rect is set to the size of layout viewport only for layout purpose. That is why we can see the upper left part of the layer being repainted, and the size is exactly the viewportSize*pageScaleFactor, which is the size of the RenderView after transformation. 2. The enlarged part of the layer automatically gets invalidated. When we pinch zoom-in, the region that was not previously inside the layer bound always get the newest contents. Combine reason 1. and 2., that is why we can't observe the phenomenon when pageScaleFactor >= 1.0 and doing zoom-in, nor when pageScaleFactor >= 1.0 and have scrollOffset == (0, 0). The conclusion is that neither the compositor or WebCore is doing anything wrong. The correct fix is to notify the non-composited content host that the RenderView transform changed, thus needs a repaint. Currently the only thing that can change RenderView transform is the pageScaleFactor, therefore I think adding a hook in WebViewImpl::setPageScaleFactor is reasonable. Now the real question is how to test this fix. I don't think DumpRenderTree uses the NonCompositedContentHost hack. We probably need to implement a webkit_unit_tests for that. I would say it is a non-trivial task and we might need to postpone the test until other high priority bugs are fixed.
James, can we land this? As Tien-Ren explained, we now think this is the correct fix. To summarize what he wrote above, the key point is that changing the CSS scale doesn't cause an invalidation.
Patching this at the WebKit layer is wrong, we should fix this in WebCore. It's not anything specific to chromium or any particular embedding of WebCore - when the page scale changes, all content on the page needs to be invalidated. This isn't a theoretical concern - we expose a window.internals function to set the page scale from layout tests (implemented in WebCore/testing/InternalSettings.cpp) that hooks directly in to WebCore::Page. This needs to have the same behavior as the actual product or layout tests intended to cover page scale will not actually cover all the logic we ship. Invalidating from WebCore should be quite easy. NCCH hosts the content that is not considered composited by WebCore (hence the name), so invalidations on the hostWindow are routed to NCCH. Calling repaint() on root Document's RenderObject should result in a correctly-sized repaint coming out via WebCore::Chrome::invalidateContentsAndRootView(). That's how style changes (such as background color) are repainting, so if that path is not working then I would expect us to see significantly more breakage.
(In reply to comment #8) > 3. Supposedly if the compositor wants to change the contents scale to repaint a high-res version, it is the compositor's responsibility to invalidate the layer. The compositor actually does it, but on the RenderView layer, which is a no-op since RenderView layer is non-drawing layer because its content is stolen by the non-composited content layer. WebCore is responsible for this property, not the compositor.
(In reply to comment #11) > (In reply to comment #8) > > 3. Supposedly if the compositor wants to change the contents scale to repaint a high-res version, it is the compositor's responsibility to invalidate the layer. The compositor actually does it, but on the RenderView layer, which is a no-op since RenderView layer is non-drawing layer because its content is stolen by the non-composited content layer. > > WebCore is responsible for this property, not the compositor. NonCompositedContentHost is owned by WebViewImpl in WebKit. So it seems reasonable to do the way the patch does. When WebCore needs to invalidate a region, it invalids a rect. This propagates to NonCompositedContentHost through WebViewImpl::invalidateRect. But when scale factor changed, we want to invalidate everything, not just the visible part.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #8) > > > 3. Supposedly if the compositor wants to change the contents scale to repaint a high-res version, it is the compositor's responsibility to invalidate the layer. The compositor actually does it, but on the RenderView layer, which is a no-op since RenderView layer is non-drawing layer because its content is stolen by the non-composited content layer. > > > > WebCore is responsible for this property, not the compositor. > > NonCompositedContentHost is owned by WebViewImpl in WebKit. So it seems reasonable to do the way the patch does. > > When WebCore needs to invalidate a region, it invalids a rect. This propagates to NonCompositedContentHost through WebViewImpl::invalidateRect. But when scale factor changed, we want to invalidate everything, not just the visible part. Sure, that's fine. That's what repaint() does.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #8) > > > > 3. Supposedly if the compositor wants to change the contents scale to repaint a high-res version, it is the compositor's responsibility to invalidate the layer. The compositor actually does it, but on the RenderView layer, which is a no-op since RenderView layer is non-drawing layer because its content is stolen by the non-composited content layer. > > > > > > WebCore is responsible for this property, not the compositor. > > > > NonCompositedContentHost is owned by WebViewImpl in WebKit. So it seems reasonable to do the way the patch does. > > > > When WebCore needs to invalidate a region, it invalids a rect. This propagates to NonCompositedContentHost through WebViewImpl::invalidateRect. But when scale factor changed, we want to invalidate everything, not just the visible part. > > Sure, that's fine. That's what repaint() does. Then can you approve the CL? In the compositor case, the composited layers are invalidated by RenderLayerCompositor. WebViewImpl needs to take care of the NonCompositedContentHost for the full layer invalidation.
(In reply to comment #10) > Patching this at the WebKit layer is wrong, we should fix this in WebCore. It's not anything specific to chromium or any particular embedding of WebCore - when the page scale changes, all content on the page needs to be invalidated. It is true that when the page scale changes we need to issue a redraw, and we also need to repaint all layers that have contents scale changed. Both are happening correctly. The only exception is NCCH for the reason I explained in previous post. It is indeed a chromium problem -- only chromium has this NCCH layer that steals contents from RenderView. If it's the embedder who re-route the painting to a different layer, then it's also the embedder's responsibility to re-route the invalidation. > This isn't a theoretical concern - we expose a window.internals function to set the page scale from layout tests (implemented in WebCore/testing/InternalSettings.cpp) that hooks directly in to WebCore::Page. This needs to have the same behavior as the actual product or layout tests intended to cover page scale will not actually cover all the logic we ship. DumpRenderTree doesn't use NCCH if my memory is correct. > Invalidating from WebCore should be quite easy. NCCH hosts the content that is not considered composited by WebCore (hence the name), so invalidations on the hostWindow are routed to NCCH. Calling repaint() on root Document's RenderObject should result in a correctly-sized repaint coming out via WebCore::Chrome::invalidateContentsAndRootView(). That's how style changes (such as background color) are repainting, so if that path is not working then I would expect us to see significantly more breakage. Background color change will work, because it implies a repaint. In contrast, changing transform matrix on a compositing layer doesn't need a repaint -- the compositor just has to redraw the layer using a different quad. (In reply to comment #11) > (In reply to comment #8) > > 3. Supposedly if the compositor wants to change the contents scale to repaint a high-res version, it is the compositor's responsibility to invalidate the layer. The compositor actually does it, but on the RenderView layer, which is a no-op since RenderView layer is non-drawing layer because its content is stolen by the non-composited content layer. > > WebCore is responsible for this property, not the compositor. I think it's updateLayerContentsScale in layer_tree_host_common.cc I don't see why WebCore is responsible for contents scale. With the new scaling model, WebCore should not even know about the existence of the page scale factor.
(In reply to comment #15) > (In reply to comment #10) > > Patching this at the WebKit layer is wrong, we should fix this in WebCore. It's not anything specific to chromium or any particular embedding of WebCore - when the page scale changes, all content on the page needs to be invalidated. > > It is true that when the page scale changes we need to issue a redraw, and we also need to repaint all layers that have contents scale changed. Both are happening correctly. The only exception is NCCH for the reason I explained in previous post. > > It is indeed a chromium problem -- only chromium has this NCCH layer that steals contents from RenderView. If it's the embedder who re-route the painting to a different layer, then it's also the embedder's responsibility to re-route the invalidation. That's not true. Every port with compositing has to deal with the non-composited (according to WebCore) content. > > > This isn't a theoretical concern - we expose a window.internals function to set the page scale from layout tests (implemented in WebCore/testing/InternalSettings.cpp) that hooks directly in to WebCore::Page. This needs to have the same behavior as the actual product or layout tests intended to cover page scale will not actually cover all the logic we ship. > > DumpRenderTree doesn't use NCCH if my memory is correct. No, it does. > > > Invalidating from WebCore should be quite easy. NCCH hosts the content that is not considered composited by WebCore (hence the name), so invalidations on the hostWindow are routed to NCCH. Calling repaint() on root Document's RenderObject should result in a correctly-sized repaint coming out via WebCore::Chrome::invalidateContentsAndRootView(). That's how style changes (such as background color) are repainting, so if that path is not working then I would expect us to see significantly more breakage. > > Background color change will work, because it implies a repaint. In contrast, changing transform matrix on a compositing layer doesn't need a repaint -- the compositor just has to redraw the layer using a different quad. Right, but changing the page scale should generate a repaint as well. > > (In reply to comment #11) > > (In reply to comment #8) > > > 3. Supposedly if the compositor wants to change the contents scale to repaint a high-res version, it is the compositor's responsibility to invalidate the layer. The compositor actually does it, but on the RenderView layer, which is a no-op since RenderView layer is non-drawing layer because its content is stolen by the non-composited content layer. > > > > WebCore is responsible for this property, not the compositor. > > I think it's updateLayerContentsScale in layer_tree_host_common.cc > I don't see why WebCore is responsible for contents scale. With the new scaling model, WebCore should not even know about the existence of the page scale factor. WebCore is responsible for the page scale factor.
Created attachment 175073 [details] Patch
Thanks for the offline discussion. I had much misunderstanding about how non-composited contents work. :) A new patch is uploaded. Is it the solution that you suggested?
Comment on attachment 175073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175073&action=review > Source/WebCore/page/Page.cpp:680 > + // Transform change on RenderView doesn't trigger repaint on non-composited contents nit: end sentence with a period > Source/WebCore/page/Page.cpp:681 > + chrome()->invalidateContentsAndRootView(IntRect(LayoutRect::infiniteRect()), false); if you go through FrameView::repaintContentRectangle or something that calls it you'll get the layout test tracking for free, since it's implemented at that layer. I think calling repaint() on the mainFrame()->contentRenderer() will do exactly what you want (although be sure to null-check it before dereferencing it), mind checking if that's the case?
(In reply to comment #19) > (From update of attachment 175073 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175073&action=review > > > Source/WebCore/page/Page.cpp:680 > > + // Transform change on RenderView doesn't trigger repaint on non-composited contents > > nit: end sentence with a period > > > Source/WebCore/page/Page.cpp:681 > > + chrome()->invalidateContentsAndRootView(IntRect(LayoutRect::infiniteRect()), false); > > if you go through FrameView::repaintContentRectangle or something that calls it you'll get the layout test tracking for free, since it's implemented at that layer. I think calling repaint() on the mainFrame()->contentRenderer() will do exactly what you want (although be sure to null-check it before dereferencing it), mind checking if that's the case? Nope, mainFrame()->contentRenderer()->repaint() doesn't work recursively. Thus it only repaints the RenderView -- which is a dummy element and its size is the layout viewport, but repaints nothing that overflows outside of the layout viewport. Lets use mainFrame()->view()->repaintContentRectangle()? Do I have to do null check for both mainFrame() and mainFrame()->view()?
(In reply to comment #20) > (In reply to comment #19) > > (From update of attachment 175073 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175073&action=review > > > > > Source/WebCore/page/Page.cpp:680 > > > + // Transform change on RenderView doesn't trigger repaint on non-composited contents > > > > nit: end sentence with a period > > > > > Source/WebCore/page/Page.cpp:681 > > > + chrome()->invalidateContentsAndRootView(IntRect(LayoutRect::infiniteRect()), false); > > > > if you go through FrameView::repaintContentRectangle or something that calls it you'll get the layout test tracking for free, since it's implemented at that layer. I think calling repaint() on the mainFrame()->contentRenderer() will do exactly what you want (although be sure to null-check it before dereferencing it), mind checking if that's the case? > > Nope, mainFrame()->contentRenderer()->repaint() doesn't work recursively. Thus it only repaints the RenderView -- which is a dummy element and its size is the layout viewport, but repaints nothing that overflows outside of the layout viewport. > > Lets use mainFrame()->view()->repaintContentRectangle()? Do I have to do null check for both mainFrame() and mainFrame()->view()? OK. mainFrame() will never be null (and it's used unguarded in this function) but its view may not be there.
(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #19) > > > (From update of attachment 175073 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=175073&action=review > > > > > > > Source/WebCore/page/Page.cpp:680 > > > > + // Transform change on RenderView doesn't trigger repaint on non-composited contents > > > > > > nit: end sentence with a period > > > > > > > Source/WebCore/page/Page.cpp:681 > > > > + chrome()->invalidateContentsAndRootView(IntRect(LayoutRect::infiniteRect()), false); > > > > > > if you go through FrameView::repaintContentRectangle or something that calls it you'll get the layout test tracking for free, since it's implemented at that layer. I think calling repaint() on the mainFrame()->contentRenderer() will do exactly what you want (although be sure to null-check it before dereferencing it), mind checking if that's the case? > > > > Nope, mainFrame()->contentRenderer()->repaint() doesn't work recursively. Thus it only repaints the RenderView -- which is a dummy element and its size is the layout viewport, but repaints nothing that overflows outside of the layout viewport. > > > > Lets use mainFrame()->view()->repaintContentRectangle()? Do I have to do null check for both mainFrame() and mainFrame()->view()? > > OK. mainFrame() will never be null (and it's used unguarded in this function) but its view may not be there. Sorry I'm wrong - a Frame's view() will never be null.
Created attachment 175105 [details] Patch
Comment on attachment 175105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175105&action=review > Source/WebCore/page/Page.cpp:681 > + mainFrame()->view()->repaintContentRectangle(IntRect(LayoutRect::infiniteRect()), false); what 'bout just calling mainFrame()->view()->invalidateRect(..); ? I realize this doesn't go through the FrameView repaint rect tracking code today, but that seems somewhat easier to fix when we add tests.
I think this is close but the friending is a bit ugly. I'm going on vacation but have cc'd a few folks who can hopefully help you finish this out.
(In reply to comment #24) > (From update of attachment 175105 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175105&action=review > > > Source/WebCore/page/Page.cpp:681 > > + mainFrame()->view()->repaintContentRectangle(IntRect(LayoutRect::infiniteRect()), false); > > what 'bout just calling mainFrame()->view()->invalidateRect(..); ? I realize this doesn't go through the FrameView repaint rect tracking code today, but that seems somewhat easier to fix when we add tests. Sounds good. Actually I considered invalidateRect() too but found it doesn't go through repaint tracking. If the you're planning to extend repaint tracking to invalidateRect(), I think it is exactly the function we want to use.
Created attachment 175117 [details] Patch
Adam, could you finish the review? This bugfix is a blocker for our dev release next week.
Why exactly can't this be tested? Is there at least a manual test case that can be run?
(In reply to comment #29) > Why exactly can't this be tested? Is there at least a manual test case that can be run? Okay it's actually not as difficult as I expected. Just managed to repro this in a layout test.
Created attachment 175294 [details] Patch
Created attachment 175297 [details] Patch
(In reply to comment #30) > (In reply to comment #29) > > Why exactly can't this be tested? Is there at least a manual test case that can be run? > > Okay it's actually not as difficult as I expected. Just managed to repro this in a layout test. Does it reproduce in all ports?
(In reply to comment #33) > (In reply to comment #30) > > (In reply to comment #29) > > > Why exactly can't this be tested? Is there at least a manual test case that can be run? > > > > Okay it's actually not as difficult as I expected. Just managed to repro this in a layout test. > > Does it reproduce in all ports? I haven't tried it other than Linux and Android, but I would guess so.
Comment on attachment 175297 [details] Patch Attachment 175297 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14934422 New failing tests: compositing/repaint/page-scale-repaint.html
Created attachment 175314 [details] Patch
Comment on attachment 175314 [details] Patch I'm not an expert on this code, but from the discussion it sounds like jamesr is happy with this approach and he's not able to complete the review because he's on vacation.
Comment on attachment 175314 [details] Patch Clearing flags on attachment: 175314 Committed r135439: <http://trac.webkit.org/changeset/135439>
All reviewed patches have been landed. Closing bug.
(In reply to comment #37) > (From update of attachment 175314 [details]) > I'm not an expert on this code, but from the discussion it sounds like jamesr is happy with this approach and he's not able to complete the review because he's on vacation. That's correct. Thanks for doing the review, Adam, and for persisting and adding a test, Tien-Ren!