Closed Bug 959526 Opened 6 years ago Closed 6 years ago

Implement PlanarYCrCbImage::GetAsSourceSurface

Categories

(Core :: Graphics: Layers, defect)

28 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: ali, Assigned: ali)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

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
Blocks: 947194
Implement GetAsSourceSurface in:
* PlanarYCrCbImage
* BasicPlanarYCrCbImage
* SharedPlanarYCrCbImage
Attachment #8359684 - Flags: review?(nical.bugzilla)
Blocks: 960053
No longer blocks: 947194
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.
Attachment #8359684 - Flags: review?(nical.bugzilla) → review-
> 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?
Flags: needinfo?(nical.bugzilla)
Implement GetAsSourceSurface in:
* PlanarYCrCbImage
* BasicPlanarYCrCbImage
* SharedPlanarYCrCbImage
Attachment #8359684 - Attachment is obsolete: true
Attachment #8361013 - Flags: review?(nical.bugzilla)
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;
Attachment #8361013 - Flags: review?(nical.bugzilla) → review-
(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).
Flags: needinfo?(nical.bugzilla)
Implement GetAsSourceSurface in:
* PlanarYCrCbImage
* BasicPlanarYCrCbImage
* SharedPlanarYCrCbImage
Attachment #8361013 - Attachment is obsolete: true
Attachment #8362496 - Flags: review?(nical.bugzilla)
Attachment #8362496 - Flags: review?(nical.bugzilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9615b0981fe
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.