994081 - Convert imgFrame to store and act on a Moz2D SourceSurface instead of a Thebes gfxASurface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: Bas's WIP patch

In bug 950372 Bas started a patch to convert imgFrame to store and act on a Moz2D SourceSurface instead of a Thebes gfxASurface. I don't actually need that patch for bug 950372 though, so I'm spinning off this bug to continue work on that.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: Bas's WIP patch to add a Moz2D Draw function to gfxDrawable

Oh, and this patch from bug 950372 too.

Comment 2

[Michael Wu [:mwu]]

Attachment: WIP: Switch imgFrame to SourceSurfaces

This seems to work for me. Going to run through try. I didn't have to add a moz2d draw to gfxDrawable, FWIW.

Comment 4

[Michael Wu [:mwu]]

Attachment: WIP: Switch imgFrame to SourceSurfaces, v2

Now with less orange.

Comment 5

[Michael Wu [:mwu]]

Attachment: WIP: Switch imgFrame to SourceSurfaces, v3

Rebased.

Comment 6

[Michael Wu [:mwu]]

Attachment: WIP: Switch imgFrame to SourceSurfaces, v4

Hopefully with no orange now.

Comment 7

[Michael Wu [:mwu]]

Attachment: WIP: Switch imgFrame to SourceSurfaces, v5

Some single color optimization issues fixed, hopefully.

Comment 8

[Michael Wu [:mwu]]

Some general comments on what I did in this path -

A lot of the code is simplified because we stop caring about platform specific surfaces during decoding, and after decoding we simply store a reference to an optimized SourceSurface. Since we're just decompressing to DataSourceSurface, we don't have to worry about marking the surface as dirty or remembering if the format changed.

There's some perf issues I need to address after getting things green on try. The drawing path doesn't deal too well with plain DataSourceSurfaces, probably because it needs to generate a cairo/skia/etc surface wrapper every time.

Comment 9

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

Thanks so much for doing this, Michael. There's some refactoring I want to do in imagelib that will become much easier thanks to this work.

Comment 10

[Michael Wu [:mwu]]

Attachment: WIP: Switch imgFrame to SourceSurfaces, v6

Store non-premultiplied colors, bypass mImageSurface->Map/Unmap when locking image data.

Comment 11

[Michael Wu [:mwu]]

Attachment: Make VolatileBufferPtrs more flexible

This makes VolatileBufferPtrs more flexible by allowing them to use different VolatileBuffers as needed. Not very confident about the changes here, but they seem to work.

Comment 12

[Michael Wu [:mwu]]

Attachment: Add SourceSurfaceVolatileBuffer

This is a DataSourceSurface for VolatileBuffers. I used this instead of attaching data since we don't have to worry about keeping things synced with cairo surfaces and it seems like a more natural approach for C++ code.

Comment 13

[Michael Wu [:mwu]]

Attachment: Convert imgFrame to SourceSurfaces

This passes all tests on try, but I think there's still perf regressions. I'm going to address perf regressions in other patches. There's also a bunch of weird things that I do in here to workaround bugs which I'll comment on.

Comment 14

[Michael Wu [:mwu]]

Attachment: Convert imgFrame to SourceSurfaces, v2

More context, and pick up some changes that should've been in this patch.

Comment 15

[Michael Wu [:mwu]]

Comment on attachment 8421212 [details] [diff] [review]
Convert imgFrame to SourceSurfaces, v2

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

::: image/src/RasterImage.cpp
@@ +864,5 @@
> +    Factory::CreateDataSourceSurface(size, SurfaceFormat::B8G8R8A8);
> +
> +  DataSourceSurface::MappedSurface mapping;
> +  DebugOnly<bool> success =
> +    surf->Map(DataSourceSurface::MapType::WRITE, &mapping);

We explicitly use a DataSourceSurface here since some users get confused if we give them something else.. I think DrawTargetCG or DrawTargetD2D gets confused, but I don't remember which one. We also explicitly use B8G8R8A8 because there's a test on WinXP which fails if we pass surface using SurfaceFormat::B8G8R8X8.

@@ +867,5 @@
> +  DebugOnly<bool> success =
> +    surf->Map(DataSourceSurface::MapType::WRITE, &mapping);
> +  NS_ASSERTION(success, "Failed to map surface");
> +  RefPtr<DrawTarget> target =
> +    Factory::CreateDrawTargetForData(BackendType::CAIRO,

Cairo here and elsewhere is specifically used because reftests fail if we try to reconstruct single color optimized images using certain other backends.

::: image/src/imgFrame.cpp
@@ +192,5 @@
>          mSinglePixel = true;
> +        mSinglePixelColor.a = ((firstPixel >> 24) & 0xFF) * (1.0f / 255.0f);
> +        mSinglePixelColor.r = ((firstPixel >> 16) & 0xFF) * (1.0f / 255.0f);
> +        mSinglePixelColor.g = ((firstPixel >>  8) & 0xFF) * (1.0f / 255.0f);
> +        mSinglePixelColor.b = ((firstPixel >>  0) & 0xFF) * (1.0f / 255.0f);

gfx::Color doesn't support taking ARGB or unpremultiplying colors, so it's all done manually here. I can move it over to gfx::Color if there could be more users.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8421191 [details] [diff] [review]
Make VolatileBufferPtrs more flexible

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

::: memory/mozalloc/VolatileBuffer.h
@@ +97,4 @@
>      return mPurged;
>    }
>  
> +  void Set(VolatileBuffer* vbuf) {

Do you need this to be public?

@@ +105,5 @@
> +    mVBuf = vbuf;
> +    mPurged = false;
> +    if (vbuf) {
> +      mPurged = !vbuf->Lock(&mMapping);
> +    }

I'd rather DRY here, and have private functions that do the "heavy" lifting for the constructor and destructor, and have those functions called here. Or in the caller.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8421197 [details] [diff] [review]
Add SourceSurfaceVolatileBuffer

Moving review to bas

Comment 18

[Bas Schouten (:bas.schouten)]

Comment on attachment 8421197 [details] [diff] [review]
Add SourceSurfaceVolatileBuffer

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

::: gfx/2d/2D.h
@@ +24,4 @@
>  #include "mozilla/RefPtr.h"
>  
>  #include "mozilla/DebugOnly.h"
> +#include "mozilla/VolatileBuffer.h"

This is not part of MFBT so can't be used from Moz2D.

Comment 19

[Bas Schouten (:bas.schouten)]

Note if we want to do something based on the lifetime of the source surface, UserData can be used.

Comment 20

[Michael Wu [:mwu]]

Attachment: Make VolatileBufferPtrs more flexible, v2

Changes made according to review comments.

Comment 21

[Michael Wu [:mwu]]

Attachment: Convert imgFrame to SourceSurfaces, v3

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8426581 [details] [diff] [review]
Make VolatileBufferPtrs more flexible, v2

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

::: memory/mozalloc/VolatileBuffer.h
@@ +133,5 @@
> +  void operator =(VolatileBuffer* vbuf) {
> +    Set(vbuf);
> +  }
> +private:
> +  VolatileBufferPtr(VolatileBufferPtr const& vbufptr);

MOZ_DELETE

Comment 23

[Michael Wu [:mwu]]

https://tbpl.mozilla.org/?tree=Try&rev=27ce3eadb6b6

Performance actually looks mostly fine. WinXP tsvgx actually speeds up for some reason. tsvgx and tp look fine elsewhere except for tsvgx on Linux where we're much slower. Wondering if we need to provide xlib surfaces in those cases.

Comment 24

[Matt Woodrow (:mattwoodrow)]

That seems likely, tsvgx draws the same image repeatedly, and passing an image surface into xlib will do an upload. Quartz has a similar 'issue' where it will cache scaled copies of an image internally as long as we reuse the CGImage object. If we're drawing with a SourceSurfaceRawData each time then we lose that.

Comment 25

[Michael Wu [:mwu]]

Attachment: Optimize surfaces on GTK/X11

This fixes tsvgx perf on Linux.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8427896 [details] [diff] [review]
Optimize surfaces on GTK/X11

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +1090,5 @@
>  TemporaryRef<SourceSurface>
>  DrawTargetCairo::OptimizeSourceSurface(SourceSurface *aSurface) const
>  {
> +#if defined(MOZ_X11) && defined(MOZ_WIDGET_GTK)
> +  if (!static_cast<gfxPlatformGtk*>(gfxPlatform::GetPlatform())->UseXRender()) {

We can't call into gfxPlatform here since Moz2D needs to build standalone (it's used for servo among other things).

We should just use cairo_surface_get_type(mSurface) == CAIRO_SURFACE_TYPE_XLIB to decide if we should do this.

Comment 27

[Michael Wu [:mwu]]

Attachment: Optimize surfaces on GTK/X11, v2

Thanks Matt. I've eliminated all dependencies on things inside gfx/thebes.

Comment 28

[Michael Wu [:mwu]]

Try run with all patches -

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

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 8428074 [details] [diff] [review]
Optimize surfaces on GTK/X11, v2

>+#if defined(MOZ_X11) && defined(MOZ_WIDGET_GTK)
>+  if (cairo_surface_get_type(mSurface) != CAIRO_SURFACE_TYPE_XLIB) {
>+    return aSurface;

I wonder why MOZ_WIDGET_GTK is here.  I expect MOZ_X11 is sufficient.

The code looks correct in this patch, but I suspect the logic is not quite right elsewhere.

The goal is to upload to the same type of surface as destination draw target.
If imgFrame doesn't want to decide how to optimize at Draw() time, and no
parameters are provided to indicate the destination surface type, then
Optimize() needs to guess the destination type.

ScreenReferenceDrawTarget() sounds like a target for drawing to a window, but
I assume imgFrames are usually drawn to thebes/content layers.  If so, then
optimization should be to the surface type used for thebes/content layers,
rather than the type of the window.  These are currently the same on X11 with
default prefs, but Skia Thebes layers are planned (e.g. bug 738937).

gfxPlatformGtk::UseXRender() should be involved in the logic somewhere
as it was in gfxPlatform::OptimizeImage().

Comment 30

[Michael Wu [:mwu]]

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #29)
> Comment on attachment 8428074 [details] [diff] [review]
> Optimize surfaces on GTK/X11, v2
> 
> >+#if defined(MOZ_X11) && defined(MOZ_WIDGET_GTK)
> >+  if (cairo_surface_get_type(mSurface) != CAIRO_SURFACE_TYPE_XLIB) {
> >+    return aSurface;
> 
> I wonder why MOZ_WIDGET_GTK is here.  I expect MOZ_X11 is sufficient.
> 

GTK/GDK is being used to grab a Screen. I've updated the patch to grab it directly from mSurface.

> The code looks correct in this patch, but I suspect the logic is not quite
> right elsewhere.
> 
> The goal is to upload to the same type of surface as destination draw target.
> If imgFrame doesn't want to decide how to optimize at Draw() time, and no
> parameters are provided to indicate the destination surface type, then
> Optimize() needs to guess the destination type.
> 

Yeah, that's definitely an issue, especially when we're on platforms where we use different backends for content and canvas. I'm just trying to port imgFrame to moz2d without changing too much of the logic.

> ScreenReferenceDrawTarget() sounds like a target for drawing to a window, but
> I assume imgFrames are usually drawn to thebes/content layers.  If so, then
> optimization should be to the surface type used for thebes/content layers,
> rather than the type of the window.  These are currently the same on X11 with
> default prefs, but Skia Thebes layers are planned (e.g. bug 738937).
> 

ScreenReferenceDrawTarget is actually for content - it's generated by gfxPlatform::CreateOffscreenContentDrawTarget.

> gfxPlatformGtk::UseXRender() should be involved in the logic somewhere
> as it was in gfxPlatform::OptimizeImage().

I did that originally, but according to comment 26, that can't be checked. We could hack it into imgFrame::Optimize, I guess..

Comment 31

[Michael Wu [:mwu]]

Attachment: Optimize surfaces on GTK/X11, v3

Dependency on GTK/GDK eliminated.

Comment 32

[Karl Tomlinson (:karlt)]

(In reply to Michael Wu [:mwu] from comment #30)
> (In reply to Karl Tomlinson (needinfo?:karlt) from comment #29)
> > ScreenReferenceDrawTarget() sounds like a target for drawing to a window,

> ScreenReferenceDrawTarget is actually for content - it's generated by
> gfxPlatform::CreateOffscreenContentDrawTarget.

Oh, thanks.  This should be good (or at least as good as the previous code) then.

Comment 33

[Michael Wu [:mwu]]

Attachment: Make VolatileBufferPtrs more flexible, v3

Unbitrotted, added MOZ_DELETE.

Comment 34

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

Comment on attachment 8426585 [details] [diff] [review]
Convert imgFrame to SourceSurfaces, v3

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

Looks good to me!

(Also looks like I've got some rebasing ahead of me. =)

::: image/src/imgFrame.cpp
@@ +62,2 @@
>  {
> +  long stride = size.width * BytesPerPixel(format);

Why |long stride| here, but |int32_t stride| on line 48, for the result of the same expression?

@@ +206,5 @@
>    /* Figure out if the entire image is a constant color */
>  
>    // this should always be true
>    if (mImageSurface->Stride() == mSize.width * 4) {
> +    uint32_t *imgData = (uint32_t*) ((uint8_t *)mVBufPtr);

Why the double cast?

Comment 35

[Michael Wu [:mwu]]

(In reply to Seth Fowler [:seth] from comment #34)
> ::: image/src/imgFrame.cpp
> @@ +62,2 @@
> >  {
> > +  long stride = size.width * BytesPerPixel(format);
> 
> Why |long stride| here, but |int32_t stride| on line 48, for the result of
> the same expression?
> 

No idea. I'll make them consistent.

> @@ +206,5 @@
> >    /* Figure out if the entire image is a constant color */
> >  
> >    // this should always be true
> >    if (mImageSurface->Stride() == mSize.width * 4) {
> > +    uint32_t *imgData = (uint32_t*) ((uint8_t *)mVBufPtr);
> 
> Why the double cast?

The first cast is to get the pointer out of mVBufPtr, and then another cast to actually cast it to the right type. I guess VolatileBufferPtrs should have a get() method for this scenario.

Comment 36

[Michael Wu [:mwu]]

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #29)
> gfxPlatformGtk::UseXRender() should be involved in the logic somewhere
> as it was in gfxPlatform::OptimizeImage().

Now that I think about it - I think we actually have this implicitly. The offscreen surface that we check against is generated by gfxPlatformGtk::CreateOffscreenSurface, which doesn't generate an XLib surface if we're not using XRender. The set of patches here should be enough.

Comment 37

[Karl Tomlinson (:karlt)]

Yes, I misunderstood "ScreenReference".

Comment 38

[Michael Wu [:mwu]]

https://tbpl.mozilla.org/?tree=Try&rev=4b8a24a28c51

Comment 39

[Michael Wu [:mwu]]

Linux x64 ASAN fails a scaling reftest. Don't think those tests were running last time I sent these patches to try. I'm guessing that the high quality scaling path in RasterImage expects a zeroed dst buffer and the memory poisoning with ASAN breaks that assumption.

Comment 40

[Jonathan Watt [:jwatt]]

(In reply to Michael Wu [:mwu] from comment #39)
> I'm guessing that the high quality
> scaling path in RasterImage expects a zeroed dst buffer and the memory
> poisoning with ASAN breaks that assumption.

Sounds like you should include "mozilla/gfx/DataSurfaceHelpers.h" and clear the surface with ClearDataSourceSurface. (That function was only just landed.)

Comment 41

[Michael Wu [:mwu]]

It was actually the image deoptimization path in LockImageData causing problems. High quality image scaling triggers image deoptimization. That code draws with OP_OVER, but we can switch it to OP_SOURCE so the poisoned memory has no effect.

Comment 42

[Michael Wu [:mwu]]

Attachment: Convert imgFrame to SourceSurfaces, v4

Unbitrotted, image deoptimization uses OP_SOURCE where necessary.

Comment 43

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c16ab7d8eef
https://hg.mozilla.org/integration/mozilla-inbound/rev/013456bbf74d
https://hg.mozilla.org/integration/mozilla-inbound/rev/79624417d247

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c16ab7d8eef
https://hg.mozilla.org/mozilla-central/rev/013456bbf74d
https://hg.mozilla.org/mozilla-central/rev/79624417d247

Comment 45

[Michael Wu [:mwu]]

This increased Tp5 XRes by about 2.5mb, which is as expected since it moves images to the x server. No corresponding drop is visible in RSS, but the RSS numbers are too noisy to really see a 2.5mb drop.

Customization animation on OSX 10.6 takes 10% longer. There also seems to be a very slight impact on OSX 10.8. On the other hand, customization animation on Windows may have improved.