1029705 - Add a way to clip to a region to the moz2d api

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

When you clip to a path with skia you end up going down the AA clip path even if you were using a region:

http://dxr.mozilla.org/mozilla-central/source/gfx/skia/trunk/src/core/SkRasterClip.cpp#82

We should just expose region clipping and only convert to a path for the backends that we need to.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

One problem with this is that we'll end up needing to expose some region type which sort of sucks. The best way to do this might be to just have ClipToNonOverlappingRects that takes an array.

Comment 2

[Andreas Gal :gal]

Can we do the inverse and in skia look at the path and detect that its a region and then convert to a skia region and call a different skia api?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Andreas Gal :gal from comment #2)
> Can we do the inverse and in skia look at the path and detect that its a
> region and then convert to a skia region and call a different skia api?

We could but I think I'd prefer to avoid lowering to a path in the ClipToRegion helper and then trying to detect a region in ClipToPath(). It also doesn't look like SkPath has the detection logic so we'd have to code it ourselves. Just detecting rects wouldn't be enough, because we'd also need to check for non-overlapping.

An additional wrinkle is that Skia doesn't seem to have a way to initialize a region from a set of non-overlapping rects. That being said it shouldn't be too hard to add this functionality ourselves and we could just have skia compute the union of the rects in the meantime.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Do it

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Are we sure this isn't still valuable?

Comment 6

[Mason Chang [Inactive] [:mchang]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Are we sure this isn't still valuable?

Do you know why this didn't land in the first place? It could be, I was just going through old bugs and closed inactive ones.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #6)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> > Are we sure this isn't still valuable?
> 
> Do you know why this didn't land in the first place? It could be, I was just
> going through old bugs and closed inactive ones.

I think it never quite got finished and the patch has probably rotted a bit now. Lee may have looked at it a bit more recently.

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: allow clipping to a list of rectangles (a region) in DrawTarget via PushClipRects

I noticed in the process of investigating bug 1294337 that we did spend a noticeable portion of time in our profiles for the compositor clipping paths via the AA clip route, which as noted is a bit slow.

Working from Jeff's original patch, I rebased it, cleaned up the bitrot, and fixed some iteration bugs. I also extended it to handle some trivial scale/translate matrixes.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8792085 [details] [diff] [review]
allow clipping to a list of rectangles (a region) in DrawTarget via PushClipRects

Review of attachment 8792085 [details] [diff] [review]:
-----------------------------------------------------------------

Probably worth getting Bas to look at this.

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 8792085 [details] [diff] [review]
allow clipping to a list of rectangles (a region) in DrawTarget via PushClipRects

Review of attachment 8792085 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor changes, but not too much.

::: gfx/2d/2D.h
@@ +1017,5 @@
> +   *
> +   * @param aRects The rects to clip to
> +   * @param aCount The number of rectangles
> +   */
> +  virtual void PushClipRects(const IntRect* aRects, uint32_t aCount);

Document that this is balanced by PopClip()

::: gfx/2d/DrawTarget.cpp
@@ +56,5 @@
> +  if (aCount == 1) {
> +    PushClipRect(Rect(aRects[0]));
> +  } else {
> +    RefPtr<Path> path = PathFromRects(this, aRects, aCount);
> +    PushClip(path);

Please add code to disable antialiasing for this pushed clip layer.

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: allow clipping to a list of device-space rectangles (a region) in DrawTarget via PushDeviceSpaceClipRects

So here is a patch that tries to address the concerns Bas raised while at the same time employing a slightly different approach.

The previous patch accepted clip rects in user space, which forced both the D2D1 and Skia draw targets to do largely the same validation for the same reasons - they could only really accelerate the function if the region produced axis-aligned/pixel-aligned rects in device space. Otherwise, it is just better to fall back to the usual PushClip(path).

So rather than duplicating that validation code, and after a sanity check from Jeff, I decided it might make more sense to just have PushClipRects accept rects directly in device space, hence PushDeviceSpaceClipRects.

The transformation/validation work is kept in gfxUtils::ClipToRegion for now, as this notably keeps the actual DrawTargets' implementations of PushDeviceSpaceClipRects much smaller. There are also some benefits with respect to reducing serialization to intermediate arrays of rects.

Comment 12

[Bas Schouten (:bas.schouten)]

Comment on attachment 8793164 [details] [diff] [review]
allow clipping to a list of device-space rectangles (a region) in DrawTarget via PushDeviceSpaceClipRects

Review of attachment 8793164 [details] [diff] [review]:
-----------------------------------------------------------------

I would be fine with the logic in PushClipRects to check if the final rects are pixel aligned. But I don't mind this approach either. So if this is what you prefer that's fine.

::: gfx/2d/DrawTargetD2D1.cpp
@@ +747,5 @@
>  void
> +DrawTargetD2D1::PushDeviceSpaceClipRects(const IntRect* aRects, uint32_t aCount)
> +{
> +  // Build a path for the union of the rects.
> +  RefPtr<PathBuilder> pathBuilder = CreatePathBuilder();

Can we just generate this directly as a D2D path and avoid all this nasty casting business? It was done in PushClipRect out of laziness because PushClip takes a Path* argument, but for this function I don't really think it makes sense.

@@ +751,5 @@
> +  RefPtr<PathBuilder> pathBuilder = CreatePathBuilder();
> +  for (uint32_t i = 0; i < aCount; i++) {
> +    AppendRectToPath(pathBuilder, Rect(aRects[i]));
> +  }
> +  RefPtr<PathD2D> pathD2D = static_cast<PathD2D*>(pathBuilder->Finish().downcast<PathD2D>());

Shouldn't be any need for this static cast, I'm surprised that even compiles.

::: gfx/thebes/gfxUtils.cpp
@@ +728,5 @@
>    }
>  
> +  // Check if the target's transform will preserve axis-aligned rects.
> +  Matrix transform = aTarget->GetTransform();
> +  if (transform.IsRectilinear()) {

Is this rare additional code worth it? It seems to me like we can just go IsIntegerTranslation() and if it isn't give up. Sure there's some optimizations we'd miss but do they occur often enough? There's some cost involved in checking every rect in a complex region.

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: allow clipping to a list of device-space rectangles (a region) in DrawTarget via PushDeviceSpaceClipRects

Sticking with the idea of device-space clip rects for now.

I managed to factor things out into a fancy new DrawTargetD2D1::PushClipGeometry which bypasses the need to work with an abstract Path by allowing us to just work with the ID2D1Geometry directly. Several callers now use this.

Also, did some inspection of what transforms were getting sent down, and for the most part, we are getting these from layers without any scaling thrown in, just translations. So I revised the validation code to just check if the transform is an integer translation as suggestion, which makes it quicker and cleans up the fallback case some.

This one should actually build as well...

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 8793446 [details] [diff] [review]
allow clipping to a list of device-space rectangles (a region) in DrawTarget via PushDeviceSpaceClipRects

Review of attachment 8793446 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetSkia.cpp
@@ +1826,5 @@
> +  }
> +
> +  // Clip with with resulting region.
> +  mCanvas->save();
> +  mCanvas->clipRegion(region, SkRegion::kIntersect_Op);

Does this operate in device space already?

Comment 15

[Lee Salzman [:lsalzman]]

(In reply to Bas Schouten (:bas.schouten) from comment #14)
> Comment on attachment 8793446 [details] [diff] [review]
> allow clipping to a list of device-space rectangles (a region) in DrawTarget
> via PushDeviceSpaceClipRects
> 
> Review of attachment 8793446 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +1826,5 @@
> > +  }
> > +
> > +  // Clip with with resulting region.
> > +  mCanvas->save();
> > +  mCanvas->clipRegion(region, SkRegion::kIntersect_Op);
> 
> Does this operate in device space already?

Yes, see here: https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/include/core/SkCanvas.h#491

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bdcf5e55af2
allow clipping to a list of device-space rectangles (a region) in DrawTarget via PushDeviceSpaceClipRects. r=bas

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bdcf5e55af2

Comment 18

[Andrei Vaida [:avaida]]

Hi Lee, is this something manual QA could help test?

Comment 19

[Lee Salzman [:lsalzman]]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #18)
> Hi Lee, is this something manual QA could help test?

We should have sufficient tests in tree to verify this functionality already.

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Lee Salzman [:lsalzman] from comment #19)
> (In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #18)
> > Hi Lee, is this something manual QA could help test?
> 
> We should have sufficient tests in tree to verify this functionality already.

Thank you for following up on this so quickly! Updating flags accordingly.