1044702 - Use Moz2D source clipping when possible

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Moz2D offers source clipping that covers some of the cases that we currently handle using CreateSamplingRestrictedDrawable.

We also previously had an optimization where we used a subimage instead of copying, that was broken by the moz2d conversion. Using Moz2D should let us get that performance back again.

Bug 1043560 simplifies the calling code a lot, so I'll base work on top of that.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove no longer needed gfxDrawable constructors

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Convert gfxSurfaceDrawable::Draw to moz2d

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part3: [WIP] Use Moz2D source clipping when possible

This is based on what I assume the needed moz2d API changes will (approximately) be like, and still needs to be rebased on top of bug 1043560

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8463179 [details] [diff] [review]
Part 1: Remove no longer needed gfxDrawable constructors

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

Man, doesn't this just fill you with joy?

::: gfx/thebes/gfxDrawable.h
@@ +62,5 @@
>                          const gfxRect& aFillRect,
>                          bool aRepeat,
>                          const GraphicsFilter& aFilter,
>                          const gfxMatrix& aTransform = gfxMatrix());
>      

nit: kill this whitespace while you're at it.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8463180 [details] [diff] [review]
Part 2: Convert gfxSurfaceDrawable::Draw to moz2d

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

::: gfx/thebes/gfxDrawable.cpp
@@ -123,5 @@
> -            aTransform.HasOnlyIntegerTranslation())
> -        {
> -          // If we only have integer translation, no special filtering needs to
> -          // happen and we explicitly use FILTER_FAST. This is fast for some
> -          // backends.

We're dropping this fast path. I wonder if that's going to hurt us anywhere. Perhaps we might as well keep it? (Using Filter::Point)

Comment 6

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8463179 [details] [diff] [review]
Part 1: Remove no longer needed gfxDrawable constructors

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

Can't wait for this stuff to land!

::: gfx/thebes/gfxDrawable.cpp
@@ +112,5 @@
>                           const GraphicsFilter& aFilter,
>                           const gfxMatrix& aTransform)
>  {
> +    nsRefPtr<gfxPattern> pattern = new gfxPattern(mSourceSurface, Matrix());
> +    

Looks like there's some stray whitespace here.

::: gfx/thebes/gfxUtils.cpp
@@ +412,5 @@
> +    aDrawable->Draw(tmpCtx, needed - needed.TopLeft(), true,
> +                    GraphicsFilter::FILTER_FAST, gfxMatrix().Translate(needed.TopLeft()));
> +    RefPtr<SourceSurface> surface = target->Snapshot();
> +
> +    return new gfxSurfaceDrawable(surface, size, gfxMatrix().Translate(-needed.TopLeft()));

This function returns an already_AddRefed<gfxDrawable>, so it's not safe to return the result of |new| directly here. (Indeed, does this compile? It looks like the raw pointer constructor of already_AddRefed<T> is explicit except on B2G.) The easiest solution is probably to switch back to using |forget()|.

::: image/src/VectorImage.cpp
@@ +933,5 @@
>                               ThebesIntRect(aParams.imageRect),
>                               ThebesIntRect(aParams.imageRect),
>                               SurfaceFormat::B8G8R8A8,
>                               GraphicsFilter::FILTER_NEAREST, aParams.flags);
> +  

A little more whitespace here.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8463180 [details] [diff] [review]
Part 2: Convert gfxSurfaceDrawable::Draw to moz2d

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

::: gfx/thebes/gfxDrawable.cpp
@@ -123,5 @@
> -            aTransform.HasOnlyIntegerTranslation())
> -        {
> -          // If we only have integer translation, no special filtering needs to
> -          // happen and we explicitly use FILTER_FAST. This is fast for some
> -          // backends.

I think we dropped this quite a while ago. FILTER_FAST maps to Filter::LINEAR using http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfx2DGlue.h#89

We could bring it back, but that's probably a separate bug.

Comment 8

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf57c5019172
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4894e77370

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf57c5019172
https://hg.mozilla.org/mozilla-central/rev/2d4894e77370

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Use Moz2D source clipping when possible

This should improve correctness on mobile (since we don't do CSRD there), and performance for desktop platforms that support it efficiently (OSX, GDI, Direct2D 1.1).

https://tbpl.mozilla.org/?tree=Try&rev=301cc4346b13

Comment 11

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c232720a2847

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c232720a2847

Comment 13

[Johannes Pfrang [:johnp]]

Did this cause bug 1067311? Were crashing in this new code :(

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Bas, is it worth backing this out to check if it's the cause for bug 1067311? (I understand Matt is on PTO, so asking the reviewer.)

Comment 15

[Bas Schouten (:bas.schouten)]

I thought we were backing it out for perf regressions anyway?

Comment 16

[Robert Kaiser]

(In reply to Bas Schouten (:bas.schouten) from comment #15)
> I thought we were backing it out for perf regressions anyway?

Doesn't look like anyone has done that. Given that bug 1067311 is the largest stability issue on Nightly atm, can you please make sure some action happens before the weekend?

Comment 17

[Bas Schouten (:bas.schouten)]

(In reply to Robert Kaiser (:[e-mail]) from comment #16)
> (In reply to Bas Schouten (:bas.schouten) from comment #15)
> > I thought we were backing it out for perf regressions anyway?
> 
> Doesn't look like anyone has done that. Given that bug 1067311 is the
> largest stability issue on Nightly atm, can you please make sure some action
> happens before the weekend?

I'll back it out later tonight or tomorrow.

Comment 18

[Benjamin Kerensa [:bkerensa]]

(In reply to Bas Schouten (:bas.schouten) from comment #17)
> (In reply to Robert Kaiser (:[e-mail]) from comment #16)
> > (In reply to Bas Schouten (:bas.schouten) from comment #15)
> > > I thought we were backing it out for perf regressions anyway?
> > 
> > Doesn't look like anyone has done that. Given that bug 1067311 is the
> > largest stability issue on Nightly atm, can you please make sure some action
> > happens before the weekend?
> 
> I'll back it out later tonight or tomorrow.

You still backing this out?

Comment 19

[Bas Schouten (:bas.schouten)]

(In reply to Benjamin Kerensa [:bkerensa] from comment #18)
> (In reply to Bas Schouten (:bas.schouten) from comment #17)
> > (In reply to Robert Kaiser (:[e-mail]) from comment #16)
> > > (In reply to Bas Schouten (:bas.schouten) from comment #15)
> > > > I thought we were backing it out for perf regressions anyway?
> > > 
> > > Doesn't look like anyone has done that. Given that bug 1067311 is the
> > > largest stability issue on Nightly atm, can you please make sure some action
> > > happens before the weekend?
> > 
> > I'll back it out later tonight or tomorrow.
> 
> You still backing this out?

Matt Woodrow is back and working on this.