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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jwatt, Assigned: mwu)

Tracking

(Depends on 1 bug)

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 14 obsolete attachments)

37.02 KB, patch
Details | Diff | Splinter Review
4.55 KB, patch
Details | Diff | Splinter Review
4.87 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.67 KB, patch
Details | Diff | Splinter Review
82.87 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 2

5 years ago
This seems to work for me. Going to run through try. I didn't have to add a moz2d draw to gfxDrawable, FWIW.
Assignee: nobody → mwu
(Assignee)

Updated

5 years ago
Duplicate of this bug: 999266
(Assignee)

Comment 4

5 years ago
Now with less orange.
Attachment #8409966 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Rebased.
Attachment #8410690 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Hopefully with no orange now.
Attachment #8410697 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Some single color optimization issues fixed, hopefully.
Attachment #8411230 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
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.
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.
(Assignee)

Comment 10

5 years ago
Store non-premultiplied colors, bypass mImageSurface->Map/Unmap when locking image data.
Attachment #8412202 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
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.
Attachment #8421191 - Flags: review?(mh+mozilla)
(Assignee)

Comment 12

5 years ago
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.
Attachment #8421197 - Flags: review?(jmuizelaar)
(Assignee)

Comment 13

5 years ago
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.
Attachment #8413032 - Attachment is obsolete: true
Attachment #8421198 - Flags: review?(seth)
(Assignee)

Comment 14

5 years ago
More context, and pick up some changes that should've been in this patch.
Attachment #8421198 - Attachment is obsolete: true
Attachment #8421198 - Flags: review?(seth)
Attachment #8421212 - Flags: review?(seth)
(Assignee)

Comment 15

5 years ago
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 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.
Attachment #8421191 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8421197 [details] [diff] [review]
Add SourceSurfaceVolatileBuffer

Moving review to bas
Attachment #8421197 - Flags: review?(jmuizelaar) → review?(bas)
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.
Attachment #8421197 - Flags: review?(bas) → review-
Note if we want to do something based on the lifetime of the source surface, UserData can be used.
(Assignee)

Comment 20

5 years ago
Changes made according to review comments.
Attachment #8421191 - Attachment is obsolete: true
Attachment #8426581 - Flags: review?(mh+mozilla)
(Assignee)

Comment 21

5 years ago
Attachment #8421197 - Attachment is obsolete: true
Attachment #8421212 - Attachment is obsolete: true
Attachment #8421212 - Flags: review?(seth)
Attachment #8426585 - Flags: review?(seth)
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
Attachment #8426581 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 23

5 years ago
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.
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.
(Assignee)

Comment 25

5 years ago
Posted patch Optimize surfaces on GTK/X11 (obsolete) — Splinter Review
This fixes tsvgx perf on Linux.
Attachment #8427896 - Flags: review?(bas)
Attachment #8427896 - Flags: feedback?(karlt)
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.
(Assignee)

Comment 27

5 years ago
Thanks Matt. I've eliminated all dependencies on things inside gfx/thebes.
Attachment #8427896 - Attachment is obsolete: true
Attachment #8427896 - Flags: review?(bas)
Attachment #8427896 - Flags: feedback?(karlt)
Attachment #8428074 - Flags: review?(bas)
Attachment #8428074 - Flags: feedback?(karlt)
(Assignee)

Comment 28

5 years ago
Try run with all patches -

https://tbpl.mozilla.org/?tree=Try&rev=f58f694c00d1
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().
Attachment #8428074 - Flags: feedback?(karlt) → feedback+
(Assignee)

Comment 30

5 years ago
(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..
(Assignee)

Comment 31

5 years ago
Dependency on GTK/GDK eliminated.
Attachment #8428074 - Attachment is obsolete: true
Attachment #8428074 - Flags: review?(bas)
Attachment #8428801 - Flags: review?(bas)
(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.
(Assignee)

Comment 33

5 years ago
Unbitrotted, added MOZ_DELETE.
Attachment #8426581 - Attachment is obsolete: true
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?
Attachment #8426585 - Flags: review?(seth) → review+
(Assignee)

Comment 35

5 years ago
(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.
(Assignee)

Comment 36

5 years ago
(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.
Yes, I misunderstood "ScreenReference".
Attachment #8428801 - Flags: review?(bas) → review+
(Assignee)

Comment 39

5 years ago
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.
(Reporter)

Comment 40

5 years ago
(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.)
(Assignee)

Comment 41

5 years ago
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.
(Assignee)

Comment 42

5 years ago
Unbitrotted, image deoptimization uses OP_SOURCE where necessary.
Attachment #8426585 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
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.
Depends on: 1022730
(Reporter)

Updated

5 years ago
Depends on: 1022930
Depends on: 1023194

Updated

5 years ago
Depends on: 1025652
(Assignee)

Updated

5 years ago
Blocks: 1062886

Updated

5 years ago
Depends on: 1070772

Updated

5 years ago
Depends on: 1081926

Updated

3 years ago
Depends on: 1242256

Updated

3 years ago
Depends on: 1254788
Blocks: 591822
You need to log in before you can comment on or make changes to this bug.