Bug 61388 – [chromium] edge aliasing on 3D transformed elements

• Component: CSS
• Status: RESOLVED FIXED

[David Reveman]

Chromium doesn't anti-alias edges of composited layers. See: http://code.google.com/p/chromium/issues/detail?id=51839

[David Reveman]

Created attachment 94677 [details]
Layer anti-aliasing

Patch implements anti-aliasing by adding transparent borders around layers and adjusting vertices to make sure we process all necessary fragments.

This approach handles tiled layers properly and produces smooth edges that match bi-linear filter output.

Known issues:
- Creates textures that are 1 or 2 pixels larger than the tile size.
- Makes the assumption that textures are initialized to transparent black.

[Adrienne Walker]

My first impression is that I feel like this is a lot of code and complexity to work around the fact that TilingData only has inter-tile border texels and not outer ones.  The TilingData abstraction already handles texture coordinate offsets, border texels, and returning tile rects with and without those borders.

Let me propose two changes which result in the equivalent edge filtering, but that might be simpler:

1) Add outer border texels to TilingData when any border texels are requested.

2) Modify the dimensions of the quad being drawn when calling drawTexturedQuad in LayerTilerChromium to include the outer border texels, rather than doing all of the math and program binding in GeometryBinding just to stretch the quad to span an additional texel.

Re: known issues.

I am less comfortable with textures being created larger than the tile size.  This circumvents a lot of the recycling mechanisms where we put unused tiles on a list to be reused under the assumption that we can just reuse the same texture resource without doing another texImage2D and clear.  It's certainly possible this is a needless performance optimization, but if the assumption is broken that tile size == texture size, then ideally we should measure if that has any impact and if it doesn't then we should remove the recycling code as well.

Textures are already initialized to transparent black (often repeatedly). See texImage2DResourceSafe called by TextureManager::requestTexture.

[David Reveman]

(In reply to comment #2)
> My first impression is that I feel like this is a lot of code and complexity to work around the fact that TilingData only has inter-tile border texels and not outer ones.  The TilingData abstraction already handles texture coordinate offsets, border texels, and returning tile rects with and without those borders.
> 
> Let me propose two changes which result in the equivalent edge filtering, but that might be simpler:
> 
> 1) Add outer border texels to TilingData when any border texels are requested.

This was my initial approach and I agree that it's the ideal place for this outer border code live. I spent some time implementing this but decided to hold off as it required a large amount of changes. LayerTilerChromium and ContentLayerPainter make a lot of assumptions about the tile sizes which break when we add an outer border (e.g. ContentLayerChromium uses the layer size as tile size).

I'm happy to move the changes into the TilingData class if we agree that this border texel approach is the proper way to implement smooth layer edges. FYI, the patch will grow significantly from this.


> 
> 2) Modify the dimensions of the quad being drawn when calling drawTexturedQuad in LayerTilerChromium to include the outer border texels, rather than doing all of the math and program binding in GeometryBinding just to stretch the quad to span an additional texel.

We're not just stretching it to include the additional texels. We're projecting vertices and making adjustments to ensure that we rasterize all necessary fragments. This is critical for any kind of minification and these adjustments results in us no longer drawing an axis-aligned quad. I guess it should be possible to translate these adjustments into a matrix transformation but I'm not convinced that's more appropriate than doing the math in GeometryBinding.


> 
> Re: known issues.
> 
> I am less comfortable with textures being created larger than the tile size.  This circumvents a lot of the recycling mechanisms where we put unused tiles on a list to be reused under the assumption that we can just reuse the same texture resource without doing another texImage2D and clear.  It's certainly possible this is a needless performance optimization, but if the assumption is broken that tile size == texture size, then ideally we should measure if that has any impact and if it doesn't then we should remove the recycling code as well.
> 

I agree. This will be taken care of by moving the outer border adjustments into TilingData class.


> Textures are already initialized to transparent black (often repeatedly). See texImage2DResourceSafe called by TextureManager::requestTexture.

I noticed this. Just wasn't sure that it was OK to rely on it to always be the case..

[WebKit Review Bot]

Attachment 94677 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/chromium/..." exit_code: 1

Source/WebCore/platform/graphics/chromium/GeometryBinding.h:52:  The parameter name "matrix" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/GeometryBinding.h:53:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/GeometryBinding.h:54:  The parameter name "filter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/GeometryBinding.h:74:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/graphics/chromium/GeometryBinding.h:75:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/graphics/chromium/GeometryBinding.h:76:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:137:  prev_edge is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:202:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:204:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:420:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:422:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 11 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Reveman]

Created attachment 94911 [details]
Layer anti-aliasing

Updated to pass the webkit style check

[Adrienne Walker]

(In reply to comment #3)
> (In reply to comment #2)
>
> > 1) Add outer border texels to TilingData when any border texels are requested.
> 
> This was my initial approach and I agree that it's the ideal place for this outer border code live. I spent some time implementing this but decided to hold off as it required a large amount of changes. LayerTilerChromium and ContentLayerPainter make a lot of assumptions about the tile sizes which break when we add an outer border (e.g. ContentLayerChromium uses the layer size as tile size).

That path with those assumptions is there just so that we can use the same codepath for painting, uploading, and drawing layers whether for tiled and "single tile" layers.

You're quite right that there's this unfortunate assumption where setting tile size = layer size implies "use a single tile".   That seems backwards.  If anything, those layers should be explicit that they want to use a single tile and then let the tiler figure out what that means in terms of layer size and texture size and whether we should include an additional border.

The other problem is that touching TilingData will require fixing up most of the TilingDataTest unit tests which assume that there's no outer border.

> I'm happy to move the changes into the TilingData class if we agree that this border texel approach is the proper way to implement smooth layer edges. FYI, the patch will grow significantly from this.

I think that's the right thing to do.  The patch might grow, but I think ultimately it'll make the code in both TilingData and LayerTilerChromium more simple.  It'd be nice to not make the LayerTilerChromium calculations for copying one subrectangle to another out of a paint rect any less clear than it already is.

I talked to Vince offline about it and given that LayerTilerChromium is the only client of TilingData at this point, we both don't see any issue with such a change to TilingData.

> > 2) Modify the dimensions of the quad being drawn when calling drawTexturedQuad in LayerTilerChromium to include the outer border texels, rather than doing all of the math and program binding in GeometryBinding just to stretch the quad to span an additional texel.
> 
> We're not just stretching it to include the additional texels. We're projecting vertices and making adjustments to ensure that we rasterize all necessary fragments.

Oh! Sorry for my misunderstanding.  What you're doing makes complete sense.

My only remaining concern related to the GeometryBinding is that we create and destroy two buffer objects for every tile we draw on screen.  Does that have no performance costs?

[David Reveman]

Created attachment 95262 [details]
Layer anti-aliasing

[David Reveman]

Latest patch moves the outer border handling into TilingData class and updates the unit tests.

> My only remaining concern related to the GeometryBinding is that we create and destroy two buffer objects for every tile we draw on screen.  Does that have no performance costs?

I don't think it has a significant performance cost. Creating and destroying buffers should be more efficiently handled by graphics drivers than reusing a buffer and just changing it's contents. We could use the shared geometry buffer for inner tiles that doesn't need smooth borders.

[David Reveman]

Created attachment 95500 [details]
Layer anti-aliasing

Fix a problem in LayerTilerChromium::growLayerToContain where max texture size was not being set correctly.

[Adrienne Walker]

Thanks for that.  I think that change simplifies the code.

[James Robinson]

Comment on attachment 95500 [details]
Layer anti-aliasing

Please flip the review flag to "?" when you want this to be reviewed.

I expect this patch will change a good number of compositor pixel test results, could you post updated baselines?

[Stephen White]

Comment on attachment 95500 [details]
Layer anti-aliasing

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

> WebCore/platform/graphics/chromium/GeometryBinding.h:75
> +        Edge(float x, float y, float z) : fX(x), fY(y), fZ(z) { }

In WebKit coding style, data members should be prefixed with m_.  ('f' is Skia style.).

(Actually, in WebKit I'd probably scrap this class, use a FloatPoint3D, and write a helper function for intersection instead.  But that's up to you.)

> WebCore/platform/graphics/chromium/GeometryBinding.cpp:68
> +    return (v1.x() * v2.y() - v1.y() * v2.x()) < 0;

Just a warning that the original version of this code gives incorrect results if the three input points are collinear.  Not sure if that can occur with a compositor quad.  Perhaps when rotating 90deg around X, for example.

> WebCore/platform/graphics/chromium/GeometryBinding.cpp:141
> +        pt[i] = prevEdge.intersect(edge);

Also here:  there may be problems if three consecutive vertices are collinear (intersection will fail).

[David Reveman]

Created attachment 95760 [details]
Layer anti-aliasing

Fix a problem with outer borders not being kept transparent and mask layer textures not having the assumed size.

[David Reveman]

(In reply to comment #12)
> In WebKit coding style, data members should be prefixed with m_.  ('f' is Skia style.).
> 
> (Actually, in WebKit I'd probably scrap this class, use a FloatPoint3D, and write a helper function for intersection instead.  But that's up to you.)

Latest patch uses FloatPoint3D.

> 
> > WebCore/platform/graphics/chromium/GeometryBinding.cpp:68
> > +    return (v1.x() * v2.y() - v1.y() * v2.x()) < 0;
> 
> Just a warning that the original version of this code gives incorrect results if the three input points are collinear.  Not sure if that can occur with a compositor quad.  Perhaps when rotating 90deg around X, for example.

I've noticed this and made sure to check that this code path is not exercised for such transforms as 90deg rotation around X. The visible layer rectangle (ContentLayerChromium::draw) should be empty in these cases.

[David Reveman]

Created attachment 95835 [details]
test_expectations.txt changes for rebaseline

[David Reveman]

(In reply to comment #11)
> (From update of attachment 95500 [details])
> Please flip the review flag to "?" when you want this to be reviewed.

I think the patch is now ready to be reviewed.

> 
> I expect this patch will change a good number of compositor pixel test results, could you post updated baselines?

I haven't managed to get the webkit rebaseline tool to create a proper patch yet but I've attached the test_expectations.txt change with all the tests that need to be rebased for now.

Test case worth pointing out is compositing/transitions/scale-transition-no-start.html which is now failing by design. This layer anti-aliasing method will not produce the output that this test is asking for.

[Vangelis Kokkevis]

The rebaseline tool must be run after the patch has landed and the bots produce their results (so that it can check for diffs and harvest them).  You'll need to mark the tests that you know will fail as FAIL IMAGE etc when the patch is submitted and once it's landed, you'll locally mark test_expectations as REBASELINE run the tool and that will create an additional patch for the new baselines.

[David Reveman]

Created attachment 95975 [details]
Layer anti-aliasing

Add missing ChangeLog entries.

[David Reveman]

Created attachment 95980 [details]
LayoutTest changes

[James Robinson]

Comment on attachment 95975 [details]
Layer anti-aliasing

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

Would you mind updating this patch to apply to ToT?  I think Vince, Vangelis, or Enne should review it - the actual graphics stuff here is well beyond me.

> WebCore/ChangeLog:12
> +        No new tests. (OOPS!)

you need to get rid of the OOPS! here - replace it with a list of tests that cover this change

> WebCore/ChangeLog:16
> +        Change maxUntiledSize to 510 to ensure that tiles are not greater
> +        than 512 with outer boarders.

typo: boarders->borders

> WebCore/ChangeLog:18
> +        (WebCore::ContentLayerChromium::updateLayerSize): To avoid tiling,
> +        pass empty tile size to tiler.

this comment doesn't make a lot of sense to me

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:189
> -    const IntSize tileSize(min(defaultTileSize, layerSize.width()), min(defaultTileSize, layerSize.height()));
> +    const IntSize tileSize(defaultTileSize, defaultTileSize);

this change seems bad - all tiles are 256x256 regardless of the layer size?

> WebCore/platform/graphics/chromium/ContentLayerChromium.h:88
> +    bool m_borderTexels;

why does ContentLayerChromium need an m_borderTexels? is this a bad merge?

> WebCore/platform/graphics/chromium/GeometryBinding.cpp:92
> +    int x1 = rect.x();
> +    int y1 = rect.y();
> +    int x2 = rect.maxX();
> +    int y2 = rect.maxY();

should this be an IntRect?

> WebCore/platform/graphics/chromium/GeometryBinding.cpp:104
> +    FloatPoint pt[4];

we have a FloatQuad data type, would that be better here?

> WebCore/platform/graphics/chromium/GeometryBinding.cpp:111
> +    for (int i = 0; i < 4; i++)
> +        pt[i] = matrix.mapPoint(pt[i]);

i believe we can map FloatQuads directly without having to do it point at a time

> WebCore/platform/graphics/chromium/GeometryBinding.h:51
> +    explicit GeometryBinding(GraphicsContext3D*,

nit: no need for 'explicit' on constructors that take more than one parameter

[David Reveman]

Created attachment 96883 [details]
Layer anti-aliasing

[WebKit Review Bot]

Attachment 96883 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/platform/graphics/chromium/GeometryBinding.h:52:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/graphics/chromium/GeometryBinding.h:53:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/graphics/chromium/GeometryBinding.h:54:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Reveman]

Comment on attachment 95975 [details]
Layer anti-aliasing

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

>> WebCore/ChangeLog:12

> 
> you need to get rid of the OOPS! here - replace it with a list of tests that cover this change

Added new platform/chromium/compositing/tiny-layer-rotated.html test and the existing platform/chromium/compositing/huge-layer-rotated.html test, they cover the change pretty well. Also included the TilingData unit test.

>> WebCore/ChangeLog:16
>> +        than 512 with outer boarders.
> 
> typo: boarders->borders

Fixed.

>> WebCore/ChangeLog:18
>> +        pass empty tile size to tiler.
> 
> this comment doesn't make a lot of sense to me

This hopefully makes a bit more sense in my latest patch.

>> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:189
>> +    const IntSize tileSize(defaultTileSize, defaultTileSize);
> 
> this change seems bad - all tiles are 256x256 regardless of the layer size?

LayerTilerChromium is not using this as the texture size anymore. It's used as the max texture size for the TilingData class. The problem with using the layerSize as tileSize here is that with outer borders this is not enough space for the layer to fit in one tile. LayerTilerChromium will now figure out what the smallest possible texture size is including any optional outer borders.

>> WebCore/platform/graphics/chromium/ContentLayerChromium.h:88
>> +    bool m_borderTexels;
> 
> why does ContentLayerChromium need an m_borderTexels? is this a bad merge?

No, this is correct. The mask layer rendering in RenderSurfaceChromium doesn't support outer borders yet so we have to avoid using border texels when content layer is used as a mask.

>> WebCore/platform/graphics/chromium/GeometryBinding.cpp:92
>> +    int y2 = rect.maxY();
> 
> should this be an IntRect?

I found the border texel adjustment code slightly more readable this way. E.g. "x--" vs "rect.shiftXEdgeTo(rect.x() - 1)". I don't really have a strong opinion here though so I'm happy to change it to an IntRect if you prefer that.

>> WebCore/platform/graphics/chromium/GeometryBinding.cpp:104
>> +    FloatPoint pt[4];
> 
> we have a FloatQuad data type, would that be better here?

The edge calculation code requires indexed access to the points, which FloatQuad doesn't provide.

>> WebCore/platform/graphics/chromium/GeometryBinding.h:51
>> +    explicit GeometryBinding(GraphicsContext3D*,
> 
> nit: no need for 'explicit' on constructors that take more than one parameter

Fixed.

[David Reveman]

Created attachment 96886 [details]
Layer anti-aliasing

[Adrienne Walker]

(In reply to comment #24)
> Created an attachment (id=96886) [details]
> Layer anti-aliasing

Unofficially, this looks good to me.

Can I hope that edge anti-aliasing for render surfaces is coming afterwards?

[David Reveman]

(In reply to comment #25)
> (In reply to comment #24)
> > Created an attachment (id=96886) [details] [details]
> > Layer anti-aliasing
> 
> Unofficially, this looks good to me.

Thanks for having a look at the patch.

If you run the compositing layout tests without my changes to test_expectations.txt you'll see minor differences in almost all compositing test. That's to expect for any transformed layers. However, we might want to avoid this for layers that are not transformed in a way that require bilinear filtering. Simply using nearest filter in these cases should take care of it but I'm thinking such a change is better left for a subsequent patch.

> 
> Can I hope that edge anti-aliasing for render surfaces is coming afterwards?

Yes, I'd happily add that once this patch has landed.

[Vangelis Kokkevis]

Comment on attachment 96886 [details]
Layer anti-aliasing

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

I worry that we're burdening the compositor with a lot of extra cpu-side operations, buffer creates/deletes and uploads.  Is there maybe a simpler way to achieve the same effect given that we're restricted to rendering quads and not general polygons?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:418
> +

It concerns me a lot that for every quad draw with borderTexels we end up creating two new vbo's and upload vertex and index data to them. Is it not possible to always include the extra vertices and triangles in the compositor's sharedGeometry and make the necessary adjustments to their positions in the vertex shader?

> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:113
> +    Edge edges[4];

This code could use some comments explaining what it does.  I'm not sure I understand the method used here.

> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:129
> +    // We don't care about Z component.

Again, I'm concerned that we're adding lots of cpu-based math that will be executed for every layer we render (possibly multiple times due to tiling).

[David Reveman]

Comment on attachment 96886 [details]
Layer anti-aliasing

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

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:418
>> +
> 
> It concerns me a lot that for every quad draw with borderTexels we end up creating two new vbo's and upload vertex and index data to them. Is it not possible to always include the extra vertices and triangles in the compositor's sharedGeometry and make the necessary adjustments to their positions in the vertex shader?

There are no extra vertices. Only adjustments to vertices/texcoords. The indices are the same so we could possibly reuse the shared geometry there. I mentioned earlier that it might be possible to translate these adjustments into a transformation matrix and use a uniform instead. It would be a bit backwards and there wouldn't really be much less data to transfer. But we would avoid creating the VBOs. I'd prefer to have some data showing that use of VBOs here actually has a significant performance impact before I make any changes though.

>> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:113
>> +    Edge edges[4];
> 
> This code could use some comments explaining what it does.  I'm not sure I understand the method used here.

Sure, the pixel offset part could use some explaining.

>> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:129
>> +    // We don't care about Z component.
> 
> Again, I'm concerned that we're adding lots of cpu-based math that will be executed for every layer we render (possibly multiple times due to tiling).

Some of it can't be avoided of course. We could possibly reuse some of the computed edges between tiles. And we can easily move the inverse transformation matrix creation to LayerTilerChromium.cpp and only do it once per layer.

[Vangelis Kokkevis]

(In reply to comment #28)
> (From update of attachment 96886 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96886&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:418
> >> +
> > 
> > It concerns me a lot that for every quad draw with borderTexels we end up creating two new vbo's and upload vertex and index data to them. Is it not possible to always include the extra vertices and triangles in the compositor's sharedGeometry and make the necessary adjustments to their positions in the vertex shader?
> 
> There are no extra vertices. Only adjustments to vertices/texcoords. The indices are the same so we could possibly reuse the shared geometry there. I mentioned earlier that it might be possible to translate these adjustments into a transformation matrix and use a uniform instead. It would be a bit backwards and there wouldn't really be much less data to transfer. But we would avoid creating the VBOs. I'd prefer to have some data showing that use of VBOs here actually has a significant performance impact before I make any changes though.

I imagine that creating and destroying fbo's here is equivalent (and probably to some extent translates to) to making new and delete calls. Added on top is additional overhead due to command buffer transfers, ANGLE mirroring of data structures, etc.  This is our inner loop so I think it's fair to assume that there will be a hit. Maybe you can create a single shared VBO and re-use it? A solution using uniforms may be even better.  Senorblanco mentioned that his original implementation of AA drawing for the old canvas path was doing that.  Stephen, can you provide a pointer to the CL? 


> 
> >> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:113
> >> +    Edge edges[4];
> > 
> > This code could use some comments explaining what it does.  I'm not sure I understand the method used here.
> 
> Sure, the pixel offset part could use some explaining.
> 
> >> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:129
> >> +    // We don't care about Z component.
> > 
> > Again, I'm concerned that we're adding lots of cpu-based math that will be executed for every layer we render (possibly multiple times due to tiling).
> 
> Some of it can't be avoided of course. We could possibly reuse some of the computed edges between tiles. And we can easily move the inverse transformation matrix creation to LayerTilerChromium.cpp and only do it once per layer.

Would it be possible to do the edge calculation once per quad instead of doing it per tile?

Thanks!

[Stephen White]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 96886 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=96886&action=review
> > 
> > >> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:418
> > >> +
> > > 
> > > It concerns me a lot that for every quad draw with borderTexels we end up creating two new vbo's and upload vertex and index data to them. Is it not possible to always include the extra vertices and triangles in the compositor's sharedGeometry and make the necessary adjustments to their positions in the vertex shader?
> > 
> > There are no extra vertices. Only adjustments to vertices/texcoords. The indices are the same so we could possibly reuse the shared geometry there. I mentioned earlier that it might be possible to translate these adjustments into a transformation matrix and use a uniform instead. It would be a bit backwards and there wouldn't really be much less data to transfer. But we would avoid creating the VBOs. I'd prefer to have some data showing that use of VBOs here actually has a significant performance impact before I make any changes though.
> 
> I imagine that creating and destroying fbo's here is equivalent (and probably to some extent translates to) to making new and delete calls. Added on top is additional overhead due to command buffer transfers, ANGLE mirroring of data structures, etc.  This is our inner loop so I think it's fair to assume that there will be a hit. Maybe you can create a single shared VBO and re-use it? A solution using uniforms may be even better.  Senorblanco mentioned that his original implementation of AA drawing for the old canvas path was doing that.  Stephen, can you provide a pointer to the CL? 

Here was the original WebKit CL:  https://bugs.webkit.org/attachment.cgi?id=90585&action=diff

It actually doesn't add any data uploads at all.  The trick it uses is to compute all the inflated edges in the vert shader, then blend them using the vertex position components to select the right pair of edges.  This requires the vertices to be in canonical form (0, 0), (0, 1), (1, 0), (1, 1).  It also requires you to upload the inverse transpose of the viewing matrix (and only the inverse transpose) as a uniform -- once it gets the screen-space edges, it just uses their intersection to find the verts.  No need to transform the resulting verts anymore, so no need for the viewing matrix.  I don't know if you can use this trick, and obviously it's only a win on machines with an actual vertex shader unit (Intel GMA950 need not apply).

My gut feeling is that the extra CPU overhead in David's current implementation isn't going to be huge.  Benchmarking would tell you, of course.  The cost of creating/destroying vert buffers will probably be bad, though.  If possible, making a persistent VBO of a fixed size would probably be a win.

> > >> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:113
> > >> +    Edge edges[4];
> > > 
> > > This code could use some comments explaining what it does.  I'm not sure I understand the method used here.
> > 
> > Sure, the pixel offset part could use some explaining.
> > 
> > >> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:129
> > >> +    // We don't care about Z component.
> > > 
> > > Again, I'm concerned that we're adding lots of cpu-based math that will be executed for every layer we render (possibly multiple times due to tiling).
> > 
> > Some of it can't be avoided of course. We could possibly reuse some of the computed edges between tiles. And we can easily move the inverse transformation matrix creation to LayerTilerChromium.cpp and only do it once per layer.
> 
> Would it be possible to do the edge calculation once per quad instead of doing it per tile?
> 
> Thanks!

[David Reveman]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > (From update of attachment 96886 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=96886&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:418
> > > >> +
> > > > 
> > > > It concerns me a lot that for every quad draw with borderTexels we end up creating two new vbo's and upload vertex and index data to them. Is it not possible to always include the extra vertices and triangles in the compositor's sharedGeometry and make the necessary adjustments to their positions in the vertex shader?
> > > 
> > > There are no extra vertices. Only adjustments to vertices/texcoords. The indices are the same so we could possibly reuse the shared geometry there. I mentioned earlier that it might be possible to translate these adjustments into a transformation matrix and use a uniform instead. It would be a bit backwards and there wouldn't really be much less data to transfer. But we would avoid creating the VBOs. I'd prefer to have some data showing that use of VBOs here actually has a significant performance impact before I make any changes though.
> > 
> > I imagine that creating and destroying fbo's here is equivalent (and probably to some extent translates to) to making new and delete calls. Added on top is additional overhead due to command buffer transfers, ANGLE mirroring of data structures, etc.  This is our inner loop so I think it's fair to assume that there will be a hit. Maybe you can create a single shared VBO and re-use it? A solution using uniforms may be even better.  Senorblanco mentioned that his original implementation of AA drawing for the old canvas path was doing that.  Stephen, can you provide a pointer to the CL? 
> 
> Here was the original WebKit CL:  https://bugs.webkit.org/attachment.cgi?id=90585&action=diff
> 
> It actually doesn't add any data uploads at all.  The trick it uses is to compute all the inflated edges in the vert shader, then blend them using the vertex position components to select the right pair of edges.  This requires the vertices to be in canonical form (0, 0), (0, 1), (1, 0), (1, 1).  It also requires you to upload the inverse transpose of the viewing matrix (and only the inverse transpose) as a uniform -- once it gets the screen-space edges, it just uses their intersection to find the verts.  No need to transform the resulting verts anymore, so no need for the viewing matrix.  I don't know if you can use this trick, and obviously it's only a win on machines with an actual vertex shader unit (Intel GMA950 need not apply).

Thanks, I see what you're doing there. I guess it should be possible to do something similar in the tiled layer case. A few differences are; not all edges are offset for tiled layers so we need to use additional uniforms or different shaders for each possible combination of edges. We also need to adjust texture coordinates and perspective interpolation need to be correct so computing x,y components is not enough. It might turn out to be quite a bit slower on machines without a vertex shader unit as we compute all edges for each vertex.

> 
> My gut feeling is that the extra CPU overhead in David's current implementation isn't going to be huge.  Benchmarking would tell you, of course.  The cost of creating/destroying vert buffers will probably be bad, though.  If possible, making a persistent VBO of a fixed size would probably be a win.

OK, I assume this is related to the ANGLE mirroring as I would normally expect the more async nature of creating/destroying VBO's to be more efficient than modifying one.

> 
> > > >> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:113
> > > >> +    Edge edges[4];
> > > > 
> > > > This code could use some comments explaining what it does.  I'm not sure I understand the method used here.
> > > 
> > > Sure, the pixel offset part could use some explaining.
> > > 
> > > >> Source/WebCore/platform/graphics/chromium/GeometryBinding.cpp:129
> > > >> +    // We don't care about Z component.
> > > > 
> > > > Again, I'm concerned that we're adding lots of cpu-based math that will be executed for every layer we render (possibly multiple times due to tiling).
> > > 
> > > Some of it can't be avoided of course. We could possibly reuse some of the computed edges between tiles. And we can easily move the inverse transformation matrix creation to LayerTilerChromium.cpp and only do it once per layer.
> > 
> > Would it be possible to do the edge calculation once per quad instead of doing it per tile?

I'll update the patch to minimize the number of edge calculations and use uniforms for offsets.

[David Reveman]

Created attachment 97931 [details]
Layer anti-aliasing

Uses a shader uniform instead of geometry bindings for AA adjusted vertices. Reduces the amount of per-tile CPU operations significantly.

[WebKit Review Bot]

Attachment 97931 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/platform/chromium/test_expectations.txt:930:  Path does not exist. svg/custom/svg-fonts-fallback.xhtml  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:932:  Path does not exist. svg/custom/svg-fonts-segmented.xhtml  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:933:  Path does not exist. svg/custom/svg-fonts-word-spacing.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:937:  Path does not exist. svg/text/text-overflow-ellipsis-svgfont.html  [test/expectations] [2]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Reveman]

James, please have a look at this latest patch as it changes the fix you landed for: https://bugs.webkit.org/show_bug.cgi?id=62243

Unused parts of tiles are cleared instead of only drawing portions of tiles in contentRect.

[David Reveman]

It's also worth pointing out that this new patch doesn't special case layers without borders like the root layer. It's always using the edge intersection technique to generate tile vertices. I'd like to keep it this way for the sake of having fewer alternative code paths if possible.

[David Reveman]

Created attachment 98238 [details]
Layer anti-aliasing

Reduces amount of per-tile CPU operations some more.

[James Robinson]

Comment on attachment 98238 [details]
Layer anti-aliasing

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

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:405
> +      TransformationMatrix(matrix.m11(), matrix.m12(), 0.0, matrix.m14(),
> +                           matrix.m21(), matrix.m22(), 0.0, matrix.m24(),
> +                           matrix.m31(), matrix.m32(), 1.0, matrix.m34(),
> +                           matrix.m41(), matrix.m42(), 0.0,

nit: WebKit style slightly prefers 0 / 1 for these constants

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:418
> +    float sign = isCCW(boundsQuad) ? -1.0f : 1.0f;

nit: in WebKit we use -1 / 1 for integral constants unless type promotion requires otherwise

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:538
> +void LayerTilerChromium::drawTexturedQuad(GraphicsContext3D* context, const FloatQuad &quad, const TransformationMatrix& projectionMatrix, const TransformationMatrix& drawMatrix,

nit: the & goes with the type as in "const FloatQuad& quad"

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:571
> +    float scaleX = 1.0f / width;
> +    float scaleY = 1.0f / height;
> +    float point[8];
> +    point[0] = quad.p1().x() * scaleX;
> +    point[1] = quad.p1().y() * scaleY;
> +    point[2] = quad.p2().x() * scaleX;
> +    point[3] = quad.p2().y() * scaleY;
> +    point[4] = quad.p3().x() * scaleX;
> +    point[5] = quad.p3().y() * scaleY;
> +    point[6] = quad.p4().x() * scaleX;
> +    point[7] = quad.p4().y() * scaleY;

would this be simpler if the quad was scaled instead of scaling the individual points?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:103
> +    void drawTexturedQuad(GraphicsContext3D*, const FloatQuad &, const TransformationMatrix& projectionMatrix, const TransformationMatrix& drawMatrix,

nit: "const FloatQuad&"

> LayoutTests/ChangeLog:8
> +        * platform/chromium/compositing/tiny-layer-rotated.html: Added.

where are the expectations for this test? = IMAGE is not the same as = MISSING.

you should generate + upload the pixel results for at least one platform

[Vangelis Kokkevis]

Great refactoring of the original code!

[David Reveman]

Comment on attachment 98238 [details]
Layer anti-aliasing

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

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:405
>> +                           matrix.m41(), matrix.m42(), 0.0,
> 
> nit: WebKit style slightly prefers 0 / 1 for these constants

Fixed

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:418
>> +    float sign = isCCW(boundsQuad) ? -1.0f : 1.0f;
> 
> nit: in WebKit we use -1 / 1 for integral constants unless type promotion requires otherwise

Fixed

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:538
>> +void LayerTilerChromium::drawTexturedQuad(GraphicsContext3D* context, const FloatQuad &quad, const TransformationMatrix& projectionMatrix, const TransformationMatrix& drawMatrix,
> 
> nit: the & goes with the type as in "const FloatQuad& quad"

Sorry, I keep making that mistake. Many years of working on projects that do it the other way around is hard to unlearn. Would be nice to have check-webkit-style pick this up.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:571
>> +    point[7] = quad.p4().y() * scaleY;
> 
> would this be simpler if the quad was scaled instead of scaling the individual points?

Moved the scaling to the quad in LayerTilerChromium::draw, which on second thought seems like a more appropriate place to handle this.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:103
>> +    void drawTexturedQuad(GraphicsContext3D*, const FloatQuad &, const TransformationMatrix& projectionMatrix, const TransformationMatrix& drawMatrix,
> 
> nit: "const FloatQuad&"

Fixed.

>> LayoutTests/ChangeLog:8
>> +        * platform/chromium/compositing/tiny-layer-rotated.html: Added.
> 
> where are the expectations for this test? = IMAGE is not the same as = MISSING.
> 
> you should generate + upload the pixel results for at least one platform

I hopefully got this right in my new patch.

[David Reveman]

Created attachment 98258 [details]
Layer anti-aliasing

[David Reveman]

Comment on attachment 98258 [details]
Layer anti-aliasing

What's the next step here? Another round of reviews before landing this?

I marked the latest patch for review.

[Adrienne Walker]

(In reply to comment #41)
> (From update of attachment 98258 [details])
> What's the next step here? Another round of reviews before landing this?

I talked with jamesr about this yesterday in person because we both want this patch to land.  jamesr said he doesn't feel like he can review the math part of this, but said informally that he was otherwise ok with these changes.

Could maybe senorblanco do the final review?

[Stephen White]

Comment on attachment 98258 [details]
Layer anti-aliasing

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

Looks good.  r=me

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:406
> +                           matrix.m44()).inverse();

Is this matrix always guaranteed to be invertible?  There is an "isInvertible" function on TransformationMatrix if you need to check.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:494
> +                p1 = matrix.mapPoint(p1);
> +                p2 = matrix.mapPoint(p2);
> +
> +                // Compute vertical edge.
> +                edgeX = computeEdge(p1, p2, sign);

Just FYI, if there's a chance that p1 == p2, computeEdge will return something meaningless.  Don't know if that's an issue here.

> Source/WebCore/platform/graphics/gpu/TilingData.cpp:52
> +    int numTiles = max(1, 1 + (totalSizeWithBorder - 1 - 2 * borderTexels) / (maxTextureSize - 2 * borderTexels));

Note that this code is shared with the legacy accelerated canvas implementation, so changes here will affect that too.  Probably academic at this point, since it's no longer being used or tested.

> LayoutTests/platform/chromium/test_expectations.txt:3988
> +BUGWK61388 GPU : compositing/color-matching/image-color-matching.html = IMAGE
> +BUGWK61388 GPU : compositing/direct-image-compositing.html = IMAGE
> +BUGWK61388 GPU : compositing/geometry/fixed-in-composited.html = IMAGE
> +BUGWK61388 GPU : compositing/geometry/fixed-position.html = IMAGE
> +BUGWK61388 GPU : compositing/geometry/horizontal-scroll-composited.html = IMAGE

(etc) Please try to rebaseline these as soon as possible, so we don't lose test coverage for very long.  Let me know if you need help with that.

[David Reveman]

Comment on attachment 98258 [details]
Layer anti-aliasing

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

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:406
>> +                           matrix.m44()).inverse();
> 
> Is this matrix always guaranteed to be invertible?  There is an "isInvertible" function on TransformationMatrix if you need to check.

CSS allows you to specify your own 4x4 matrix so it's not impossible that we might end up with the matrix that is not invertible here. We're probably better off not drawing anything in those cases. Adding a call to isInvertible to check for this.

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:494
>> +                edgeX = computeEdge(p1, p2, sign);
> 
> Just FYI, if there's a chance that p1 == p2, computeEdge will return something meaningless.  Don't know if that's an issue here.

Shouldn't happen but I'm adding an ASSERT to computeEdge so it's easy to tell if this for some reason changes.

>> LayoutTests/platform/chromium/test_expectations.txt:3988
>> +BUGWK61388 GPU : compositing/geometry/horizontal-scroll-composited.html = IMAGE
> 
> (etc) Please try to rebaseline these as soon as possible, so we don't lose test coverage for very long.  Let me know if you need help with that.

Sure, I'll do that asap after the patch is merged.

[David Reveman]

Created attachment 100050 [details]
Layer anti-aliasing

Rebased and updated with minor changes since last code review.

[Stephen White]

Comment on attachment 100050 [details]
Layer anti-aliasing

OK, thanks for the changes.  r=me

[WebKit Review Bot]

Comment on attachment 100050 [details]
Layer anti-aliasing

Clearing flags on attachment: 100050

Committed r90646: <http://trac.webkit.org/changeset/90646>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

Looks like this introduced some duplicate GPU expectations which are causing the bots to early-out.  I'll land a quick fix, but in the future, please run new-run-webkit-tests --lint-test-files before landing.

[David Reveman]

(In reply to comment #49)
> Looks like this introduced some duplicate GPU expectations which are causing the bots to early-out.  I'll land a quick fix, but in the future, please run new-run-webkit-tests --lint-test-files before landing.

Sorry, didn't know about --lint-test-files. Thanks for taking care of this and those other mac/win expectations that were missing.

Created this bug to rebaseline the tests:
https://bugs.webkit.org/show_bug.cgi?id=64211