Closed Bug 1282496 Opened 4 years ago Closed 4 years ago

memset unoptimized images with RGBX surface format to 0xFF when using the skia backend

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(1 file, 6 obsolete files)

No description provided.
Whiteboard: gfx-noted
Comment on attachment 8765519 [details] [diff] [review]
Memset RGBX surfaces to white

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

The concept looks great! Some tweaks are necessary before this can land, though. I'm unsetting the review flag; please re-set it when you've made the necessary changes.

::: image/decoders/nsGIFDecoder2.cpp
@@ +891,5 @@
>      const size_t size = 3 << depth;
>      if (mColormapSize > size) {
>        // Clear the part of the colormap which will be unused with this palette.
> +      // Clear it to 0xFF since Skia requires RGBX values to be 0xFF alpha
> +      // Better to overwrite bad gifs with opaque white.

This comment needs some rewording.

First, a nit: put periods on the ends of your sentences.

Second: what's happening here is not that you're overwriting bad GIFs. What's happening is that you're ensuring that if the GIF reads an invalid palette entry, that palette entry will be opaque white, which is necessary because if it isn't opaque then it can cause problems with RGBX surfaces on Skia. Please update the comment so the reader will understand this.

::: image/imgFrame.cpp
@@ +82,5 @@
> +      // Skia doesn't support RGBX surfaces, so ensure the alpha value is set
> +      // to opaque white.
> +      memset(vbufptr, 0xFF, stride * size.height);
> +    } else if (buf->OnHeap()) {
> +      memset(vbufptr, 0, stride * size.height);

Please add a comment here explaining that we only need to zero out the buffer if it's on the heap because if not, it was allocated via mmap(), and the OS zero'd it.

@@ +835,5 @@
>      // If we're using a surface format with alpha but the image has no alpha,
>      // change the format. This doesn't change the underlying data at all, but
>      // allows DrawTargets to avoid blending when drawing known opaque images.
> +    if (mHasNoAlpha && mFormat == SurfaceFormat::B8G8R8A8 && mImageSurface &&
> +    (gfxPlatform::GetPlatform()->GetDefaultContentBackend() != BackendType::SKIA)) {

This will cause crashes. You can't assume that gfxPlatform is available in this method; we could be getting called after shutdown has started.

The best bet is probably to move this block of code into imgFrame::Optimize(), maybe right here:

https://dxr.mozilla.org/mozilla-central/source/image/imgFrame.cpp#347

That is, after the ShutdownTracker check, which is absolutely required, but before the |if (!mOptimizable || gDisableOptimize)| check. You should probably add a comment explaining that this optimization is free when safe and so we always do it. (You could also put it after that check, but then we'll stop doing this optimization for multipart/x-mixed-replace images and the like, and given that it *is* free I think we're better off continuing to do it.)

Also, please figure out a way to format this so that you don't have to shift the second line of the |if| condition to the left of the first line. I'd *vastly* prefer going over 80 columns to something like this, which could confuse someone who isn't reading closely.

@@ +837,5 @@
>      // allows DrawTargets to avoid blending when drawing known opaque images.
> +    if (mHasNoAlpha && mFormat == SurfaceFormat::B8G8R8A8 && mImageSurface &&
> +    (gfxPlatform::GetPlatform()->GetDefaultContentBackend() != BackendType::SKIA)) {
> +      // On skia platforms, RGBX isn't supported so ensure we don't optimize to an RGBX
> +      // surface

Nit: Please put periods on the end of your sentences.

Bonus nit: Please capitalize Skia, or at least be consistent about it within the same patch. =)
Attachment #8765519 - Flags: review?(seth)
Attached patch Memset RGBX surfaces to white (obsolete) — Splinter Review
Thanks for the quick review, let's try again.
Attachment #8765519 - Attachment is obsolete: true
Attachment #8765611 - Flags: review?(seth)
Comment on attachment 8765611 [details] [diff] [review]
Memset RGBX surfaces to white

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

Looks good, man. Thanks for those fixes. r+ with some additional fixes below. =)

::: image/decoders/nsGIFDecoder2.cpp
@@ +892,5 @@
>      if (mColormapSize > size) {
>        // Clear the part of the colormap which will be unused with this palette.
> +      // If a gif references an invalid palette entry, ensure the entry is opaque white.
> +      // This is needed for Skia as if it isn't, RGBX surfaces will cause problems
> +      // with Skia.

I realize I'm being OCD here, but I can guarantee that nobody will remember what the heck this is about in six months. So:

Nit 1: Instead of saying "will cause problems with Skia", say something like "will cause blending issues with Skia, which doesn't really support RGBX surfaces."

Nit 2: Please capitalize GIF.

::: image/imgFrame.cpp
@@ +83,5 @@
> +      // to opaque white.
> +      memset(vbufptr, 0xFF, stride * size.height);
> +    } else if (buf->OnHeap()) {
> +      // We only need to memset it if the buffer was allocated on the heap.
> +      // Otherwise, it's allocated via mmaped and refers to a zeroed page and will

s/mmaped/mmap

@@ +330,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +imgFrame::CanOptimizeOpaqueImage()

You're touching data which is guarded by |mMonitor| in this method. (See imgFrame.h for a list of which members are guarded by |mMonitor| and which aren't.) Additionally, gfxPlatform is not available during shutdown, and ShutdownTracker is unsafe to access off-main-thread. That means that at the beginning of this method, you need this code:

MOZ_ASSERT(NS_IsMainThread());
MOZ_ASSERT(!ShutdownTracker::ShutdownHasStarted());
mMonitor.AssertCurrentThreadOwns();

@@ +365,5 @@
>    if (ShutdownTracker::ShutdownHasStarted()) {
>      return NS_OK;
>    }
>  
> +  if (CanOptimizeOpaqueImage()) {

Please add a comment saying something brief like "// This optimization is basically free, so we perform it even if optimization is disabled."

@@ +370,5 @@
> +    mFormat = SurfaceFormat::B8G8R8X8;
> +    mImageSurface = CreateLockedSurface(mVBuf, mFrameRect.Size(), mFormat);
> +  }
> +
> +

Nit: You've got an extra blank line here.
Attachment #8765611 - Flags: review?(seth) → review+
Attached patch Memset RGBX surfaces to white (obsolete) — Splinter Review
Carrying r+, updated with review feedback. Try looking good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=e42b3636c015
Attachment #8765611 - Attachment is obsolete: true
Attachment #8765678 - Flags: review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/269a48e67579
memset unoptimized images with RGBX surface format to 0xFF when using the skia backend. r=seth
Attached patch Memset RGBX surfaces to white (obsolete) — Splinter Review
The crashes were happening because the first VolatileBufferPtr used is the only one guaranteed not to be purged. Looks like on Windows 8, between allocating the memory and creating a locked surface out of it, the image got purged.
Attachment #8765678 - Attachment is obsolete: true
Attachment #8766047 - Flags: review?(seth)
Comment on attachment 8766047 [details] [diff] [review]
Memset RGBX surfaces to white

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

Almost there. =)

::: image/imgFrame.cpp
@@ +59,5 @@
> +  if ((format == SurfaceFormat::B8G8R8X8) &&
> +        (gfxPlatform::GetPlatform()->GetDefaultContentBackend() == BackendType::SKIA)) {
> +      // Skia doesn't support RGBX surfaces, so ensure the alpha value is set
> +      // to opaque white.
> +      memset(*vbufptr, 0xFF, stride * size.height);

You can't do this...

@@ +371,5 @@
>  
> +  // This optimization is basically free, so we perform it even if optimization is disabled.
> +  if (CanOptimizeOpaqueImage()) {
> +    mFormat = SurfaceFormat::B8G8R8X8;
> +    mImageSurface = CreateLockedSurface(mVBuf, mFrameRect.Size(), mFormat);

...because of this.

Instead you need to do your memset here:

https://dxr.mozilla.org/mozilla-central/source/image/imgFrame.cpp?q=path%3AimgFrame.cpp&redirect_type=single#223

At that point you know the volatile buffer is locked.

This is why the BMP tests are failing; this patch broke optimization to RGBX.
Attachment #8766047 - Flags: review?(seth)
Attached patch Memset RGBX surfaces to white (obsolete) — Splinter Review
Attachment #8766047 - Attachment is obsolete: true
Attachment #8766593 - Flags: review?(seth)
Comment on attachment 8766593 [details] [diff] [review]
Memset RGBX surfaces to white

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

Still needs some work. I'm sorry this is so complex. =\

::: image/imgFrame.cpp
@@ +55,5 @@
>    MOZ_ASSERT(!vbufptr->WasBufferPurged(), "Expected image data!");
>  
>    int32_t stride = VolatileSurfaceStride(size, format);
>  
> +    // The VolatileBufferPtr is held by this DataSourceSurface.

I don't think you meant to change this line, right? (And the change you made was to mess up the indentation. =)

@@ +82,5 @@
>    return nullptr;
>  }
>  
> +static void
> +InitVolatileBuffer(VolatileBuffer* vbuf, const IntSize& size, SurfaceFormat format)

Use "aVBuf", "aSize", "aFormat". Read the style guide yo. =)

Also, this function name is too vague. I don't think the "VolatileBuffer" part is really the important part. Maybe "ClearSurface"? Whatever you want but consider something a little more specific.

@@ +86,5 @@
> +InitVolatileBuffer(VolatileBuffer* vbuf, const IntSize& size, SurfaceFormat format)
> +{
> +  VolatileBufferPtr<unsigned char> vbufptr(vbuf);
> +  if (vbufptr.WasBufferPurged()) {
> +    NS_WARNING("VolatileDataSourceSurface buffer was purged");

I know this came from the old code, but there's actually no such thing as VolatileDataSourceSurface right now. Change this to just read "VolatileBuffer was purged".

Additionally, return a boolean from this method, because if you hit this failure you need to set |mAborted = true| in InitForDecoder() and return something other than NS_OK.

@@ +90,5 @@
> +    NS_WARNING("VolatileDataSourceSurface buffer was purged");
> +    return;
> +  }
> +
> +  int32_t stride = VolatileSurfaceStride(size, format);

Nit: |const int32_t|

@@ +92,5 @@
> +  }
> +
> +  int32_t stride = VolatileSurfaceStride(size, format);
> +  if ((format == SurfaceFormat::B8G8R8X8) &&
> +      (gfxPlatform::GetPlatform()->GetDefaultContentBackend() == BackendType::SKIA)) {

This code runs off-main-thread and may run after shutdown has occurred. You are really running into problems here because ShutdownTracker is not thread-safe right now so you can't even use it to check if shutdown has started. (And you need to check because gfxPlatform is not available after shutdown has started.)

It's going to be a kinda a pain if you want to do the check for Skia. You'll need to add a static variable to cache the default content backend in, and you'll need to add a static method to get the value, and you'll need to call it from mozilla::image::EnsureModuleInitialized(), and you may need to update some GTest files. It's probably OK to always do the memset() if the format is BGRX; if we discover it's a performance problem, we can optimize it later in a separate patch.

@@ +240,5 @@
>        mAborted = true;
>        return NS_ERROR_OUT_OF_MEMORY;
>      }
> +
> +    InitVolatileBuffer(mVBuf, mFrameRect.Size(), mFormat);

See above; you need to do something like |if (!ClearSurface(...)) { mAborted = true; return NS_ERROR_OUT_OF_MEMORY; }|

@@ +293,5 @@
> +
> +    mImageSurface = CreateLockedSurface(mVBuf, mFrameRect.Size(), mFormat);
> +
> +    if (!mImageSurface) {
> +      NS_WARNING("Failed to create VolatileDataSourceSurface");

Again, let's get rid of "VolatileDataSourceSurface" here, a thing that does not actually exist.

@@ +300,3 @@
>      }
> +
> +    InitVolatileBuffer(mVBuf, mFrameRect.Size(), mFormat);

See above.
Attachment #8766593 - Flags: review?(seth)
Attached patch Memset RGBX surfaces to white (obsolete) — Splinter Review
For the log, this is the hardest memset ever.
Attachment #8766593 - Attachment is obsolete: true
Attachment #8766605 - Flags: review?(seth)
Comment on attachment 8766605 [details] [diff] [review]
Memset RGBX surfaces to white

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

OK, this looks ready to land. Thanks for sticking with it. Hardest memset() I've ever seen.
Attachment #8766605 - Flags: review?(seth) → review+
Keywords: checkin-needed
Carrying r+, updated to include bug number in header.
Attachment #8766605 - Attachment is obsolete: true
Attachment #8766861 - Flags: review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13cd8e7c973a
memset unoptimized images with RGBX surface format to 0xFF. r=seth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/13cd8e7c973a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.