Last Comment Bug 950372 - Convert imgIContainer::GetFrame to return a Moz2D SourceSurface instead of a Thebes gfxASurface
: Convert imgIContainer::GetFrame to return a Moz2D SourceSurface instead of a ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla31
Assigned To: Jonathan Watt [:jwatt]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 976371 944442 950368 950371 950647 960524 975900 979853 979883 979905 981430 981857 982698 982732 983591 995832 996393 996673 996687 996929 1011865
Blocks: 703159 Moz2Dification 950556 987190 1004500
  Show dependency treegraph
 
Reported: 2013-12-14 10:18 PST by Jonathan Watt [:jwatt]
Modified: 2014-08-31 13:19 PDT (History)
6 users (show)
matt.woodrow: needinfo? (seth.bugzilla)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add Moz2D Draw function to gfxDrawable (WIP) (4.55 KB, patch)
2014-02-03 09:47 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Bas's patch converting imgFrame (WIP) (46.39 KB, patch)
2014-02-03 09:48 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
jwatt's final patch - WIP - RasterImage still to be finished (61.46 KB, patch)
2014-03-21 17:22 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
jwatt's final patch - WIP - RasterImage still to be finished (62.03 KB, patch)
2014-04-02 18:09 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch (62.40 KB, patch)
2014-04-09 07:06 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch (62.32 KB, patch)
2014-04-09 15:31 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch (60.64 KB, patch)
2014-04-14 03:10 PDT, Jonathan Watt [:jwatt]
matt.woodrow: review+
Details | Diff | Splinter Review
interdiff to address comments (2.96 KB, patch)
2014-04-15 04:45 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
interdiff to address comments (3.64 KB, patch)
2014-04-15 05:06 PDT, Jonathan Watt [:jwatt]
matt.woodrow: review+
Details | Diff | Splinter Review

Description User image Jonathan Watt [:jwatt] 2013-12-14 10:18:48 PST

    
Comment 1 User image Bas Schouten (:bas.schouten) 2014-02-03 09:47:26 PST
Created attachment 8369528 [details] [diff] [review]
Add Moz2D Draw function to gfxDrawable (WIP)
Comment 2 User image Bas Schouten (:bas.schouten) 2014-02-03 09:48:12 PST
Created attachment 8369529 [details] [diff] [review]
Bas's patch converting imgFrame (WIP)
Comment 3 User image Jonathan Watt [:jwatt] 2014-03-21 17:22:59 PDT
Created attachment 8395124 [details] [diff] [review]
jwatt's final patch - WIP - RasterImage still to be finished
Comment 4 User image Jonathan Watt [:jwatt] 2014-04-02 18:09:25 PDT
Created attachment 8401024 [details] [diff] [review]
jwatt's final patch - WIP - RasterImage still to be finished
Comment 5 User image Jonathan Watt [:jwatt] 2014-04-09 07:02:29 PDT
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 User image Jonathan Watt [:jwatt] 2014-04-09 07:05:55 PDT
Comment on attachment 8369528 [details] [diff] [review]
Add Moz2D Draw function to gfxDrawable (WIP)

Same for this patch.
Comment 7 User image Jonathan Watt [:jwatt] 2014-04-09 07:06:28 PDT
Created attachment 8403993 [details] [diff] [review]
patch
Comment 8 User image Jonathan Watt [:jwatt] 2014-04-09 15:31:30 PDT
Created attachment 8404296 [details] [diff] [review]
patch

Looks like I forgot to refresh my changes to RasterImage. Done now.
Comment 9 User image Matt Woodrow (:mattwoodrow) 2014-04-09 16:27:21 PDT
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 User image Matt Woodrow (:mattwoodrow) 2014-04-09 16:30:21 PDT
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 User image Jonathan Watt [:jwatt] 2014-04-09 17:27:53 PDT
(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 User image Jonathan Watt [:jwatt] 2014-04-10 10:44:10 PDT
(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 User image Jonathan Watt [:jwatt] 2014-04-14 03:10:53 PDT
Created attachment 8406048 [details] [diff] [review]
patch
Comment 14 User image Matt Woodrow (:mattwoodrow) 2014-04-14 04:11:04 PDT
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 User image Matt Woodrow (:mattwoodrow) 2014-04-14 19:54:30 PDT
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 User image Jonathan Watt [:jwatt] 2014-04-15 04:45:18 PDT
Created attachment 8406786 [details] [diff] [review]
interdiff to address comments
Comment 17 User image Matt Woodrow (:mattwoodrow) 2014-04-15 04:52:19 PDT
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 User image Jonathan Watt [:jwatt] 2014-04-15 05:06:50 PDT
Created attachment 8406791 [details] [diff] [review]
interdiff to address comments
Comment 19 User image Jonathan Watt [:jwatt] 2014-04-15 06:27:32 PDT
No longer depend on bug 995409 since bug 996393 should fix the <canvas> premultiplied alpha issues.
Comment 21 User image Jonathan Watt [:jwatt] 2014-04-15 17:50:31 PDT
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 User image Ryan VanderMeulen [:RyanVM] 2014-04-15 20:32:17 PDT
https://hg.mozilla.org/mozilla-central/rev/a54345bfd338
Comment 23 User image Carsten Book [:Tomcat] 2014-04-16 06:23:23 PDT
https://hg.mozilla.org/mozilla-central/rev/1a2e5e4a6760
Comment 24 User image Matt Woodrow (:mattwoodrow) 2014-04-16 17:28:22 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.