622184 - implement webgl color conversion/premultiplication semantics

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add support for unpremultiplied/non-colorspace-converted image data

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.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

I try, unsuccessfully, to avoid hitting the

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

assertion in InitDecoder.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated v3

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.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 4

[Joe Drew (not getting mail)]

(In reply to comment #3)
> regular web browsing should not be
> impacted in any way.

famous last words

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v3 with --unified 8

Comment 6

[Joe Drew (not getting mail)]

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);

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v4, updated

updated with review comments.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 502970 [details] [diff] [review]
v4, updated

this had better not come back to bite me, vukicevic

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/98749e68d9d3

Yay!