Bug 55679 – [chromium] Regression: r80081

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jonathan Backer]

Affects all platforms (Windows requires --use-gl=desktop to repro): enable accelerated compositing by moving the z slider on URL above. Then select some text on the rotated layer. This causes the layer to display some corrupted graphics. I built Chrome by hand and narrowed it down to the specific commit.

http://trac.webkit.org/changeset/80081

[Adrienne Walker]

Thanks for taking the time to narrow this down to a single commit.  I'll take a look.

[Adrienne Walker]

This looks like it's occurring on large layers.  Probably just a mismatched stride somewhere.

[Adrienne Walker]

Created attachment 84583 [details]
Patch

[James Robinson]

Comment on attachment 84583 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:215
>      const uint8_t* uploadPixels = pixels + srcStride * m_uploadUpdateRect.x();

this line is also clearly wrong (multiplying a stride by an x offset makes no sense).  would you mind changing this as well?

can you take a shot at making a test?  clearly the tests in LayoutTests/compositing/ are insufficient to catch this error, since they all pass, and since we're planning to rewrite this code again I would really like to have some tests that exercise this code a bit more.

[Adrienne Walker]

(In reply to comment #4)
> (From update of attachment 84583 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84583&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:215
> >      const uint8_t* uploadPixels = pixels + srcStride * m_uploadUpdateRect.x();
> 
> this line is also clearly wrong (multiplying a stride by an x offset makes no sense).  would you mind changing this as well?
> 
> can you take a shot at making a test?  clearly the tests in LayoutTests/compositing/ are insufficient to catch this error, since they all pass, and since we're planning to rewrite this code again I would really like to have some tests that exercise this code a bit more.

We do have a large layer test for images, but it's a solid color, so a stride difference is a no-op.  I could just add a gradient and rebaseline.

[James Robinson]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 84583 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=84583&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:215
> > >      const uint8_t* uploadPixels = pixels + srcStride * m_uploadUpdateRect.x();
> > 
> > this line is also clearly wrong (multiplying a stride by an x offset makes no sense).  would you mind changing this as well?
> > 
> > can you take a shot at making a test?  clearly the tests in LayoutTests/compositing/ are insufficient to catch this error, since they all pass, and since we're planning to rewrite this code again I would really like to have some tests that exercise this code a bit more.
> 
> We do have a large layer test for images, but it's a solid color, so a stride difference is a no-op.  I could just add a gradient and rebaseline.

D'oh!  Yeah having some sort of pattern or gradient or something would be awesome so we can tell if we are uploading garbage.  Either augmenting the existing test or just adding a new one sound great to me.

[James Robinson]

Comment on attachment 84583 [details]
Patch

I promise to R+ once we have test coverage :)

[Adrienne Walker]

As it turns out the huge-layer-img.html test would have caught this had it been a pixel test.  Of course, that uncovered a second bug (yay testing) where the image layer is replaced by a scrollbar texture (oops), so I'll fix that too before I upload another patch.

[James Robinson]

I've confirmed locally that this fixes the stride issue on https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/webgl-conformance-tests.html as well (to repro, click 'run' on a test such as conformance/buffer-preserve-test.html and select some text in the iframe in the top left.  It's a large layer and looks pretty jacked up without this patch).

[Adrienne Walker]

Created attachment 84791 [details]
Patch

[Adrienne Walker]

(In reply to comment #10)
> Created an attachment (id=84791) [details]
> Patch

I added one image for this test, but it's going to fail on a bunch of platforms because it was previously text-only.  What's the best way to handle this? Is it ok to land and immediately fix the tests that I know will be broken?

It's a little unclear for me how to sort out which platforms even run this test, but another option might be to change test_expectations to fail IMAGE for all the rest as a part of this patch?

[James Robinson]

We chatted in person.  I think the best approach is to make a new copy of this test that does not have visible text and has overflow:hidden on the <body> to hide the scrollbars.

[Adrienne Walker]

Created attachment 84798 [details]
Patch

[James Robinson]

Comment on attachment 84798 [details]
Patch

R=me.

[Adrienne Walker]

Committed r80384: <http://trac.webkit.org/changeset/80384>