[Chromium] Issue 60965: Layout test fast/canvas/setWidthResetAfterForcedRender.html is failing on GPU path
Created attachment 83250 [details] Patch
James, Could you review my patch? I guess 2D canvas acceleration doesn't need "requiresLayer()". If it returns true, the layout is different from the non accelerated layout. Thanks,
Attachment 83250 [details] did not build on win: Build output: http://queues.webkit.org/results/7940821
Comment on attachment 83250 [details] Patch This is wrong, accelerated 2d canvases also require a layer. Why do you think this helps on that test?
Attachment 83250 [details] did not build on qt: Build output: http://queues.webkit.org/results/7938998
Attachment 83250 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7943179
James, Thank you for your review. As you know, requiresLayer() function is called bye RenderBoxModelObject::styleDidChange() RenderObject::updateFillImages() If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated. For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer? Still I'm wondering why the requiresLayer() can change rendering tree order. Thanks, (In reply to comment #4) > (From update of attachment 83250 [details]) > This is wrong, accelerated 2d canvases also require a layer. Why do you think this helps on that test?
James, Could you give me your thought? Thanks, (In reply to comment #7) > James, > > Thank you for your review. > > As you know, requiresLayer() function is called bye > RenderBoxModelObject::styleDidChange() > RenderObject::updateFillImages() > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated. > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer? > > Still I'm wondering why the requiresLayer() can change rendering tree order. > > Thanks, > > (In reply to comment #4) > > (From update of attachment 83250 [details] [details]) > > This is wrong, accelerated 2d canvases also require a layer. Why do you think this helps on that test?
(In reply to comment #7) > James, > > Thank you for your review. > > As you know, requiresLayer() function is called bye > RenderBoxModelObject::styleDidChange() > RenderObject::updateFillImages() > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated. Yes - this is expected. An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer. Different does not mean wrong. > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. I'm not sure what you mean by 'becomes the same layer of the root' - could you expand? > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer? The RenderLayer is required because accelerated canvas require their own composited layer. See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check. I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?
James, Thank you for your comment. I'll make sure Stephan's patch. Still I'm a beginner for WebKit, I have to study a lot... So I'm sorry if my patch is far from the correct fix;-) But thank you for your dedication every time. (In reply to comment #9) > (In reply to comment #7) > > James, > > > > Thank you for your review. > > > > As you know, requiresLayer() function is called bye > > RenderBoxModelObject::styleDidChange() > > RenderObject::updateFillImages() > > > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated. > > Yes - this is expected. An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer. Different does not mean wrong. > > > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. > > I'm not sure what you mean by 'becomes the same layer of the root' - could you expand? > > > > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer? > > The RenderLayer is required because accelerated canvas require their own composited layer. See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check. > > I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?
James, I confirmed Stephen's fix and it looks Ok with Chrome. But it doesn't with DumpRenderTree. As you know, we can run the test like following, ./webkit/tools/layout_tests/run_webkit_tests.sh --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html It fails as following, RenderText {#text} at (0,0) size 176x19 text run at (0,0) width 176: "Actual output: (blank canvas)" RenderBlock (anonymous) at (0,152) size 784x55 - RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 RenderText {#text} at (0,0) size 0x0 RenderText {#text} at (0,0) size 0x0 +layer at (8,168) size 100x50 + RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 As you see, rendering tree is different. That's why I said > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. If we have to fix completely, we have to consider DumpRenderTree part, right? What do you think? (In reply to comment #10) > James, > > Thank you for your comment. I'll make sure Stephan's patch. Still I'm a beginner for WebKit, I have to study a lot... So I'm sorry if my patch is far from the correct fix;-) > > But thank you for your dedication every time. > > (In reply to comment #9) > > (In reply to comment #7) > > > James, > > > > > > Thank you for your review. > > > > > > As you know, requiresLayer() function is called bye > > > RenderBoxModelObject::styleDidChange() > > > RenderObject::updateFillImages() > > > > > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated. > > > > Yes - this is expected. An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer. Different does not mean wrong. > > > > > > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. > > > > I'm not sure what you mean by 'becomes the same layer of the root' - could you expand? > > > > > > > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer? > > > > The RenderLayer is required because accelerated canvas require their own composited layer. See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check. > > > > I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?
James, Could you give me your comment? Thanks, (In reply to comment #11) > James, > > I confirmed Stephen's fix and it looks Ok with Chrome. > > But it doesn't with DumpRenderTree. > > As you know, we can run the test like following, > ./webkit/tools/layout_tests/run_webkit_tests.sh --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html > > It fails as following, > RenderText {#text} at (0,0) size 176x19 > text run at (0,0) width 176: "Actual output: (blank canvas)" > RenderBlock (anonymous) at (0,152) size 784x55 > - RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 > RenderText {#text} at (0,0) size 0x0 > RenderText {#text} at (0,0) size 0x0 > +layer at (8,168) size 100x50 > + RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 > > As you see, rendering tree is different. > > That's why I said > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. > > If we have to fix completely, we have to consider DumpRenderTree part, right? > > What do you think? > > (In reply to comment #10) > > James, > > > > Thank you for your comment. I'll make sure Stephan's patch. Still I'm a beginner for WebKit, I have to study a lot... So I'm sorry if my patch is far from the correct fix;-) > > > > But thank you for your dedication every time. > > > > (In reply to comment #9) > > > (In reply to comment #7) > > > > James, > > > > > > > > Thank you for your review. > > > > > > > > As you know, requiresLayer() function is called bye > > > > RenderBoxModelObject::styleDidChange() > > > > RenderObject::updateFillImages() > > > > > > > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated. > > > > > > Yes - this is expected. An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer. Different does not mean wrong. > > > > > > > > > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. > > > > > > I'm not sure what you mean by 'becomes the same layer of the root' - could you expand? > > > > > > > > > > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer? > > > > > > The RenderLayer is required because accelerated canvas require their own composited layer. See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check. > > > > > > I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?
James, Could you give me your comment? (In reply to comment #12) > James, > > Could you give me your comment? > > Thanks, > > (In reply to comment #11) > > James, > > > > I confirmed Stephen's fix and it looks Ok with Chrome. > > > > But it doesn't with DumpRenderTree. > > > > As you know, we can run the test like following, > > ./webkit/tools/layout_tests/run_webkit_tests.sh --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html > > > > It fails as following, > > RenderText {#text} at (0,0) size 176x19 > > text run at (0,0) width 176: "Actual output: (blank canvas)" > > RenderBlock (anonymous) at (0,152) size 784x55 > > - RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 > > RenderText {#text} at (0,0) size 0x0 > > RenderText {#text} at (0,0) size 0x0 > > +layer at (8,168) size 100x50 > > + RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 > > > > As you see, rendering tree is different. > > > > That's why I said > > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. > > > > If we have to fix completely, we have to consider DumpRenderTree part, right? > > > > What do you think? > > > > (In reply to comment #10) > > > James, > > > > > > Thank you for your comment. I'll make sure Stephan's patch. Still I'm a beginner for WebKit, I have to study a lot... So I'm sorry if my patch is far from the correct fix;-) > > > > > > But thank you for your dedication every time. > > > > > > (In reply to comment #9) > > > > (In reply to comment #7) > > > > > James, > > > > > > > > > > Thank you for your review. > > > > > > > > > > As you know, requiresLayer() function is called bye > > > > > RenderBoxModelObject::styleDidChange() > > > > > RenderObject::updateFillImages() > > > > > > > > > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated. > > > > > > > > Yes - this is expected. An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer. Different does not mean wrong. > > > > > > > > > > > > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part. > > > > > > > > I'm not sure what you mean by 'becomes the same layer of the root' - could you expand? > > > > > > > > > > > > > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer? > > > > > > > > The RenderLayer is required because accelerated canvas require their own composited layer. See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check. > > > > > > > > I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?
(In reply to comment #11) > James, > > I confirmed Stephen's fix and it looks Ok with Chrome. > > But it doesn't with DumpRenderTree. > > As you know, we can run the test like following, > ./webkit/tools/layout_tests/run_webkit_tests.sh --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html > > It fails as following, > RenderText {#text} at (0,0) size 176x19 > text run at (0,0) width 176: "Actual output: (blank canvas)" > RenderBlock (anonymous) at (0,152) size 784x55 > - RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 > RenderText {#text} at (0,0) size 0x0 > RenderText {#text} at (0,0) size 0x0 > +layer at (8,168) size 100x50 > + RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 > > As you see, rendering tree is different. Yes. The new render tree is correct when accelerated canvas is enabled as I said in comment #9.
I see, so we have to change test patterns. Because even after the change, still the test says "Error" in setWidthResetAfterForcedRender.html. (In reply to comment #14) > (In reply to comment #11) > > James, > > > > I confirmed Stephen's fix and it looks Ok with Chrome. > > > > But it doesn't with DumpRenderTree. > > > > As you know, we can run the test like following, > > ./webkit/tools/layout_tests/run_webkit_tests.sh --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html > > > > It fails as following, > > RenderText {#text} at (0,0) size 176x19 > > text run at (0,0) width 176: "Actual output: (blank canvas)" > > RenderBlock (anonymous) at (0,152) size 784x55 > > - RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 > > RenderText {#text} at (0,0) size 0x0 > > RenderText {#text} at (0,0) size 0x0 > > +layer at (8,168) size 100x50 > > + RenderHTMLCanvas {CANVAS} at (0,0) size 100x50 > > > > As you see, rendering tree is different. > > Yes. The new render tree is correct when accelerated canvas is enabled as I said in comment #9.