implement webgl color conversion/premultiplication semantics

RESOLVED FIXED

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker])

Attachments

(1 attachment, 4 obsolete attachments)

Created attachment 500405 [details] [diff] [review]
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.
Attachment #500405 - Flags: review?(joe)
Created attachment 501789 [details] [diff] [review]
updated patch

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]
Created attachment 502634 [details] [diff] [review]
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.
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
Created attachment 502839 [details] [diff] [review]
v3 with --unified 8
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-
Created attachment 502970 [details] [diff] [review]
v4, updated

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+
http://hg.mozilla.org/mozilla-central/rev/98749e68d9d3

Yay!
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 696569
You need to log in before you can comment on or make changes to this bug.