Closed Bug 1044702 Opened 6 years ago Closed 5 years ago

Use Moz2D source clipping when possible

Categories

(Core :: Graphics, defect)

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Assignee: nobody → matt.woodrow
Attachment #8463179 - Flags: review?(bas)
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
Attachment #8463179 - Flags: review?(bas) → review?(seth)
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.
Attachment #8463179 - Flags: review+
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)
Attachment #8463180 - Flags: review?(bas) → review+
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.
Attachment #8463179 - Flags: review?(seth) → review+
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.
Keywords: leave-open
Depends on: 1062723
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
Attachment #8463181 - Attachment is obsolete: true
Attachment #8487717 - Flags: review?(bas)
Attachment #8487717 - Flags: review?(bas) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/c232720a2847
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Did this cause bug 1067311? Were crashing in this new code :(
Depends on: 1067311
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.)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bas)
Depends on: 1067588
I thought we were backing it out for perf regressions anyway?
Flags: needinfo?(bas)
Depends on: 1068565
(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?
Flags: needinfo?(bas)
Depends on: 1069341
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
Flags: needinfo?(bas)
(In reply to Bas Schouten (:bas.schouten) from comment #17)
> (In reply to Robert Kaiser (:kairo@mozilla.com) 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?
Flags: needinfo?(bas)
(In reply to Benjamin Kerensa [:bkerensa] from comment #18)
> (In reply to Bas Schouten (:bas.schouten) from comment #17)
> > (In reply to Robert Kaiser (:kairo@mozilla.com) 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.
Flags: needinfo?(bas)
Depends on: 1075467
Depends on: 1075616
Depends on: 1076725
Depends on: 1077434
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.