Bug 64338 - Bad filtering of edges on tiled layers
Summary: Bad filtering of edges on tiled layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 19:09 PDT by David Reveman
Modified: 2011-07-13 08:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.19 KB, patch)
2011-07-11 19:37 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (4.05 KB, patch)
2011-07-12 09:31 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (12.07 KB, patch)
2011-07-12 14:12 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (12.08 KB, patch)
2011-07-12 20:36 PDT, David Reveman
no flags 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-11 19:09:29 PDT
The transparent outer borders used for anti-aliasing of tiled layers cause filtering at the layer edges to sometimes produce incorrect results.
Comment 1 David Reveman 2011-07-11 19:37:44 PDT
Created attachment 100428 [details]
Patch
Comment 2 David Reveman 2011-07-12 09:31:31 PDT
Created attachment 100499 [details]
Patch
Comment 3 Kenneth Russell 2011-07-12 12:36:54 PDT
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.
Comment 4 Adrienne Walker 2011-07-12 12:48:00 PDT
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.
Comment 5 James Robinson 2011-07-12 12:52:28 PDT
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.
Comment 6 David Reveman 2011-07-12 14:12:41 PDT
Created attachment 100560 [details]
Patch
Comment 7 David Reveman 2011-07-12 14:14:46 PDT
(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.
Comment 8 David Reveman 2011-07-12 14:30:58 PDT
(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.
Comment 9 Adrienne Walker 2011-07-12 14:41:26 PDT
> (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
Comment 10 David Reveman 2011-07-12 14:55:44 PDT
(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.
Comment 11 James Robinson 2011-07-12 15:21:22 PDT
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.
Comment 12 James Robinson 2011-07-12 15:22:37 PDT
Do you want the latest patch considered for review?  If so please flip the review? flag.
Comment 13 David Reveman 2011-07-12 15:35:55 PDT
(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.
Comment 14 David Reveman 2011-07-12 15:48:57 PDT
(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.
Comment 15 James Robinson 2011-07-12 17:36:02 PDT
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.
Comment 16 David Reveman 2011-07-12 20:36:31 PDT
Created attachment 100616 [details]
Patch
Comment 17 James Robinson 2011-07-12 21:34:01 PDT
Comment on attachment 100616 [details]
Patch

R=me thanks
Comment 18 WebKit Review Bot 2011-07-12 22:35:28 PDT
Comment on attachment 100616 [details]
Patch

Clearing flags on attachment: 100616

Committed r90887: <http://trac.webkit.org/changeset/90887>
Comment 19 WebKit Review Bot 2011-07-12 22:35:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Stephen White 2011-07-13 06:50:28 PDT
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?
Comment 21 David Reveman 2011-07-13 07:12:58 PDT
(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.
Comment 22 Stephen White 2011-07-13 08:17:57 PDT
(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.
Comment 23 David Reveman 2011-07-13 08:48:17 PDT
(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.