Bug 72965 – [chromium] Use blending on opaque non-axis-aligned layers.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Dana Jansens]

These layers will have non-orthogonal edges. If marked opaque, and not blended, they will have strongly aliased edges.

[Dana Jansens]

Created attachment 116239 [details]
Patch

[W. James MacLean]

(In reply to comment #1)
> Created an attachment (id=116239) [details]
> Patch

You might want to take a look at Dave Reveman's comment on https://bugs.webkit.org/show_bug.cgi?id=70533 about making the axis-align (and pixel-aligned) test more consistent with what is used elsewhere in the compositor.

He recommends:

> This also needs to snap to the pixel grid to be correct. Instead of analyzing the transform, I think we should use the same logic as the anti-aliasing code, which is: 
> transformedQuad.isRectilinear() && transformedQuad.boundingBox().isExpressibleAsIntRect()

[Dana Jansens]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=116239) [details] [details]
> > Patch
> 
> You might want to take a look at Dave Reveman's comment on https://bugs.webkit.org/show_bug.cgi?id=70533 about making the axis-align (and pixel-aligned) test more consistent with what is used elsewhere in the compositor.
> 
> He recommends:
> 
> > This also needs to snap to the pixel grid to be correct. Instead of analyzing the transform, I think we should use the same logic as the anti-aliasing code, which is: 
> > transformedQuad.isRectilinear() && transformedQuad.boundingBox().isExpressibleAsIntRect()

If an opaque rect is half inside a pixel, but axis aligned, would it want blending?

It would be possible to apply the layer's drawTransform() to its visibleLayerRect() and see if the resulting quad passes the above test.

@reveman would this be preferable?

[David Reveman]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Created an attachment (id=116239) [details] [details] [details]
> > > Patch
> > 
> > You might want to take a look at Dave Reveman's comment on https://bugs.webkit.org/show_bug.cgi?id=70533 about making the axis-align (and pixel-aligned) test more consistent with what is used elsewhere in the compositor.
> > 
> > He recommends:
> > 
> > > This also needs to snap to the pixel grid to be correct. Instead of analyzing the transform, I think we should use the same logic as the anti-aliasing code, which is: 
> > > transformedQuad.isRectilinear() && transformedQuad.boundingBox().isExpressibleAsIntRect()
> 
> If an opaque rect is half inside a pixel, but axis aligned, would it want blending?

Yes, we need blending enabled whenever an anti-aliasing shader is used.

> 
> It would be possible to apply the layer's drawTransform() to its visibleLayerRect() and see if the resulting quad passes the above test.
> 
> @reveman would this be preferable?

Yes, have a look at:

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp#L115

this is exactly what we're doing there.

[Dana Jansens]

Created attachment 116421 [details]
use transformed quad to determine axis-aligned

[Dana Jansens]

Created attachment 116424 [details]
use transformed quad to determine axis-aligned v2

[David Reveman]

Comment on attachment 116424 [details]
use transformed quad to determine axis-aligned v2

This will fix the problem but we could do better for tiled layers. Only the tiles that have edges at the borders of the layer will need blending. The inner tiles don't.

Maybe we should move the disable blend code into CCLayerImpl::Draw?

[W. James MacLean]

(In reply to comment #7)
> (From update of attachment 116424 [details])
> This will fix the problem but we could do better for tiled layers. Only the tiles that have edges at the borders of the layer will need blending. The inner tiles don't.
> 
> Maybe we should move the disable blend code into CCLayerImpl::Draw?

Given that CCTileLayerImpl is about to be heavily refactored, I think we should defer blending just the edge-tiles until after that time. It can be marked as a FIXME in this patch.

[David Reveman]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 116424 [details] [details])
> > This will fix the problem but we could do better for tiled layers. Only the tiles that have edges at the borders of the layer will need blending. The inner tiles don't.
> > 
> > Maybe we should move the disable blend code into CCLayerImpl::Draw?
> 
> Given that CCTileLayerImpl is about to be heavily refactored, I think we should defer blending just the edge-tiles until after that time. It can be marked as a FIXME in this patch.

That's fine with me. Please create another bugzilla entry for this and have it depend on the CCTileLayerImpl refactoring changes.

[Dana Jansens]

Created attachment 116453 [details]
Patch

[Dana Jansens]

Pushed the decision to blend off to CCLayerImpl. This way CCTiledLayerImpl can override the behavior, but have access to the CCLayerImpl versions of the functions to help make consistent decisions on a per-tile basis.

[Dana Jansens]

Created attachment 118650 [details]
Patch

Put the blending decision behind a single public function: bool drawWithBlending().

Made protected methods to decide blending based on the contents of the layer (i.e. are all pixels opaque), and the edges of the layer (i.e. is it rectilinear or does it require anti-aliasing for the edges).

[Adrienne Walker]

(In reply to comment #12)
> Created an attachment (id=118650) [details]
> Patch
> 
> Put the blending decision behind a single public function: bool drawWithBlending().
> 
> Made protected methods to decide blending based on the contents of the layer (i.e. are all pixels opaque), and the edges of the layer (i.e. is it rectilinear or does it require anti-aliasing for the edges).

Not all layer types have antialiased edges.  Don't we want to draw any opaque layer without antialiased edges with blending off, regardless of whether it lines up on integer coordinates?

(I think https://bugs.webkit.org/show_bug.cgi?id=73059 might do some of this?)

[Dana Jansens]

Yes, 73059 is going to do exactly the right thing I think :) Hooray.

*** This bug has been marked as a duplicate of bug 73059 ***

[Eric Seidel (no email)]

Comment on attachment 118650 [details]
Patch

How can we test this?

[Eric Seidel (no email)]

Comment on attachment 118650 [details]
Patch

Cleared review? from attachment 118650 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).