Bug 65037 – [chromium] Accelerated drawing broken by mismatched compositor tile sizes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

Layer compositor currently use the smallest possible size for each tile instead of the same same size for all tiles. Accelerated drawing assumes that all tiles are of the same size.

[David Reveman]

Created attachment 101747 [details]
Patch

[Alok Priyadarshi]

This patch does fix accelerated-drawing path. Thank You!
I think we should try to push this for M14. It will make accelerated-drawing path functional.

I will leave it to enne and jamesr to make the final call.

[Brian Salomon]

(In reply to comment #2)
> This patch does fix accelerated-drawing path. Thank You!
> I think we should try to push this for M14. It will make accelerated-drawing path functional.

Wouldn't we also need to resolve the DEPTH_STENCIL attachment issue?

[Adrienne Walker]

Comment on attachment 101747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101747&action=review

Thanks for working on fixing this.  :)

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:203
>  IntRect LayerTilerChromium::tileTexRect(const Tile* tile) const
>  {
>      const int index = m_tilingData.tileIndex(tile->i(), tile->j());
> -    return m_tilingData.tileBoundsWithOuterBorder(index);
> +    IntRect texRect = m_tilingData.tileBoundsWithOuterBorder(index);
> +    texRect.setSize(m_tileSize);
> +    return texRect;
>  }

Is this function needed anymore? Can we remove it and just call tileLayerRect?

[Alok Priyadarshi]

(In reply to comment #3)
> (In reply to comment #2)
> > This patch does fix accelerated-drawing path. Thank You!
> > I think we should try to push this for M14. It will make accelerated-drawing path functional.
> 
> Wouldn't we also need to resolve the DEPTH_STENCIL attachment issue?

You are right.

jamesr: If we push this patch, I would request you to reconsider https://bugs.webkit.org/show_bug.cgi?id=64613. Brian is working on a long-term solution that will address your concerns.

[James Robinson]

Comment on attachment 101747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101747&action=review

> LayoutTests/platform/chromium/test_expectations.txt:3855
> +BUGWK65037 : compositing/color-matching/image-color-matching.html = IMAGE
> +BUGWK65037 : compositing/geometry/fixed-position.html = IMAGE
> +BUGWK65037 : compositing/geometry/vertical-scroll-composited.html = IMAGE
> +BUGWK65037 : compositing/overflow/fixed-position-ancestor-clip.html = IMAGE
> +BUGWK65037 : platform/chromium/compositing/huge-layer-rotated.html = IMAGE
> +BUGWK65037 : platform/chromium/compositing/tiny-layer-rotated.html = IMAGE

fyi: having these edits at the very end of test_expectations.txt means that you will almost certainly get merge conflicts.  i'd suggest moving them up to a random spot a few hundred lines off the end.  also, be careful that you don't introduce any duplicates (run new-run-webkit-tests --lint-test-files to check)

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:197
>  IntRect LayerTilerChromium::tileTexRect(const Tile* tile) const

this sure does look identical to tileLayerRect() to me, as enne noted. please unify

[David Reveman]

(In reply to comment #6)
> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:197
> >  IntRect LayerTilerChromium::tileTexRect(const Tile* tile) const
> 
> this sure does look identical to tileLayerRect() to me, as enne noted. please unify

I didn't do that just to keep this temporary fix minimal. This patch only makes sense until https://bugs.webkit.org/show_bug.cgi?id=64942 has landed so I'll wait until I know the outlook for that before I put any more work into this.

[David Reveman]

(In reply to comment #7)
> (In reply to comment #6)
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:197
> > >  IntRect LayerTilerChromium::tileTexRect(const Tile* tile) const
> > 
> > this sure does look identical to tileLayerRect() to me, as enne noted. please unify
> 
> I didn't do that just to keep this temporary fix minimal. This patch only makes sense until https://bugs.webkit.org/show_bug.cgi?id=64942 has landed so I'll wait until I know the outlook for that before I put any more work into this.

That was a confusing comment. I meant to say that I DID this to keep the fix minimal but that's irrelevant now as 64942 has landed.

Is this still an issue? It should have been resolved by 64942.

[Alok Priyadarshi]

Fixed in r92255