Bug 64942 – [chromium] incorrect 3D CSS anti-aliasing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

We should use an edge distance based method instead of the current linear filtering method for producing anti-aliased edges on 3D transformed layers.

[David Reveman]

Created attachment 101677 [details]
Patch

[David Reveman]

Created attachment 101732 [details]
Patch

[David Reveman]

This patch is not ready to land yet but some feedback on the current version would be appreciated.

It affects more layout tests than the ones added to test_exceptions.txt. I've only added the ones that will necessarily change. 

I haven't moved any of the edge intersection code into the vertex shader as I'm not sure how to do that without increasing the number of uniforms.

The fragment shader is relatively simple but it's probably worth using a different shader in the case where we don't have to produce anti-aliased edges. Current patch doesn't do this.

The same tile size is now used for all tiles in a layer. I'm clamping texture coordinates in the fragment shader to make sure that we're not sampling outside the contents region of a tile.

[Brian Salomon]

Meta-comment: The layer update current code requires each tile in accelerated drawing to be the same size in order to share a stencil buffer via a single DrawingBuffer. In the M15 timeframe this won't be a requirement at all. Skia will create stencil-buffers on demand and mixed tiles won't be a problem. Is it worth doing significant changes to make accelerated drawing work for M14? Another option is to disable the flag for M14.

[Alok Priyadarshi]

(In reply to comment #4)
> Meta-comment: The layer update current code requires each tile in accelerated drawing to be the same size in order to share a stencil buffer via a single DrawingBuffer. In the M15 timeframe this won't be a requirement at all. Skia will create stencil-buffers on demand and mixed tiles won't be a problem. 

Is this still true for pure GLES2 implementation? I thought gles2 requires all buffers in an FBO to be of the same size.

[Brian Salomon]

That is true, but if skia is managing the buffers it will see the edge tiles' sizes are different, create a stencil for them, and put it in its cache.

[Alok Priyadarshi]

(In reply to comment #6)
> That is true, but if skia is managing the buffers it will see the edge tiles' sizes are different, create a stencil for them, and put it in its cache.

Which means skia will have to create fewer stencil buffers if texture sizes are same.

[Brian Salomon]

(In reply to comment #7)
> (In reply to comment #6)
> > That is true, but if skia is managing the buffers it will see the edge tiles' sizes are different, create a stencil for them, and put it in its cache.
> 
> Which means skia will have to create fewer stencil buffers if texture sizes are same.

Yes. I assume the top/left tiles aligned so worst case would be 4 stencils (full size, right edge, bottom edge, and bottom/right corner). I also hope we are able to use the more relaxed fbo completeness rules in chrome on desktop hw.

[Adrienne Walker]

I will also point out that the tiler has some code to try to reuse tile textures.  If tiles are not going to be the same size, that code should be stripped out.

My personal feeling is that we should make all tiles the same size, because it makes life simpler and we will (likely) have fewer floating point inconsistencies in testing for tests where texture coordinates (1/texture size) cannot be represented exactly for some texture sizes.

[James Robinson]

(In reply to comment #4)
> Meta-comment: The layer update current code requires each tile in accelerated drawing to be the same size in order to share a stencil buffer via a single DrawingBuffer. In the M15 timeframe this won't be a requirement at all. Skia will create stencil-buffers on demand and mixed tiles won't be a problem. Is it worth doing significant changes to make accelerated drawing work for M14? Another option is to disable the flag for M14.

I don't think --accelerated-drawing is a priority for M14, so don't worry about it too much in that context.

[David Reveman]

Created attachment 101945 [details]
Patch

[David Reveman]

Created attachment 102020 [details]
Patch

[David Reveman]

I'm comfortable landing the current patch. It adds a few instructions to the fragment shader used for all tile rendering which I don't think will have any performance impact but without a good set of performance tests it's hard to know.

[Vangelis Kokkevis]

(In reply to comment #13)
> I'm comfortable landing the current patch. It adds a few instructions to the fragment shader used for all tile rendering which I don't think will have any performance impact but without a good set of performance tests it's hard to know.

Have we measured the performance impact of edge-AA on lower end devices (e.g. on CrOS devices).  Is it something we want to enable on demand?

[David Reveman]

(In reply to comment #14)
> (In reply to comment #13)
> > I'm comfortable landing the current patch. It adds a few instructions to the fragment shader used for all tile rendering which I don't think will have any performance impact but without a good set of performance tests it's hard to know.
> 
> Have we measured the performance impact of edge-AA on lower end devices (e.g. on CrOS devices).  Is it something we want to enable on demand?

I don't think this is something we want to enable on a per device bases. If it has a measurable performance impact it shouldn't be significant enough that we would consider aliased output.

My only concern is that this anti-aliasing code is currently used for all tiled layers. Including the root layer, which obviously doesn't need anti-aliasing. I like using the same code path for everything but there might be cases where this has a small negative performance impact. So far I haven't found a page where this is the case on any of my cros devices.

[Vangelis Kokkevis]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I'm comfortable landing the current patch. It adds a few instructions to the fragment shader used for all tile rendering which I don't think will have any performance impact but without a good set of performance tests it's hard to know.
> > 
> > Have we measured the performance impact of edge-AA on lower end devices (e.g. on CrOS devices).  Is it something we want to enable on demand?
> 
> I don't think this is something we want to enable on a per device bases. If it has a measurable performance impact it shouldn't be significant enough that we would consider aliased output.
> 
> My only concern is that this anti-aliasing code is currently used for all tiled layers. Including the root layer, which obviously doesn't need anti-aliasing. I like using the same code path for everything but there might be cases where this has a small negative performance impact. So far I haven't found a page where this is the case on any of my cros devices.

The fact that we're using it for all tiled layers including the root makes me think that we'd want to have an option to turn it off. There are devices that we want to squeeze as much performance out of them, or have weak (read:non-existent) vector units, or possibly the resolution is such that AA wouldn't matter.  Would it be hard to also provide the old shaders and drawing code alongside the new ones?

[David Reveman]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > I'm comfortable landing the current patch. It adds a few instructions to the fragment shader used for all tile rendering which I don't think will have any performance impact but without a good set of performance tests it's hard to know.
> > > 
> > > Have we measured the performance impact of edge-AA on lower end devices (e.g. on CrOS devices).  Is it something we want to enable on demand?
> > 
> > I don't think this is something we want to enable on a per device bases. If it has a measurable performance impact it shouldn't be significant enough that we would consider aliased output.
> > 
> > My only concern is that this anti-aliasing code is currently used for all tiled layers. Including the root layer, which obviously doesn't need anti-aliasing. I like using the same code path for everything but there might be cases where this has a small negative performance impact. So far I haven't found a page where this is the case on any of my cros devices.
> 
> The fact that we're using it for all tiled layers including the root makes me think that we'd want to have an option to turn it off. There are devices that we want to squeeze as much performance out of them, or have weak (read:non-existent) vector units, or possibly the resolution is such that AA wouldn't matter.  Would it be hard to also provide the old shaders and drawing code alongside the new ones?

No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.

[Adrienne Walker]

(In reply to comment #17)

> No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.

Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.

[David Reveman]

(In reply to comment #18)
> (In reply to comment #17)
> 
> > No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.
> 
> Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.

Sure, turning off linear filtering would be more for consistency than performance. If the edges are aliased, I prefer the interior to be aliased too.

[Vangelis Kokkevis]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > 
> > > No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.
> > 
> > Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.
> 
> Sure, turning off linear filtering would be more for consistency than performance. If the edges are aliased, I prefer the interior to be aliased too.

There's a lot of cases where the composited layer's edges aren't really visible.  For example if the background color is the same as that of the target surface or it's transparent.

I would prefer to keep filtering on always and just have the option to turn of edge AA.

[James Robinson]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > 
> > > No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.
> > 
> > Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.
> 
> Sure, turning off linear filtering would be more for consistency than performance. If the edges are aliased, I prefer the interior to be aliased too.

We have to be able to scale without antialiasing the edges.

[David Reveman]

Created attachment 102296 [details]
Patch

[David Reveman]

New patch only uses the more complicated anti-aliasing shader when necessary. We would need a different fragment shader to do aliased edges on linear filtered transformed tiles as we would otherwise incorrectly sample outside the layer region of tile textures. But that's a relatively small change for us to do when we need that functionality.

[David Reveman]

(In reply to comment #21)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > 
> > > > No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.
> > > 
> > > Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.
> > 
> > Sure, turning off linear filtering would be more for consistency than performance. If the edges are aliased, I prefer the interior to be aliased too.
> 
> We have to be able to scale without antialiasing the edges.

Are you saying we need that for a different reason than performance? Please provide some more details.

[James Robinson]

(In reply to comment #24)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > (In reply to comment #18)
> > > > (In reply to comment #17)
> > > > 
> > > > > No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.
> > > > 
> > > > Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.
> > > 
> > > Sure, turning off linear filtering would be more for consistency than performance. If the edges are aliased, I prefer the interior to be aliased too.
> > 
> > We have to be able to scale without antialiasing the edges.
> 
> Are you saying we need that for a different reason than performance? Please provide some more details.

As Vangelis said, sometimes the edges aren't visible (for example with the root layer).  If we want to disable edge AA for those layers when scaling, then we obviously still need linear filtering.

[David Reveman]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #21)
> > > (In reply to comment #19)
> > > > (In reply to comment #18)
> > > > > (In reply to comment #17)
> > > > > 
> > > > > > No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.
> > > > > 
> > > > > Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.
> > > > 
> > > > Sure, turning off linear filtering would be more for consistency than performance. If the edges are aliased, I prefer the interior to be aliased too.
> > > 
> > > We have to be able to scale without antialiasing the edges.
> > 
> > Are you saying we need that for a different reason than performance? Please provide some more details.
> 
> As Vangelis said, sometimes the edges aren't visible (for example with the root layer).  If we want to disable edge AA for those layers when scaling, then we obviously still need linear filtering.

Ah, yes if we have a non-aa shader, we can use it in all cases where it produces the same output as the anti-aliasing shader. Current patch does that for integer translation but it can easily be made to use the non-aa shader for special case scaling too. However, this is all just for performance. We don't *have* to be able to scale without anti-aliasing.

[Vangelis Kokkevis]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #21)
> > > > (In reply to comment #19)
> > > > > (In reply to comment #18)
> > > > > > (In reply to comment #17)
> > > > > > 
> > > > > > > No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.
> > > > > > 
> > > > > > Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.
> > > > > 
> > > > > Sure, turning off linear filtering would be more for consistency than performance. If the edges are aliased, I prefer the interior to be aliased too.
> > > > 
> > > > We have to be able to scale without antialiasing the edges.
> > > 
> > > Are you saying we need that for a different reason than performance? Please provide some more details.
> > 
> > As Vangelis said, sometimes the edges aren't visible (for example with the root layer).  If we want to disable edge AA for those layers when scaling, then we obviously still need linear filtering.
> 
> Ah, yes if we have a non-aa shader, we can use it in all cases where it produces the same output as the anti-aliasing shader. Current patch does that for integer translation but it can easily be made to use the non-aa shader for special case scaling too. However, this is all just for performance. We don't *have* to be able to scale without anti-aliasing.

There's the case where edge AA won't make a difference (e.g. axis aligned layers) but also there could be platforms where edge AA is either not required or the speed vs quality tradeoffs are don't warrant it.

[David Reveman]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (In reply to comment #24)
> > > > (In reply to comment #21)
> > > > > (In reply to comment #19)
> > > > > > (In reply to comment #18)
> > > > > > > (In reply to comment #17)
> > > > > > > 
> > > > > > > > No, not hard. If there's cases like these where we want to turn off AA we probably also want to turn off linear filtering.
> > > > > > > 
> > > > > > > Not necessarily? On a lot of hardware, bilinear filtering is as free as nearest neighbor.
> > > > > > 
> > > > > > Sure, turning off linear filtering would be more for consistency than performance. If the edges are aliased, I prefer the interior to be aliased too.
> > > > > 
> > > > > We have to be able to scale without antialiasing the edges.
> > > > 
> > > > Are you saying we need that for a different reason than performance? Please provide some more details.
> > > 
> > > As Vangelis said, sometimes the edges aren't visible (for example with the root layer).  If we want to disable edge AA for those layers when scaling, then we obviously still need linear filtering.
> > 
> > Ah, yes if we have a non-aa shader, we can use it in all cases where it produces the same output as the anti-aliasing shader. Current patch does that for integer translation but it can easily be made to use the non-aa shader for special case scaling too. However, this is all just for performance. We don't *have* to be able to scale without anti-aliasing.
> 
> There's the case where edge AA won't make a difference (e.g. axis aligned layers) but also there could be platforms where edge AA is either not required or the speed vs quality tradeoffs are don't warrant it.

Linear filtering and non AA edges requires an additional shader that does texture coordinate clamping but no edge distance calculations. The current patch makes it easy to add this at a later stage.

[Adrienne Walker]

Comment on attachment 102296 [details]
Patch

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

Is there a reason that we're doing so much complicated CPU-side work to keep track of edge positions? It still seems like we're doing the work of a vertex shader on the CPU.  If we're pushing work to the fragment shader, why not also to the vertex shader? Thinking about this naively, it would also mean that you wouldn't have to duplicate all the edge calculation and geometry generation code from LayerTilerChromium for render surfaces.

This approach would also make the draw function less complicated.  It would also make it less brittle in terms of keeping track of scale flipping and previous edges when skipping tiles.  I'm partially concerned here because the brittleness of this function's iteration order (every tile, top to bottom, left to right, in content space) caused a lot of pain for me in getting the RTL patch working.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:401
> +    // Use anti-aliasing programs when border texels are preset and transform
> +    // is not an integer translation.
> +    bool useAA = (m_tilingData.borderTexels() && !globalTransform.isIntegerTranslation());

Aren't integer scales also ok?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:466
> +    IntRect layerRect = contentRectToLayerRect(contentRect);
> +    IntRect bounds(layerRect);
> +    bounds.move(m_layerPosition.x(), m_layerPosition.y());

Nit: use bounds = contentRect.size() here?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:513
> +            // Skip empty rows.
> +            if (tileRect.isEmpty())
> +                continue;

When does this occur? I would expect that we'd only be iterating through tiles intersecting layerRect.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:544
> +            IntRect displayRect = tileRect;
> +            tileRect.intersect(layerRect);

tileRect is in content space at this point.  You should intersect by the content space rect of the tile and not the layer space one.  This doesn't make a difference now because no layers actually use translations, but it will when I land my RTL layer patch.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:552
> +            // Skip empty tiles.
> +            if (tileRect.isEmpty())
> +                continue;

Don't you need to be setting previous edges here?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:562
> +            FloatRect clampRect(tileRect);
> +            // Clamp texture coordinates to avoid sampling outside the layer.
> +            const float epsilon = 1 / 1024.0f;
> +            float clampX = min(0.5, clampRect.width() / 2.0 - epsilon);
> +            float clampY = min(0.5, clampRect.height() / 2.0 - epsilon);
> +            clampRect.inflateX(-clampX);
> +            clampRect.inflateY(-clampY);

Forgive me for asking, but I don't follow this.  The comment says "sampling outside the layer", but you're clamping to the tile rect in content space.  We already set CLAMP_TO_EDGE for all textures requested from the TextureManager, so I'm unsure of what this is needed for.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:579
> +            // Map clamping rectangle to unit square.
> +            float vsTexTranslateX = -clampRect.x() / clampRect.width();
> +            float vsTexTranslateY = -clampRect.y() / clampRect.height();
> +            float vsTexScaleX = tileRect.width() / clampRect.width();
> +            float vsTexScaleY = tileRect.height() / clampRect.height();
> +
> +            // Map to normalized texture coordinates.
> +            float fsTexTranslateX = texOffset.x() / tileWidth;
> +            float fsTexTranslateY = texOffset.y() / tileHeight;
> +            float fsTexScaleX = clampRect.width() / tileWidth;
> +            float fsTexScaleY = clampRect.height() / tileHeight;

Style nit: don't abbreviate vs and fs here.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:634
> +                GLC(context, context->uniform4fv(edgeLocation, edge, 4));

Can't we just pass the layer edges as a uniform once instead of the tile edges for every draw call? FragmentShaderRGBATexAlphaAA looks like it converts fragment position into viewport space and then compares against tile edges.  Would it be incorrect to compare against layer edges there?

[David Reveman]

(In reply to comment #29)
> (From update of attachment 102296 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102296&action=review
> 
> Is there a reason that we're doing so much complicated CPU-side work to keep track of edge positions? It still seems like we're doing the work of a vertex shader on the CPU.  If we're pushing work to the fragment shader, why not also to the vertex shader? Thinking about this naively, it would also mean that you wouldn't have to duplicate all the edge calculation and geometry generation code from LayerTilerChromium for render surfaces.

The main reason for doing this on the CPU-side right now is that I didn't want this first patch to get too large. My plan is to move what makes sense to the vertex shader after this first patch has landed.

It's also not obvious how this CPU-side work is efficiently moved to the shader but I was hoping we could have that discussion after this has landed. The CPU-side work done in this patch is not different from what's done currently in trunk so we can land this without having to worry about any performance regressions related to it.

For render surface support, we would either move things to the vertex shader or re-factor some of this to make sure we don't duplicate code.

> 
> This approach would also make the draw function less complicated.  It would also make it less brittle in terms of keeping track of scale flipping and previous edges when skipping tiles.  I'm partially concerned here because the brittleness of this function's iteration order (every tile, top to bottom, left to right, in content space) caused a lot of pain for me in getting the RTL patch working.

Well, it's an optimization. If we want to sacrifice some performance for making it less complicated, that's easy to do. The reason we have the current code is that people were pushing for these kind of optimization during the review process for the previous anti-aliasing patch. It will change if we move things to the vertex shader...

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:401
> > +    // Use anti-aliasing programs when border texels are preset and transform
> > +    // is not an integer translation.
> > +    bool useAA = (m_tilingData.borderTexels() && !globalTransform.isIntegerTranslation());
> 
> Aren't integer scales also ok?

No, they need fragment shader clamping of texture coordinates to avoid sampling outside the contents area of tiles.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:466
> > +    IntRect layerRect = contentRectToLayerRect(contentRect);
> > +    IntRect bounds(layerRect);
> > +    bounds.move(m_layerPosition.x(), m_layerPosition.y());
> 
> Nit: use bounds = contentRect.size() here?

Ok.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:513
> > +            // Skip empty rows.
> > +            if (tileRect.isEmpty())
> > +                continue;
> 
> When does this occur? I would expect that we'd only be iterating through tiles intersecting layerRect.

It seems to happen when the layer shrinks so that some tiles are no longer used. I guess we could intersect the contentRect by the layerRect before passing it to contentRectToTileIndices. The current code is the way it was before I touched this.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:544
> > +            IntRect displayRect = tileRect;
> > +            tileRect.intersect(layerRect);
> 
> tileRect is in content space at this point.  You should intersect by the content space rect of the tile and not the layer space one.  This doesn't make a difference now because no layers actually use translations, but it will when I land my RTL layer patch.

Ok, this code is from before landing any anti-aliasing patches. I'll change it if you think it should be part of this patch.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:552
> > +            // Skip empty tiles.
> > +            if (tileRect.isEmpty())
> > +                continue;
> 
> Don't you need to be setting previous edges here?

The only time we encounter this is for right-most tiles and we wouldn't actually want to update the previous edge if it happened for the left-most tile anyhow.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:562
> > +            FloatRect clampRect(tileRect);
> > +            // Clamp texture coordinates to avoid sampling outside the layer.
> > +            const float epsilon = 1 / 1024.0f;
> > +            float clampX = min(0.5, clampRect.width() / 2.0 - epsilon);
> > +            float clampY = min(0.5, clampRect.height() / 2.0 - epsilon);
> > +            clampRect.inflateX(-clampX);
> > +            clampRect.inflateY(-clampY);
> 
> Forgive me for asking, but I don't follow this.  The comment says "sampling outside the layer", but you're clamping to the tile rect in content space.  We already set CLAMP_TO_EDGE for all textures requested from the TextureManager, so I'm unsure of what this is needed for.

Tile is often larger than the contents and we need to constrain texture sampling to the contents area. Linear filtering will sample outside the contents area when the texture is scaled or when anti-aliasing is used. This is to prevent that.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:579
> > +            // Map clamping rectangle to unit square.
> > +            float vsTexTranslateX = -clampRect.x() / clampRect.width();
> > +            float vsTexTranslateY = -clampRect.y() / clampRect.height();
> > +            float vsTexScaleX = tileRect.width() / clampRect.width();
> > +            float vsTexScaleY = tileRect.height() / clampRect.height();
> > +
> > +            // Map to normalized texture coordinates.
> > +            float fsTexTranslateX = texOffset.x() / tileWidth;
> > +            float fsTexTranslateY = texOffset.y() / tileHeight;
> > +            float fsTexScaleX = clampRect.width() / tileWidth;
> > +            float fsTexScaleY = clampRect.height() / tileHeight;
> 
> Style nit: don't abbreviate vs and fs here.

Sure. I assume you want the ShaderChromium.* code changed too.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:634
> > +                GLC(context, context->uniform4fv(edgeLocation, edge, 4));
> 
> Can't we just pass the layer edges as a uniform once instead of the tile edges for every draw call? FragmentShaderRGBATexAlphaAA looks like it converts fragment position into viewport space and then compares against tile edges.  Would it be incorrect to compare against layer edges there?

We could. Doing it once per tile is just in line with eventually moving the edge intersection code to the vertex shader but now that I'm thinking about it seems like I can make the patch a bit smaller by not doing this and just setting it once per tile.

[David Reveman]

Created attachment 102537 [details]
Patch

[David Reveman]

Created attachment 102541 [details]
Patch

[David Reveman]

This latest patch became simpler by not setting the edge uniform for each tile which allowed me to remove the LayerTilerChromium:Edge type.

[Adrienne Walker]

(In reply to comment #30)

> The main reason for doing this on the CPU-side right now is that I didn't want this first patch to get too large. My plan is to move what makes sense to the vertex shader after this first patch has landed.

Ok.  If you're planning to follow up with that, then I'm fine with it not being in this patch.  Can you file a bug to do so now and make this one block it?

> > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:513
> > > +            // Skip empty rows.
> > > +            if (tileRect.isEmpty())
> > > +                continue;
> > 
> > When does this occur? I would expect that we'd only be iterating through tiles intersecting layerRect.
> 
> It seems to happen when the layer shrinks so that some tiles are no longer used. I guess we could intersect the contentRect by the layerRect before passing it to contentRectToTileIndices. The current code is the way it was before I touched this.

Thanks for the info.  That's fine to leave, I was mostly just curious when it might happen.  Maybe things would be more clear if the tiler had an explicit resize layer function.

> > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:544
> > > +            IntRect displayRect = tileRect;
> > > +            tileRect.intersect(layerRect);
> > 
> > tileRect is in content space at this point.  You should intersect by the content space rect of the tile and not the layer space one.  This doesn't make a difference now because no layers actually use translations, but it will when I land my RTL layer patch.
> 
> Ok, this code is from before landing any anti-aliasing patches. I'll change it if you think it should be part of this patch.

Just as a side note, David and I talked about this offline and I think I'm wrong here.  I'll look at this again when I look at fixing the RTL issues.

> > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:562
> > > +            FloatRect clampRect(tileRect);
> > > +            // Clamp texture coordinates to avoid sampling outside the layer.
> > > +            const float epsilon = 1 / 1024.0f;
> > > +            float clampX = min(0.5, clampRect.width() / 2.0 - epsilon);
> > > +            float clampY = min(0.5, clampRect.height() / 2.0 - epsilon);
> > > +            clampRect.inflateX(-clampX);
> > > +            clampRect.inflateY(-clampY);
> > 
> > Forgive me for asking, but I don't follow this.  The comment says "sampling outside the layer", but you're clamping to the tile rect in content space.  We already set CLAMP_TO_EDGE for all textures requested from the TextureManager, so I'm unsure of what this is needed for.
> 
> Tile is often larger than the contents and we need to constrain texture sampling to the contents area. Linear filtering will sample outside the contents area when the texture is scaled or when anti-aliasing is used. This is to prevent that.

Ah, I see.  So, you go in half a pixel on either end and implement a fake clamp to edge (via the clamp instruction in the fragment shader and two texture transforms) so you don't double apply the edge fading for right/bottom tiles.

And, in case you have a tile with only one pixel in content space (but on a larger texture with borders), you have to subtract epsilon so you don't divide by a zero width when normalizing and unnormalizing to this "half texel deflated" space.

Would it be more simple to just explicitly pass a max/min texture coord pair to the fragment shader rather than this double transform and extra code here? (It might not be.  I'm just curious.) If it's not, could you instead add more comments here about what all this code is doing?

> > Style nit: don't abbreviate vs and fs here.
> 
> Sure. I assume you want the ShaderChromium.* code changed too.

Yup.  That's WebKit for you.  Also, please update the ChangeLog to reflect that too.

> > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:634
> > > +                GLC(context, context->uniform4fv(edgeLocation, edge, 4));
> > 
> > Can't we just pass the layer edges as a uniform once instead of the tile edges for every draw call? FragmentShaderRGBATexAlphaAA looks like it converts fragment position into viewport space and then compares against tile edges.  Would it be incorrect to compare against layer edges there?
> 
> We could. Doing it once per tile is just in line with eventually moving the edge intersection code to the vertex shader but now that I'm thinking about it seems like I can make the patch a bit smaller by not doing this and just setting it once per tile.

Thanks for doing that.

[David Reveman]

(In reply to comment #34)
> (In reply to comment #30)
> 
> > The main reason for doing this on the CPU-side right now is that I didn't want this first patch to get too large. My plan is to move what makes sense to the vertex shader after this first patch has landed.
> 
> Ok.  If you're planning to follow up with that, then I'm fine with it not being in this patch.  Can you file a bug to do so now and make this one block it?

Done.

> 
> > > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:513
> > > > +            // Skip empty rows.
> > > > +            if (tileRect.isEmpty())
> > > > +                continue;
> > > 
> > > When does this occur? I would expect that we'd only be iterating through tiles intersecting layerRect.
> > 
> > It seems to happen when the layer shrinks so that some tiles are no longer used. I guess we could intersect the contentRect by the layerRect before passing it to contentRectToTileIndices. The current code is the way it was before I touched this.
> 
> Thanks for the info.  That's fine to leave, I was mostly just curious when it might happen.  Maybe things would be more clear if the tiler had an explicit resize layer function.
> 
> > > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:544
> > > > +            IntRect displayRect = tileRect;
> > > > +            tileRect.intersect(layerRect);
> > > 
> > > tileRect is in content space at this point.  You should intersect by the content space rect of the tile and not the layer space one.  This doesn't make a difference now because no layers actually use translations, but it will when I land my RTL layer patch.
> > 
> > Ok, this code is from before landing any anti-aliasing patches. I'll change it if you think it should be part of this patch.
> 
> Just as a side note, David and I talked about this offline and I think I'm wrong here.  I'll look at this again when I look at fixing the RTL issues.
> 
> > > > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:562
> > > > +            FloatRect clampRect(tileRect);
> > > > +            // Clamp texture coordinates to avoid sampling outside the layer.
> > > > +            const float epsilon = 1 / 1024.0f;
> > > > +            float clampX = min(0.5, clampRect.width() / 2.0 - epsilon);
> > > > +            float clampY = min(0.5, clampRect.height() / 2.0 - epsilon);
> > > > +            clampRect.inflateX(-clampX);
> > > > +            clampRect.inflateY(-clampY);
> > > 
> > > Forgive me for asking, but I don't follow this.  The comment says "sampling outside the layer", but you're clamping to the tile rect in content space.  We already set CLAMP_TO_EDGE for all textures requested from the TextureManager, so I'm unsure of what this is needed for.
> > 
> > Tile is often larger than the contents and we need to constrain texture sampling to the contents area. Linear filtering will sample outside the contents area when the texture is scaled or when anti-aliasing is used. This is to prevent that.
> 
> Ah, I see.  So, you go in half a pixel on either end and implement a fake clamp to edge (via the clamp instruction in the fragment shader and two texture transforms) so you don't double apply the edge fading for right/bottom tiles.

Exactly.

> 
> And, in case you have a tile with only one pixel in content space (but on a larger texture with borders), you have to subtract epsilon so you don't divide by a zero width when normalizing and unnormalizing to this "half texel deflated" space.

Yup.

> 
> Would it be more simple to just explicitly pass a max/min texture coord pair to the fragment shader rather than this double transform and extra code here? (It might not be.  I'm just curious.) If it's not, could you instead add more comments here about what all this code is doing?

My intuition tells me that clamping to 0-1 range + a MAD instruction with two constants is more efficient on the GPU than clamping to a specific range. I could be wrong.

I'll add some more comments.

> 
> > > Style nit: don't abbreviate vs and fs here.
> > 
> > Sure. I assume you want the ShaderChromium.* code changed too.
> 
> Yup.  That's WebKit for you.  Also, please update the ChangeLog to reflect that too.

Oh, good catch.

[David Reveman]

Created attachment 102562 [details]
Patch

[David Reveman]

Created attachment 102670 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 102562 [details]
Patch

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

I'm afraid I'm still a bit uncertain about how edge AA works.  A high level description of how it affects the tiles, which tiles get borders, and how the shader deals with them would be super helpful (both for the review and also for posterity).

I'm also somewhat concerned that, according to my reading of the code, it looks like we'll be loosing subpixel AA text in all tiled layers, including the root.  Is that correct?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:443
> +void LayerTilerChromium::drawTiles(const IntRect& contentRect, const TransformationMatrix& globalTransform, float opacity, const T* program, int fragmentTexTransformLocation, int edgeLocation)

Nit:  It might be more clear to call this method drawTilesAA.  The arguments alone are not a good indication of how it differs from the "regular" drawTiles method. As a matter of fact, it looks lik the fragmentTexTransformLocation and edgeLocation can both be computed in this method directly from the program, thus reducing the number of arguments passed.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:129
> +        LayerTilerChromium::HasBorderTexels);

if we force all tiles to have border texels, it seems like we will looks subpixel AA text everywhere (including the root layer).  LayerTextureUpdatedBitmap::prepareToUpdate uses the existence of border texels as a sign to turn off  AA text

[David Reveman]

(In reply to comment #38)
> (From update of attachment 102562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102562&action=review
> 
> I'm afraid I'm still a bit uncertain about how edge AA works.  A high level description of how it affects the tiles, which tiles get borders, and how the shader deals with them would be super helpful (both for the review and also for posterity).

Sure, I can put together that. What's done in the shader and not will likely change after this patch has landed though.

Here's the short description for now:

1. Device space layer edges are computed and passed to the fragment shader.
2. Vertex and texture coordinates are adjusted so that all fragments that are partially covered by the layer are at least 50% covered. This is simply to make sure that all fragments that are affected by the layer anti-aliasing are processed.
3. The alpha value of each fragment is computed based on the distance to each of the 4 device space layer edges.

That's basically it for non-tiled layers. The difference for a tiled layer is that step 2 is done only to tile edges that represent a real edge of the layer and texture coordinates need to be clamped to avoid sampling outside the contents area of tile textures.

> 
> I'm also somewhat concerned that, according to my reading of the code, it looks like we'll be loosing subpixel AA text in all tiled layers, including the root.  Is that correct?

No, nothing changes in that respect.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:443
> > +void LayerTilerChromium::drawTiles(const IntRect& contentRect, const TransformationMatrix& globalTransform, float opacity, const T* program, int fragmentTexTransformLocation, int edgeLocation)
> 
> Nit:  It might be more clear to call this method drawTilesAA.  The arguments alone are not a good indication of how it differs from the "regular" drawTiles method. As a matter of fact, it looks lik the fragmentTexTransformLocation and edgeLocation can both be computed in this method directly from the program, thus reducing the number of arguments passed.

|drawTiles| does both anti-aliased drawing an aliased drawing so I'm not sure |drawTilesAA| is a good name. The |draw| function determines what shader program to use for drawing the layer. |drawTiles| uses a template type for the program argument and cannot access AA program specific members, which is why fragmentTexTransformLocation and edgeLocation are passed as arguments to |drawTiles|.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:129
> > +        LayerTilerChromium::HasBorderTexels);
> 
> if we force all tiles to have border texels, it seems like we will looks subpixel AA text everywhere (including the root layer).  LayerTextureUpdatedBitmap::prepareToUpdate uses the existence of border texels as a sign to turn off  AA text

This is the same as before the first AA patch landed. The root layer is not affected by this. The reason this is currently not always HasBorderTexels in ToT is that the mask layer rendering code can't handle layers that include the outer border used by the AA method currently in ToT.

[Vangelis Kokkevis]

Ok, thanks!  I have a much better understanding of the code now.  I was mostly reading it wrong... Unofficial r+ from me.

[James Robinson]

Comment on attachment 102670 [details]
Patch

OK, R=me.  I'll land by hand to make sure that we don't check bad expectations into test_expectations.txt

[James Robinson]

Committed r92255: <http://trac.webkit.org/changeset/92255>

[Jian Li]

Reactivate this bug since the following tests are still failing:

BUGWK64942 WIN GPU : compositing/color-matching/image-color-matching.html = IMAGE
BUGWK64942 WIN GPU : compositing/direct-image-compositing.html = IMAGE
BUGWK64942 WIN GPU : compositing/geometry/ancestor-overflow-change.html = IMAGE
BUGWK64942 WIN GPU : compositing/geometry/fixed-position.html = IMAGE
BUGWK64942 WIN GPU : compositing/geometry/horizontal-scroll-composited.html = IMAGE
BUGWK64942 WIN GPU : compositing/geometry/layer-due-to-layer-children-deep.html = IMAGE
BUGWK64942 WIN GPU : compositing/geometry/layer-due-to-layer-children.html = IMAGE
BUGWK64942 WIN GPU : compositing/geometry/vertical-scroll-composited.html = IMAGE
BUGWK64942 WIN GPU : compositing/iframes/composited-iframe-alignment.html = IMAGE
BUGWK64942 WIN GPU : compositing/masks/masked-ancestor.html = IMAGE
BUGWK64942 WIN GPU : compositing/overflow/fixed-position-ancestor-clip.html = IMAGE
BUGWK64942 WIN GPU : compositing/reflections/nested-reflection-transition.html = IMAGE
BUGWK64942 WIN GPU : compositing/reflections/transform-inside-reflection.html = IMAGE
BUGWK64942 WIN GPU : compositing/shadows/shadow-drawing.html = IMAGE
BUGWK64942 WIN MAC GPU :  compositing/transitions/scale-transition-no-start.html = IMAGE
BUGWK64942 MAC GPU : media/video-transformed.html = IMAGE
BUGWK64942 MAC GPU : media/video-zoom-controls.html = IMAGE
BUGWK64942 MAC GPU : platform/chromium/compositing/backface-visibility-transformed.html = IMAGE
BUGWK64942 MAC GPU : platform/chromium/compositing/huge-layer-rotated.html = IMAGE
BUGWK64942 MAC GPU : platform/chromium/compositing/layout-width-change.html = IMAGE
BUGWK64942 MAC GPU : platform/chromium/compositing/perpendicular-layer-sorting.html = IMAGE
BUGWK64942 MAC GPU : platform/chromium/compositing/tiny-layer-rotated.html = IMAGE

[David Reveman]

AFAIK, this is only related to rebaselining of affected tests. Closing this bug and reopening https://bugs.webkit.org/show_bug.cgi?id=65736 instead.