Bug 64338 – Bad filtering of edges on tiled layers

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[David Reveman]

The transparent outer borders used for anti-aliasing of tiled layers cause filtering at the layer edges to sometimes produce incorrect results.

[David Reveman]

Created attachment 100428 [details]
Patch

[David Reveman]

Created attachment 100499 [details]
Patch

[Kenneth Russell]

Comment on attachment 100499 [details]
Patch

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

This basically looks fine aside from a naming convention issue.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1129
> +bool TransformationMatrix::IsIntegerTranslation() const

This should be named isIntegerTranslation per WebKit style.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:342
> +    bool IsIntegerTranslation() const;

Same here.

[Adrienne Walker]

My apologies, but I need some convincing that this does the right thing in all cases.  If we're seeing floating point error, wouldn't nearest neighbor filtering round *down* in some cases, changing the integral width of these layers?

I know that we're already living with some floating point inconsistencies (some of which are my fault), but I feel like a layer of integral width on integer boundaries should not be off by one pixel.  I would feel more comfortable if we had a test like compositing/checkerboard.html for content layers.

On the same note, I'd like if this patch also adjusted test_expectations.txt.  I'd like to see what tests are expected to still be broken after this patch.

[James Robinson]

Also, what happens if you have a layer that's rotated by 90 degress about the Z axis or something of that nature?  It would suck to incorrectly filter that case too.

[David Reveman]

Created attachment 100560 [details]
Patch

[David Reveman]

(In reply to comment #3)
> (From update of attachment 100499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100499&action=review
> 
> This basically looks fine aside from a naming convention issue.

Sorry, I should have noticed that. Fixed in the latest patch.

[David Reveman]

(In reply to comment #4)
> My apologies, but I need some convincing that this does the right thing in all cases.  If we're seeing floating point error, wouldn't nearest neighbor filtering round *down* in some cases, changing the integral width of these layers?
> 
> I know that we're already living with some floating point inconsistencies (some of which are my fault), but I feel like a layer of integral width on integer boundaries should not be off by one pixel.  I would feel more comfortable if we had a test like compositing/checkerboard.html for content layers.

We're not off by one pixel. The problem is due to the transparent border that now exists around layers. A small part of the border color is sometimes included in the pixels at the edge of the layer when linear filtering is used (instead of RGBA:0xff,0xff,0xff,0xff we end up with RGBA:0xfe,0xfe,0xfe,0xfe). The result here seem to be GL implementation dependent and it happens with or without the vertex offset code used to make sure all necessary fragments are processed.

> 
> On the same note, I'd like if this patch also adjusted test_expectations.txt.  I'd like to see what tests are expected to still be broken after this patch.

Done.

[Adrienne Walker]

> (In reply to comment #8)
> We're not off by one pixel. The problem is due to the transparent border that now exists around layers. A small part of the border color is sometimes included in the pixels at the edge of the layer when linear filtering is used (instead of RGBA:0xff,0xff,0xff,0xff we end up with RGBA:0xfe,0xfe,0xfe,0xfe). The result here seem to be GL implementation dependent and it happens with or without the vertex offset code used to make sure all necessary fragments are processed.

Ah, ok.  We're always smearing over the edge of a layer and never the other way.  Thanks for the extra info.

> > On the same note, I'd like if this patch also adjusted test_expectations.txt.  I'd like to see what tests are expected to still be broken after this patch.
> 
> Done.

Thanks.  That looks like a much more reasonable set of tests that need to be rebaselined.

The following content layer checkerboard (courtesy of jamesr) also looks like it renders properly when using nearest neighbor filtering, so I feel much more confident in this change.

http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cdiv%20id%3D%22checker%22%20style%3D%22-webkit-transform%3AtranslateZ(0)%3Bwidth%3A4000px%3B%20height%3A4000px%3B%20background-repeat%3A%20repeat%3B%20background-size%3A%202px%22%3E%3C%2Fdiv%3E%0A%3Cscript%3E%0Avar%20c%20%3D%20document.createElement('canvas')%3B%0Ac.width%3D2%3B%0Ac.height%3D2%3B%0Avar%20ctx%20%3D%20c.getContext(%222d%22)%3B%0Actx.fillStyle%20%3D%20%22black%22%3B%0Actx.fillRect(0%2C%200%2C%201%2C%201)%3B%0Actx.fillRect(1%2C%201%2C%201%2C%201)%3B%0Adocument.getElementById(%22checker%22).style.backgroundImage%20%3D%20%22url(%22%2Bc.toDataURL()%2B%22)%22%3B%0A%3C%2Fscript%3E

[David Reveman]

(In reply to comment #5)
> Also, what happens if you have a layer that's rotated by 90 degress about the Z axis or something of that nature?  It would suck to incorrectly filter that case too.

The patch could pretty easily be changed to handle this.

But what does incorrect filtering really mean here? GL implementations will always produce slightly different results for linear filtering. I'm not sure that putting a lot of effort into detecting special cases where we can get pixel exact 
matching between different renderers is necessarily the best way to go. It makes sense to get pixel exact results independent of renderer for un-transformed elements. But it might make more sense to determine whether we should be producing pixel exact results to depend on if a CSS 3D property is set or not instead of analyzing the transformation matrix we end up with in the layer compositor. E.g. setting translateZ to 0 makes the element 3D transformed and as a result output is also not guaranteed to be pixel exact anymore.

[James Robinson]

Comment on attachment 100560 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1138
> +    // Z translate
> +    if (m_matrix[3][2])
> +        return false;
> +
> +    // pixel exact X/Y translate

In WebKit, comments begin with an uppercase letter and end with a period.

[James Robinson]

Do you want the latest patch considered for review?  If so please flip the review? flag.

[David Reveman]

(In reply to comment #11)
> In WebKit, comments begin with an uppercase letter and end with a period.

Sorry, I'll eventually get all these details right. I promise :)

Are there any guidelines for how to prioritize webkit style vs being consistent with existing code in a file? Would correcting the style of unrelated code in a file as part of change be good or bad? Thanks.

[David Reveman]

(In reply to comment #12)
> Do you want the latest patch considered for review?  If so please flip the review? flag.

I figured we should agree on if this is the way to solve the problem before I had someone waste their time reviewing the patch again..

I think this patch is a good enough solution for now but long term we should consider what I mentioned in comment #10. It would be good to have a well defined basis for when we need to produce pixel exact results and not.

[James Robinson]

Comment on attachment 100560 [details]
Patch

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

I think approach is OK for now.  Please fix up the comments/line wrapping and post a new patch as soon as you can, currently we have lots of tests marked as = IMAGE and I'm really concerned that they might regression other, less acceptable ways.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1140
> +    if (static_cast<int>(m_matrix[3][0]) != m_matrix[3][0]
> +        || static_cast<int>(m_matrix[3][1]) != m_matrix[3][1])

nit: don't line wrap this, or if you do line wrap it make the !=s line up.

[David Reveman]

Created attachment 100616 [details]
Patch

[James Robinson]

Comment on attachment 100616 [details]
Patch

R=me thanks

[WebKit Review Bot]

Comment on attachment 100616 [details]
Patch

Clearing flags on attachment: 100616

Committed r90887: <http://trac.webkit.org/changeset/90887>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

Comment on attachment 100616 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:503
> +    GC3Dint filter = (m_tilingData.borderTexels() && !matrix.isIntegerTranslation()) ? GraphicsContext3D::LINEAR : GraphicsContext3D::NEAREST;

Post-landing drive-by:  if it's an integer translate, wouldn't it be better to simply skip the inflate step and extra geometry altogether?

[David Reveman]

(In reply to comment #20)
> (From update of attachment 100616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100616&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:503
> > +    GC3Dint filter = (m_tilingData.borderTexels() && !matrix.isIntegerTranslation()) ? GraphicsContext3D::LINEAR : GraphicsContext3D::NEAREST;
> 
> Post-landing drive-by:  if it's an integer translate, wouldn't it be better to simply skip the inflate step and extra geometry altogether?

It's not the inflate step that is causing the output difference. It's the existence of transparent border texels, which we can't do without unless we know that the layer will never be transformed. E.g. the root layer doesn't have this problem as it's created without these border texels.

[Stephen White]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 100616 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=100616&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:503
> > > +    GC3Dint filter = (m_tilingData.borderTexels() && !matrix.isIntegerTranslation()) ? GraphicsContext3D::LINEAR : GraphicsContext3D::NEAREST;
> > 
> > Post-landing drive-by:  if it's an integer translate, wouldn't it be better to simply skip the inflate step and extra geometry altogether?
> 
> It's not the inflate step that is causing the output difference. It's the existence of transparent border texels, which we can't do without unless we know that the layer will never be transformed. E.g. the root layer doesn't have this problem as it's created without these border texels.

I was thinking that you could avoid drawing the border texels, perhaps by adjusting the texture coordinates inward by a pixel to accommodate the non-inflated geometry.  Although with tiling, perhaps this special case might be more work than it's worth.

[David Reveman]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (From update of attachment 100616 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=100616&action=review
> > > 
> > > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:503
> > > > +    GC3Dint filter = (m_tilingData.borderTexels() && !matrix.isIntegerTranslation()) ? GraphicsContext3D::LINEAR : GraphicsContext3D::NEAREST;
> > > 
> > > Post-landing drive-by:  if it's an integer translate, wouldn't it be better to simply skip the inflate step and extra geometry altogether?
> > 
> > It's not the inflate step that is causing the output difference. It's the existence of transparent border texels, which we can't do without unless we know that the layer will never be transformed. E.g. the root layer doesn't have this problem as it's created without these border texels.
> 
> I was thinking that you could avoid drawing the border texels, perhaps by adjusting the texture coordinates inward by a pixel to accommodate the non-inflated geometry.  Although with tiling, perhaps this special case might be more work than it's worth.

Sorry for the confusion. What you're describing here is what I tried to explain in my previous comment that it isn't working. The existence of the border texels make a difference whether they are included in the texture coordinates or not. When creating the layer AA changes, this was the approach I first thought would be the best solution and my tests showing that it wasn't reliable is how I came to the conclusion that the nearest neighbor filter approach is better.