We want to use direct2d 1.1's source clipping where we currently use CreateSamplingRestrictedDrawable. Since this call path ends up with a FillRect call, we need to be able to specify the source clipping rectangle within SurfacePattern. How does this look for the API Bas?

Comment on attachment 8483978 [details] [diff] [review] moz2d-subimage Review of attachment 8483978 [details] [diff] [review]: ----------------------------------------------------------------- Personally I'd say make it an intrect :) Sampling from half pixels is rather arbitrary (and hard to support for some backends) I'd say, even if D2D seems to 'support' it :). Other than that it looks good! ::: gfx/2d/2D.h @@ +290,5 @@ > Matrix mMatrix; //!< Transforms the pattern into user space > + > + Rect mSamplingRect; /**< Rect that must not be sampled outside of, > + or an empty rect if none has been specified. */ > + nit: Whitespace

(In reply to Bas Schouten (:bas.schouten) from comment #6) > Comment on attachment 8483978 [details] [diff] [review] > moz2d-subimage > > Review of attachment 8483978 [details] [diff] [review]: > ----------------------------------------------------------------- > > Personally I'd say make it an intrect :) Sampling from half pixels is rather > arbitrary (and hard to support for some backends) I'd say, even if D2D seems > to 'support' it :). Other than that it looks good! Yeah, probably a good idea. Quartz and D2D take a float rect, the rest take ints. I was thinking that they were identical, but I guess that's not true for extend modes that aren't CLAMP.

Comment on attachment 8487711 [details] [diff] [review] Part 2: Implement source clipping for DrawTargetCairo Review of attachment 8487711 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetCairo.cpp @@ +175,5 @@ > cairo_surface_mark_dirty(surf); > return surf; > } > > +cairo_surface_t* GetAsImageSurface(cairo_surface_t* aSurface) nit: Document that this function never adds a reference, it's very relevant later on in this patch. @@ +265,3 @@ > if (aSurface->GetType() == SurfaceType::CAIRO) { > cairo_surface_t* surf = static_cast<SourceSurfaceCairo*>(aSurface)->GetSurface(); > cairo_surface_reference(surf); Hrm, I'd find this more readable(and less of a footgun) if we did one of two things: 1. Only do this ref inside an else clause below. 2. Call ExtractSubImage unconditionally, and have it add a reference and return aSurface if the subimage != surfacerect What do you think?

Comment on attachment 8487714 [details] [diff] [review] Part 5: Implement source clipping for DrawTargetD2D Review of attachment 8487714 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetD2D.cpp @@ +2367,5 @@ > + IntRect samplingRect = pat->mSamplingRect; > + > + RefPtr<DrawTargetD2D> dt = new DrawTargetD2D(); > + if (!dt->Init(samplingRect.Size(), > + source->GetFormat())) { MOZ_ASSERT here, this suggests the samplingRect was bigger than the max texture size and we want anyone debugging an issue to be swiftly made aware of that :)

You're right, that reference add/release around ExtractSubImage was really weird.

Comment on attachment 8487713 [details] [diff] [review] Part 4: Implement source clipping for DrawTargetSkia Review of attachment 8487713 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetSkia.cpp @@ +812,5 @@ > > #endif > > void > +DrawTargetSkia::Init(unsigned char* aData, const IntSize &aSize, int32_t aStride, SurfaceFormat aFormat){ Accident?

(In reply to George Wright (:gw280) from comment #19) > Comment on attachment 8487713 [details] [diff] [review] > Part 4: Implement source clipping for DrawTargetSkia > > Review of attachment 8487713 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/2d/DrawTargetSkia.cpp > @@ +812,5 @@ > > > > #endif > > > > void > > +DrawTargetSkia::Init(unsigned char* aData, const IntSize &aSize, int32_t aStride, SurfaceFormat aFormat){ > > Accident? Correct, will fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb44fb42459 https://hg.mozilla.org/integration/mozilla-inbound/rev/a692842b5327 https://hg.mozilla.org/integration/mozilla-inbound/rev/35c8cbf463ee https://hg.mozilla.org/integration/mozilla-inbound/rev/c395b4f2bc29 https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe172f57c0b https://hg.mozilla.org/integration/mozilla-inbound/rev/5cae10117cf6

This caused a bunch of performance regressions as far as I can tell (SVG-ASAP, Tab animation, customization animations, Tp5 scroll Tresize, Tp5 Optimized, Tp5 Optimized (XRes), etc). Is that expected?

(In reply to Boris Zbarsky [:bz] from comment #22) > This caused a bunch of performance regressions as far as I can tell > (SVG-ASAP, Tab animation, customization animations, Tp5 scroll Tresize, Tp5 > Optimized, Tp5 Optimized (XRes), etc). Is that expected? No, it's expected to cause improvements! Matt's on PTO, and I know little about the layout side implementation, so let's back it out.

https://hg.mozilla.org/mozilla-central/rev/9cb44fb42459 https://hg.mozilla.org/mozilla-central/rev/a692842b5327 https://hg.mozilla.org/mozilla-central/rev/35c8cbf463ee https://hg.mozilla.org/mozilla-central/rev/c395b4f2bc29 https://hg.mozilla.org/mozilla-central/rev/ebe172f57c0b https://hg.mozilla.org/mozilla-central/rev/5cae10117cf6