Bug 45400 – chromium/mac: let image layer data row order match skia

• Component: New Bugs
• Status: RESOLVED FIXED

[Nico Weber]

chromium/mac: let image layer data row order match skia

[Nico Weber]

Created attachment 66915 [details]
Patch

[Nico Weber]

With this, images are no longer upside down when using the compositor with chrome mac.

This is http://crbug.com/54834

[David Levin]

What layout test does this fix?

If none, please add one.

[David Levin]

Comment on attachment 66915 [details]
Patch

See comment in bug. (If this fixes an existing layout test, add the info to the bug.)

[Nico Weber]

This happens on a lot of images (every image with a css transform i believe), so I'm pretty sure this is covered by existing layout tests. The problem is that we don't run layout tests with compositing enabled yet, which is why this wasn't caught by tests.

[David Levin]

(In reply to comment #5)
> This happens on a lot of images (every image with a css transform i believe), so I'm pretty sure this is covered by existing layout tests. The problem is that we don't run layout tests with compositing enabled yet, which is why this wasn't caught by tests.

Pls say something simple in the ChangeLog (and I'll gladly r+ it).

Something like
"Test: This will be caught by layout tests that have images with a css transform when the compositor is turned on for layout tests."

[Kenneth Russell]

There is already Core Graphics-specific logic in ContentLayerChromium (see ContentLayerChromium::SharedValues::SharedValues(), fragmentShaderString) which performs a vertical flip. The origin difference between CG and Skia also affects scrolling; see LayerRendererChromium::drawLayers. Have these been taken into consideration with this patch?

[Nico Weber]

Created attachment 66934 [details]
Patch

[Nico Weber]

(In reply to comment #7)
> There is already Core Graphics-specific logic in ContentLayerChromium (see ContentLayerChromium::SharedValues::SharedValues(), fragmentShaderString) which performs a vertical flip. The origin difference between CG and Skia also affects scrolling; see LayerRendererChromium::drawLayers. Have these been taken into consideration with this patch?

It hasn't. I just noticed that stuff looked wrong, and with this patch it looks right. After looking at things, it seems to be much cleaner to have the in-memory representation for skia and cg be identical and then remove all these tweaks (like we do in the software path). I've uploaded a revised patch that does this.

[Nico Weber]

Comment on attachment 66934 [details]
Patch

Please wait with reviewing this in detail.

[Nico Weber]

Comment on attachment 66934 [details]
Patch

I'm not sure why this is not necessary for ImageLayerChromium too, but ImageCG.cpp calls     CGContextScaleCTM(context, 1, -1) as well, so I suppose CGImageRefs are already flipped for some reason.

This change is ready for review.

[Kenneth Russell]

Comment on attachment 66934 [details]
Patch

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

Thanks for cleaning this up. Would appreciate it if you could clean up two very minor things before commit.

> WebCore/ChangeLog:20
> +        (WebCore::ImageLayerChromium::updateContents):
Could you regenerate the ChangeLog? ImageLayerChromium.cpp isn't touched in the diffs below.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-87
> -        "  vec4 texColor = texture2D(s_texture, vec2(v_texCoord.x, 1.0 - v_texCoord.y)); \n"
Could you please clean up the comment at the top of the fragmentShaderString too, in particular the reference to the differences in origin?

[Nico Weber]

Created attachment 67500 [details]
Patch

[Kenneth Russell]

Comment on attachment 67500 [details]
Patch

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

Looks good to me. The commit queue might reject this because you cq+'d it before it was r+'d; if that happens just cq+ it again.

[WebKit Commit Bot]

Comment on attachment 67500 [details]
Patch

Clearing flags on attachment: 67500

Committed r67493: <http://trac.webkit.org/changeset/67493>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.