Bug 64942 - [chromium] incorrect 3D CSS anti-aliasing
Summary: [chromium] incorrect 3D CSS anti-aliasing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks: 65037
  Show dependency treegraph
 
Reported: 2011-07-21 07:26 PDT by David Reveman
Modified: 2011-08-05 13:02 PDT (History)
7 users (show)

See Also:


Attachments
Patch (68.99 KB, patch)
2011-07-21 17:38 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (69.08 KB, patch)
2011-07-22 07:43 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (68.95 KB, patch)
2011-07-25 17:07 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (68.99 KB, patch)
2011-07-26 10:10 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (74.67 KB, patch)
2011-07-28 14:02 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (69.46 KB, patch)
2011-08-01 11:20 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (69.46 KB, patch)
2011-08-01 12:07 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (69.82 KB, patch)
2011-08-01 14:52 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (69.96 KB, patch)
2011-08-02 11:42 PDT, David Reveman
jamesr: review+
jamesr: commit-queue-
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-21 07:26:20 PDT
We should use an edge distance based method instead of the current linear filtering method for producing anti-aliased edges on 3D transformed layers.
Comment 1 David Reveman 2011-07-21 17:38:44 PDT
Created attachment 101677 [details]
Patch
Comment 2 David Reveman 2011-07-22 07:43:22 PDT
Created attachment 101732 [details]
Patch
Comment 3 David Reveman 2011-07-22 09:04:47 PDT
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.
Comment 4 Brian Salomon 2011-07-22 11:07:24 PDT
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.
Comment 5 Alok Priyadarshi 2011-07-22 11:19:21 PDT
(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.
Comment 6 Brian Salomon 2011-07-22 11:24:56 PDT
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.
Comment 7 Alok Priyadarshi 2011-07-22 11:27:00 PDT
(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.
Comment 8 Brian Salomon 2011-07-22 11:30:43 PDT
(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.
Comment 9 Adrienne Walker 2011-07-22 11:44:53 PDT
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.
Comment 10 James Robinson 2011-07-22 12:24:34 PDT
(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.
Comment 11 David Reveman 2011-07-25 17:07:00 PDT
Created attachment 101945 [details]
Patch
Comment 12 David Reveman 2011-07-26 10:10:39 PDT
Created attachment 102020 [details]
Patch
Comment 13 David Reveman 2011-07-26 10:11:42 PDT
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.
Comment 14 Vangelis Kokkevis 2011-07-26 10:59:43 PDT
(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?
Comment 15 David Reveman 2011-07-26 14:16:44 PDT
(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.
Comment 16 Vangelis Kokkevis 2011-07-26 15:00:13 PDT
(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?
Comment 17 David Reveman 2011-07-28 08:16:59 PDT
(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.
Comment 18 Adrienne Walker 2011-07-28 09:17:28 PDT
(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.
Comment 19 David Reveman 2011-07-28 09:33:42 PDT
(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.
Comment 20 Vangelis Kokkevis 2011-07-28 10:53:07 PDT
(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.
Comment 21 James Robinson 2011-07-28 11:05:15 PDT
(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.
Comment 22 David Reveman 2011-07-28 14:02:07 PDT
Created attachment 102296 [details]
Patch
Comment 23 David Reveman 2011-07-28 14:14:00 PDT
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.
Comment 24 David Reveman 2011-07-28 14:16:51 PDT
(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.
Comment 25 James Robinson 2011-07-28 16:29:54 PDT
(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.
Comment 26 David Reveman 2011-07-29 07:10:29 PDT
(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.
Comment 27 Vangelis Kokkevis 2011-07-29 11:34:02 PDT
(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.
Comment 28 David Reveman 2011-07-29 12:11:15 PDT
(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.
Comment 29 Adrienne Walker 2011-07-29 16:32:39 PDT
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?
Comment 30 David Reveman 2011-08-01 08:00:25 PDT
(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.
Comment 31 David Reveman 2011-08-01 11:20:45 PDT
Created attachment 102537 [details]
Patch
Comment 32 David Reveman 2011-08-01 12:07:04 PDT
Created attachment 102541 [details]
Patch
Comment 33 David Reveman 2011-08-01 12:08:41 PDT
This latest patch became simpler by not setting the edge uniform for each tile which allowed me to remove the LayerTilerChromium:Edge type.
Comment 34 Adrienne Walker 2011-08-01 13:14:56 PDT
(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.
Comment 35 David Reveman 2011-08-01 13:45:55 PDT
(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.
Comment 36 David Reveman 2011-08-01 14:52:43 PDT
Created attachment 102562 [details]
Patch
Comment 37 David Reveman 2011-08-02 11:42:02 PDT
Created attachment 102670 [details]
Patch
Comment 38 Vangelis Kokkevis 2011-08-02 12:01:58 PDT
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
Comment 39 David Reveman 2011-08-02 14:56:46 PDT
(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.
Comment 40 Vangelis Kokkevis 2011-08-02 16:29:17 PDT
Ok, thanks!  I have a much better understanding of the code now.  I was mostly reading it wrong... Unofficial r+ from me.
Comment 41 James Robinson 2011-08-02 16:35:22 PDT
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
Comment 42 James Robinson 2011-08-02 19:52:35 PDT
Committed r92255: <http://trac.webkit.org/changeset/92255>
Comment 43 Jian Li 2011-08-05 10:55:25 PDT
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
Comment 44 David Reveman 2011-08-05 13:02:33 PDT
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.