Closed Bug 622184 Opened 9 years ago Closed 9 years ago

implement webgl color conversion/premultiplication semantics


(Core :: Canvas: WebGL, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: vlad, Assigned: vlad)



(Whiteboard: [hardblocker])


(1 file, 4 obsolete files)

WebGL requires us to make available the original contents of an image, including without alpha premultiplication and colorspace conversion.

The attached patch does this.  There are two parts:

- Implementing these options as flags to GetFrame in imglib
- Passing the right flags down through WebGL and nsLayoutUtils::SurfaceFromElement

Both are pretty straightforward, but I need some feedback on the first part especially.  Implementing this in imglib is done via discarding; each imgContainer now gets a frameflags parameter that contains these flags.  If GetFrame is ever called with flags that don't match the currently decoded bits, we forcibly discard the decoded data and redecode.  This can lead to some thrashing, but I think it'll be extremely rare, and will fall under the category of "don't do that" -- basically frequently calling teximage2d with an image that's also used for display or something, or calling teximage2d with different flags frequently.
Attachment #500405 - Flags: review?(joe)
Attached patch updated patch (obsolete) — Splinter Review
I try, unsuccessfully, to avoid hitting the

  NS_ABORT_IF_FALSE(!DiscardingActive(), "Discard Timer active in InitDecoder()!");

assertion in InitDecoder.
Attachment #500405 - Attachment is obsolete: true
Attachment #501789 - Flags: review?(joe)
Attachment #500405 - Flags: review?(joe)
blocking2.0: --- → betaN+
Whiteboard: [hardblocker]
Attached patch updated v3 (obsolete) — Splinter Review
Updated again!  I was removing the discard timer only if a decoder existed, which of course it not always the case.  I can't trigger the assertion any more.

However, as I said on irc, I'm not sure if I caught all the entry points into RasterImage that need to ensure that the decode flags are the default ones.
Attachment #501789 - Attachment is obsolete: true
Attachment #502634 - Flags: review?(joe)
Attachment #501789 - Flags: review?(joe)
oh, I should say: the new code here will only trigger if being called from webgl.  So any subtle bugs that might be left will have to be triggered through webgl (or as a result of webgl usage); regular web browsing should not be impacted in any way.
(In reply to comment #3)
> regular web browsing should not be
> impacted in any way.

famous last words
Attached patch v3 with --unified 8 (obsolete) — Splinter Review
Attachment #502634 - Attachment is obsolete: true
Attachment #502839 - Flags: review?(joe)
Attachment #502634 - Flags: review?(joe)
Comment on attachment 502839 [details] [diff] [review]
v3 with --unified 8

>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp

>@@ -3601,6 +3606,10 @@ nsLayoutUtils::SurfaceFromElement(nsIDOM
>         return result;
>     }
>+    if (aSurfaceFlags & SFE_NO_PREMULTIPLY_ALPHA) {
>+      gfxUtils::UnpremultiplyImageSurface(static_cast<gfxImageSurface*>(surf.get()));
>+    }

Put a comment above this saying that it's okay to modify the surface because we forced a copy into an image surface above.

>diff --git a/modules/libpr0n/decoders/nsPNGDecoder.cpp b/modules/libpr0n/decoders/nsPNGDecoder.cpp

>@@ -216,6 +217,8 @@ void nsPNGDecoder::EndImageFrame()
> void
> nsPNGDecoder::InitInternal()
> {
>+  mDisableCMS = (mDecodeFlags & DECODER_NO_COLORSPACE_CONVERSION) != 0;

Can you change this to be an mCMSMode like the JPEG decoder? It would change our behaviour for gamma correction, but we should already be turning off gamma correction when colour correction is globally off, I think.

>diff --git a/modules/libpr0n/src/RasterImage.cpp b/modules/libpr0n/src/RasterImage.cpp

I believe you got all the entry points where we might need to discard and redecode. I'm not in love with the special case for resetting the discard tracker in Discard(), but I don't know if there's a better way.

>diff --git a/modules/libpr0n/src/RasterImage.h b/modules/libpr0n/src/RasterImage.h

>@@ -208,17 +208,17 @@ public:

>   /* Triggers discarding. */
>-  void Discard();
>+  void Discard(bool force = false);

I'd much prefer a self-documenting enumerated type as an argument here, but I would accept all calls being changed to the form
  Discard(/* force = */ true);
Attachment #502839 - Flags: review?(joe) → review-
Attached patch v4, updatedSplinter Review
updated with review comments.
Attachment #502839 - Attachment is obsolete: true
Attachment #502970 - Flags: review?(joe)
Comment on attachment 502970 [details] [diff] [review]
v4, updated

this had better not come back to bite me, vukicevic
Attachment #502970 - Flags: review?(joe) → review+

Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 696569
You need to log in before you can comment on or make changes to this bug.