959526 - Implement PlanarYCrCbImage::GetAsSourceSurface

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ali Ak]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

Implement PlanarYCrCbImage::GetAsSourceSurface and in its derived classes to do what PlanarYCrCbImage::DeprecatedGetAsSurface does for a SourceSurface

Comment 1

[Ali Ak]

Attachment: Implement PlanarYCrCbImage and derived classes GetAsSourceSurface

Implement GetAsSourceSurface in:
* PlanarYCrCbImage
* BasicPlanarYCrCbImage
* SharedPlanarYCrCbImage

Comment 2

[Ali Ak]

try: https://tbpl.mozilla.org/?tree=Try&rev=539fb37fa6fb

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8359684 [details] [diff] [review]
Implement PlanarYCrCbImage and derived classes GetAsSourceSurface

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

::: gfx/layers/ImageContainer.cpp
@@ +666,5 @@
> +    return mSourceSurface.get();
> +  }
> +
> +  gfx::SurfaceFormat format = gfx::ImageFormatToSurfaceFormat(GetOffscreenFormat());
> +  gfx::IntSize size(mSize);

I think this is a left over from when this code used a mix of gfxIntSize and IntSize. Now you should drop the size variable and just use mSize.

::: gfx/layers/basic/BasicImages.cpp
@@ +194,5 @@
> +  if (!drawTarget) {
> +    return nullptr;
> +  }
> +
> +  mRecycleBin->RecycleBuffer(mDecodedBuffer.forget(), mSize.height * mStride);

Important: You need to make sure that the DrawTarget dies before recycling the buffer, because recycling may or may not delete the buffer, and the DrawTarget's destruction is what will trigger the copy in the snapshot.

@@ +196,5 @@
> +  }
> +
> +  mRecycleBin->RecycleBuffer(mDecodedBuffer.forget(), mSize.height * mStride);
> +
> +  RefPtr<SourceSurface> surface = drawTarget->Snapshot();

This looks weird: We are not drawing anything here, just getting a SourceSurface, so we should not be using a DrawTarget. Ok after digging a bit it looks like you did this because DrawTarget conveniently handles the copy if it dies before its snapshot. Then if you want to do this, add a small comment explaining why you use a DrawTarget.

Comment 4

[Ali Ak]

> This looks weird: We are not drawing anything here, just getting a
> SourceSurface, so we should not be using a DrawTarget. Ok after digging a
> bit it looks like you did this because DrawTarget conveniently handles the
> copy if it dies before its snapshot. Then if you want to do this, add a
> small comment explaining why you use a DrawTarget.

Yeah it feels weird :p

I was thinking of maybe using CreateWrappingDataSourceSurface to wrap mDecodeBuffer. But then the problem is that the destructor of BasicPlanarYCbCrImage recycles mDecodeBuffer. So returning a wrapped SourceSurface seems a bit unsafe.

Or unless I'm missing a better way to do this?

Comment 5

[Ali Ak]

Attachment: Implement PlanarYCrCbImage and derived classes GetAsSourceSurface. r=nical.

Implement GetAsSourceSurface in:
* PlanarYCrCbImage
* BasicPlanarYCrCbImage
* SharedPlanarYCrCbImage

Comment 6

[Ali Ak]

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

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8361013 [details] [diff] [review]
Implement PlanarYCrCbImage and derived classes GetAsSourceSurface. r=nical.

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

You need to fix the RecycleBuffer business below, the rest looks good

::: gfx/layers/basic/BasicImages.cpp
@@ +198,5 @@
> +  if (!drawTarget) {
> +    return nullptr;
> +  }
> +
> +  mRecycleBin->RecycleBuffer(mDecodedBuffer.forget(), mSize.height * mStride);

RecycleBuffer may delete the buffer, so you need to do it after the destruction of the DrawTarget, because it is DrawTarget's destructor that will trigger the copy into the snapshot (before that, the snapshot is just pointing to the DrawTarget's data).
You can just add a scope like so:

{
  Refptr<DrawTarget> dt = create the draw target;
  surface = dt->Snapshot();
} // copy happens here
RecycleBuffer; // now we can recycle the data
return surface;

Comment 8

[Nicolas Silva [:nical]]

(In reply to Ali Ak from comment #4)
> > This looks weird: We are not drawing anything here, just getting a
> > SourceSurface, so we should not be using a DrawTarget. Ok after digging a
> > bit it looks like you did this because DrawTarget conveniently handles the
> > copy if it dies before its snapshot. Then if you want to do this, add a
> > small comment explaining why you use a DrawTarget.
> 
> Yeah it feels weird :p
> 
> I was thinking of maybe using CreateWrappingDataSourceSurface to wrap
> mDecodeBuffer. But then the problem is that the destructor of
> BasicPlanarYCbCrImage recycles mDecodeBuffer. So returning a wrapped
> SourceSurface seems a bit unsafe.
> 
> Or unless I'm missing a better way to do this?

Yeah, you can't return a wrapping surface of the buffer since the wrapping surface doesn't own it's data. I think what you did with a little comment to explain why, is good (and making sure RecycleBuffer happens after the DrawTarget's destructor).

Comment 9

[Ali Ak]

Attachment: Implement PlanarYCrCbImage and derived classes GetAsSourceSurface. r=nical.

Implement GetAsSourceSurface in:
* PlanarYCrCbImage
* BasicPlanarYCrCbImage
* SharedPlanarYCrCbImage

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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