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

RESOLVED FIXED in mozilla31
(NeedInfo from)

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

Comment hidden (empty)
Blocks: 950556
(Assignee)

Updated

4 years ago
Depends on: 950647
Created attachment 8369528 [details] [diff] [review]
Add Moz2D Draw function to gfxDrawable (WIP)
Created attachment 8369529 [details] [diff] [review]
Bas's patch converting imgFrame (WIP)
(Assignee)

Updated

3 years ago
Depends on: 975900
(Assignee)

Updated

3 years ago
Depends on: 976371
(Assignee)

Updated

3 years ago
Depends on: 979853
(Assignee)

Updated

3 years ago
Depends on: 979883
(Assignee)

Updated

3 years ago
Depends on: 979905
(Assignee)

Updated

3 years ago
Depends on: 981430
(Assignee)

Updated

3 years ago
Depends on: 981857
Depends on: 982698
(Assignee)

Updated

3 years ago
Depends on: 982732
(Assignee)

Updated

3 years ago
Depends on: 983591
(Assignee)

Updated

3 years ago
Attachment #8369529 - Attachment description: Convert GetFrame to Moz2D (WIP) → Bas's patch converting imgFrame (WIP)
(Assignee)

Comment 3

3 years ago
Created attachment 8395124 [details] [diff] [review]
jwatt's final patch - WIP - RasterImage still to be finished
(Assignee)

Updated

3 years ago
Blocks: 987190
(Assignee)

Comment 4

3 years ago
Created attachment 8401024 [details] [diff] [review]
jwatt's final patch - WIP - RasterImage still to be finished
Attachment #8395124 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 960524
Depends on: 993784
(Assignee)

Comment 5

3 years ago
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.
Attachment #8369529 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Same for this patch.
Attachment #8369528 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8403993 [details] [diff] [review]
patch
Assignee: nobody → jwatt
Attachment #8401024 - Attachment is obsolete: true
Attachment #8403993 - Flags: review?(matt.woodrow)
(Assignee)

Updated

3 years ago
No longer depends on: 993784
(Assignee)

Comment 8

3 years ago
Created attachment 8404296 [details] [diff] [review]
patch

Looks like I forgot to refresh my changes to RasterImage. Done now.
Attachment #8403993 - Attachment is obsolete: true
Attachment #8403993 - Flags: review?(matt.woodrow)
Attachment #8404296 - Flags: review?(matt.woodrow)
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?
Attachment #8403993 - Attachment is obsolete: false
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.
(Assignee)

Comment 11

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

Comment 12

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

Updated

3 years ago
Depends on: 995409
(Assignee)

Updated

3 years ago
Depends on: 995832
(Assignee)

Comment 13

3 years ago
Created attachment 8406048 [details] [diff] [review]
patch
Attachment #8403993 - Attachment is obsolete: true
Attachment #8404296 - Attachment is obsolete: true
Attachment #8404296 - Flags: review?(matt.woodrow)
Attachment #8406048 - Flags: review?(matt.woodrow)
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.
Attachment #8406048 - Flags: review?(matt.woodrow) → review+
Depends on: 996393
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.
(Assignee)

Comment 16

3 years ago
Created attachment 8406786 [details] [diff] [review]
interdiff to address comments
Attachment #8406786 - Flags: review?(matt.woodrow)
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.
(Assignee)

Comment 18

3 years ago
Created attachment 8406791 [details] [diff] [review]
interdiff to address comments
Attachment #8406786 - Attachment is obsolete: true
Attachment #8406786 - Flags: review?(matt.woodrow)
Attachment #8406791 - Flags: review?(matt.woodrow)
Attachment #8406791 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 19

3 years ago
No longer depend on bug 995409 since bug 996393 should fix the <canvas> premultiplied alpha issues.
No longer depends on: 995409
(Assignee)

Updated

3 years ago
Depends on: 996673
(Assignee)

Updated

3 years ago
Depends on: 996687
(Assignee)

Comment 20

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a54345bfd338
(Assignee)

Updated

3 years ago
Blocks: 902952
(Assignee)

Updated

3 years ago
Depends on: 996929
(Assignee)

Comment 21

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/a54345bfd338
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/1a2e5e4a6760

Updated

3 years ago
Depends on: 997369
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.
Flags: needinfo?(seth)
(Assignee)

Updated

3 years ago
Blocks: 1004500
(Assignee)

Updated

3 years ago
Depends on: 1011865
No longer depends on: 997369
No longer blocks: 902952
You need to log in before you can comment on or make changes to this bug.