Bug 72965

Created attachment 116239 [details] Patch

(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()

(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?

(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.

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

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

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?

(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.

(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.

Created attachment 116453 [details] Patch

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.

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).

(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?)

Yes, 73059 is going to do exactly the right thing I think :) Hooray. *** This bug has been marked as a duplicate of bug 73059 ***

Comment on attachment 118650 [details] Patch How can we test this?

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).