Bug 65037 - [chromium] Accelerated drawing broken by mismatched compositor tile sizes
Summary: [chromium] Accelerated drawing broken by mismatched compositor tile sizes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on: 64613 64942
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-22 11:40 PDT by David Reveman
Modified: 2011-09-21 12:55 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.72 KB, patch)
2011-07-22 11:47 PDT, David Reveman
jamesr: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2011-07-22 11:40:28 PDT
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.
Comment 1 David Reveman 2011-07-22 11:47:12 PDT
Created attachment 101747 [details]
Patch
Comment 2 Alok Priyadarshi 2011-07-22 12:32:52 PDT
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.
Comment 3 Brian Salomon 2011-07-22 12:36:26 PDT
(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?
Comment 4 Adrienne Walker 2011-07-22 12:37:53 PDT
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?
Comment 5 Alok Priyadarshi 2011-07-22 12:42:49 PDT
(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.
Comment 6 James Robinson 2011-07-25 17:02:11 PDT
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
Comment 7 David Reveman 2011-07-26 10:48:02 PDT
(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.
Comment 8 David Reveman 2011-08-26 11:51:32 PDT
(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.
Comment 9 Alok Priyadarshi 2011-09-21 12:55:27 PDT
Fixed in r92255