Closed Bug 1029705 Opened 6 years ago Closed 3 years ago

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

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jrmuizel, Assigned: lsalzman)

References

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(1 file, 3 obsolete files)

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.
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.
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?
(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.
Blocks: skia-windows
Attached patch Do it (obsolete) — Splinter Review
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Are we sure this isn't still valuable?
Flags: needinfo?(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.
Flags: needinfo?(mchang) → needinfo?(jmuizelaar)
(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.
Flags: needinfo?(jmuizelaar) → needinfo?(lsalzman)
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.
Assignee: nobody → lsalzman
Attachment #8524190 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Flags: needinfo?(lsalzman)
Attachment #8792085 - Flags: review?(jmuizelaar)
Resolution: WONTFIX → ---
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.
Attachment #8792085 - Flags: review?(jmuizelaar)
Attachment #8792085 - Flags: review?(bas)
Attachment #8792085 - Flags: review+
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.
Attachment #8792085 - Flags: review?(bas) → review-
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.
Attachment #8793164 - Flags: review?(bas)
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.
Attachment #8793164 - Flags: review?(bas) → review-
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...
Attachment #8792085 - Attachment is obsolete: true
Attachment #8793164 - Attachment is obsolete: true
Attachment #8793446 - Flags: review?(bas)
Keywords: feature
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Whiteboard: [gfx-noted]
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?
Attachment #8793446 - Flags: review?(bas) → review+
(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
Pushed by lsalzman@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/3bdcf5e55af2
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi Lee, is this something manual QA could help test?
Flags: needinfo?(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.
Flags: needinfo?(lsalzman)
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.