950372 - Convert imgIContainer::GetFrame to return a Moz2D SourceSurface instead of a Thebes gfxASurface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Add Moz2D Draw function to gfxDrawable (WIP)

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Bas's patch converting imgFrame (WIP)

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: jwatt's final patch - WIP - RasterImage still to be finished

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: jwatt's final patch - WIP - RasterImage still to be finished

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 8369529 [details] [diff] [review]
Bas's patch converting imgFrame (WIP)

I've spun out a separate bug (bug 994081) for this since I don't need it to finish this bug.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8369528 [details] [diff] [review]
Add Moz2D Draw function to gfxDrawable (WIP)

Same for this patch.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

Looks like I forgot to refresh my changes to RasterImage. Done now.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8403993 [details] [diff] [review]
patch

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

::: image/src/OrientedImage.cpp
@@ +97,2 @@
>    if (mOrientation.SwapsWidthAndHeight()) {
> +    Swap(width, height);

Small 'S' for swap? That's what the other callers in this file are using.

@@ +110,5 @@
>      imageFormat = gfxImageFormat::ARGB32;
>    }
>  
>    // Create a surface to draw into.
> +  mozilla::RefPtr<mozilla::gfx::DrawTarget> target =

Can drop the 'mozilla::gfx::' here.

::: image/src/RasterImage.cpp
@@ +961,5 @@
> +  // XXX  return framesurf.forget();
> +  // Convert format to SurfaceFormat::B8G8R8A8
> +  nsRefPtr<gfxImageSurface> is = framesurf->CopyToARGB32ImageSurface();
> +  return is->CopyToB8G8R8A8DataSourceSurface();
> +  //gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(nullptr, framesurf);

Why are we converting and copying here? The commented out line makes more sense.

@@ +1011,5 @@
> +                  IntRect(IntPoint(0, 0), surface->GetSize()),
> +                  IntPoint(0, 0));
> +  nsRefPtr<gfxASurface> imageSurface =
> +    gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(dt);
> +  // XXX

Again, we really want to avoid copying here. It's not really clear what's wrong with just putting 'surface' into the CairoImage::Data. What would call CreateOffscreenContentDrawTarget?

This is our optimized path to get images directly into layers, adding two extra copies into that pipeline is full of sad.

::: layout/base/nsLayoutUtils.cpp
@@ +5269,5 @@
> +        result.mSourceSurface = result.mSourceSurface->GetDataSurface();
> +        if (!result.mSourceSurface) {
> +          return result;
> +        }
> +      }

This isn't necessary. The only caller that causes wantImageSurface to get set (uses both SFE_WANT_IMAGE_SURFACE and SFE_NO_PREMULTIPLY_ALPHA) still calls GetDataSurface on the result. It isn't relying on SurfaceData::DATA, only that the data is available (which wasn't guaranteed with thebes, but is for moz2d).

We can just get rid of SFE_WANT_IMAGE_SURFACE and wantImageSurface I think.

::: widget/windows/nsImageClipboard.cpp
@@ +166,2 @@
>          options.AppendInt(24);
> +        nsAutoArrayPtr<uint8_t> data = SurfaceToPackedBGR(dataSurface);

This makes no sense to me. We weren't compressing to 24bit BGR before, why do we need to do it now?

Comment 10

[Matt Woodrow (:mattwoodrow)]

Looks like I reviewed the older version of the patch. The second half of my comments on RasterImage can be ignored. The potential format conversion to BGRA32 in ::GetFrame still seems unnecessary. Why do we need to do that now? We have lots of BGRX images, and keeping that information around should be a performance win when we blend them in software.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> Small 'S' for swap? That's what the other callers in this file are using.

Huh, interesting. They're using std::swap, whereas I was using Swap from mozilla/Move.h. I've switched to the std version now and removed the Move.h include.

> ::: layout/base/nsLayoutUtils.cpp
> @@ +5269,5 @@
> > +        result.mSourceSurface = result.mSourceSurface->GetDataSurface();
> > +        if (!result.mSourceSurface) {
> > +          return result;
> > +        }
> > +      }
> 
> This isn't necessary. The only caller that causes wantImageSurface to get
> set (uses both SFE_WANT_IMAGE_SURFACE and SFE_NO_PREMULTIPLY_ALPHA) still
> calls GetDataSurface on the result.

Looks like this is SFE_WANT_IMAGE_SURFACE __OR__ SFE_NO_PREMULTIPLY_ALPHA. I'll take a closer look tomorrow to see whether SFE_WANT_IMAGE_SURFACE can die.

> ::: widget/windows/nsImageClipboard.cpp
> @@ +166,2 @@
> >          options.AppendInt(24);
> > +        nsAutoArrayPtr<uint8_t> data = SurfaceToPackedBGR(dataSurface);
> 
> This makes no sense to me. We weren't compressing to 24bit BGR before, why
> do we need to do it now?

Will review this tomorrow.

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> > This isn't necessary. The only caller that causes wantImageSurface to get
> > set (uses both SFE_WANT_IMAGE_SURFACE and SFE_NO_PREMULTIPLY_ALPHA) still
> > calls GetDataSurface on the result.
> 
> Looks like this is SFE_WANT_IMAGE_SURFACE __OR__ SFE_NO_PREMULTIPLY_ALPHA.
> I'll take a closer look tomorrow to see whether SFE_WANT_IMAGE_SURFACE can
> die.

I sent a separate patch just to kill off this code to Try, and it causes genuine WebGL failures on OS X:

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

I won't be able to dig into this until tomorrow.

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8406048 [details] [diff] [review]
patch

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

::: layout/base/nsLayoutUtils.cpp
@@ +5416,5 @@
> +        result.mSourceSurface = result.mSourceSurface->GetDataSurface();
> +        if (!result.mSourceSurface) {
> +          return result;
> +        }
> +      }

Are you sure this is necessary? None of the callers actually check for SurfaceType::DATA, so I don't see why we need to do it.

Comment 15

[Matt Woodrow (:mattwoodrow)]

I've filed bug 996393 for fixing the issues with SurfaceFromElement.

Basically you just want to move the code from SFE up into GetFrame and everything should just work. 

The un-premultiplied piece is already contained in the flags, but I think you'll want a new flag to GetFrame that reflects that callers intentions to access the pixel data of the returned surface.

This will avoid us calling GetSourceSurfaceForSurface (which could upload to Direct2D), only to have WebGL then call GetDataSurface() and read it right back. It's not required for correctness (unlike checking the un-premultiplied flag), but is good for performance.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: interdiff to address comments

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8406786 [details] [diff] [review]
interdiff to address comments

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

::: image/src/RasterImage.cpp
@@ +959,5 @@
>      CopyFrame(aWhichFrame, aFlags, getter_AddRefs(imgsurf));
>      framesurf = imgsurf;
>    }
>  
> +  if (aFlags & FLAG_WANT_DATA_SURFACE) {

Check for non premultiplied alpha flag here too, since we *always* want to return a data surface in that case, regardless of it the user passed FLAG_WANT_DATA_SURFACE or not.

@@ +960,5 @@
>      framesurf = imgsurf;
>    }
>  
> +  if (aFlags & FLAG_WANT_DATA_SURFACE) {
> +    return gfxPlatform::GetPlatform()->GetWrappedDataSourceSurface(framesurf);

If framesurf is an optimized surface (mOptSurface in imgFrame), then it might fail the GetAsImageSurface() call within this function. In that case we want to fall through to the GetSourceSurfaceForSurface path.

That shouldn't be possible if we passed the non-premultiplied alpha flag, since we never optimize in that case.

::: layout/base/nsLayoutUtils.cpp
@@ +5395,5 @@
>      return result;
>  
>    if (!noRasterize || imgContainer->GetType() == imgIContainer::TYPE_RASTER) {
> +    if ((aSurfaceFlags & SFE_WANT_IMAGE_SURFACE) != 0 ||
> +        (aSurfaceFlags & SFE_NO_PREMULTIPLY_ALPHA) != 0) {

Can remove the check for the premultiply alpha here, and make FLAG_DECODE_NO_PREMULTIPLY_ALPHA imply FLAG_WANT_DATA_SURFACE instead.

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: interdiff to address comments

Comment 19

[Jonathan Watt [:jwatt]]

No longer depend on bug 995409 since bug 996393 should fix the <canvas> premultiplied alpha issues.

Comment 20

[Jonathan Watt [:jwatt]]

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

Comment 21

[Jonathan Watt [:jwatt]]

Follow-up for B2G KK Emulator Opt/Debug bustage (because somehow that platform includes linux/fs.h, which defines READ, which breaks Moz2D's 2D.h):

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2e5e4a6760

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a2e5e4a6760

Comment 24

[Matt Woodrow (:mattwoodrow)]

Adding ni?seth just to make sure that he saw these changes to imglib.

Seth, I'm pretty sure all the changes here are ok, but I'd be happy to do follow-ups if you have any complaints.