Last Comment Bug 698169 - [WebGL] texSubImage2D with an Image seems to copy premultiplied alpha values
: [WebGL] texSubImage2D with an Image seems to copy premultiplied alpha values
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86_64 Linux
: -- normal with 2 votes (vote)
: Firefox 11
Assigned To: Jeff Gilbert [:jgilbert]
Depends on:
Blocks: 738757 webgl-conformance 696569
  Show dependency treegraph
Reported: 2011-10-28 19:19 PDT by Bill Baxter
Modified: 2012-03-23 12:39 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Demonstrate the problem with texSubImage2D on Firefox Nightly (6.57 KB, application/octet-stream)
2011-10-28 19:19 PDT, Bill Baxter
no flags Details
Make WebGL texSubImage2D respect alpha-premultiplied DOM sources (1.01 KB, patch)
2011-12-15 05:46 PST, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description Bill Baxter 2011-10-28 19:19:32 PDT
Created attachment 570447 [details]
Demonstrate the problem with texSubImage2D on Firefox Nightly

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.102 Safari/535.2

Steps to reproduce:

I used texSubImage2D to upload image data from an Image object that has transparency.  This is with Nightly running on Linux.

Actual results:

When using texImage2D everything works as expected, but when using texSubImage2D some of the low-alpha pixels turn dark.

Expected results:

The texImage2D and texSubImage2D cases should both look the same.

I wrote a test program to reproduce the problem.  To see the issue, unzip the files into a directory and try opening the "iconAlpha.html" in the browser.   Should look fine, since this version uses texImage2D.  Now open "iconAlpha.html?sub".  The fringe pixels will get dark because this version uses texSubImage2D.  This behavior is not seen in WebKit based browsers.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2011-11-30 22:32:46 PST
Jeff, is this the same as Bug 696569 ?
Comment 2 Jeff Gilbert [:jgilbert] 2011-12-01 03:51:46 PST
It can't be the same, since it's different for texImage* and texSubImage*. However, this should be a prerequisite for the final patch for that bug to pass.
Comment 3 Jeff Gilbert [:jgilbert] 2011-12-15 05:40:58 PST
Ah, here's this bug. Confirmed, also should be easy.
Comment 4 Jeff Gilbert [:jgilbert] 2011-12-15 05:46:08 PST
Created attachment 581936 [details] [diff] [review]
Make WebGL texSubImage2D respect alpha-premultiplied DOM sources
Comment 6 Matt Brubeck (:mbrubeck) 2011-12-17 09:28:15 PST
Comment 7 Bill Baxter 2012-02-27 21:36:26 PST
Any chance this will get backported to FF10?  It makes all the MapsGL icons look bad.
Comment 8 Jeff Gilbert [:jgilbert] 2012-02-28 02:12:04 PST
(In reply to Bill Baxter from comment #7)
> Any chance this will get backported to FF10?  It makes all the MapsGL icons
> look bad.

Extremely unlikely, in my understanding. Basically 'Release' only gets chem-spill fixes. I believe we're roughly three weeks away from 11 hitting Release, though.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-03-23 08:46:53 PDT
I could be missing something, but there is something wrong with the code as it currently stands with this patch.

This patch makes TexImage2D_dom call TexImage2D_base with srcPremultiplied = mPixelStorePremultiplyAlpha.

I don't understand: mPixelStorePremultiplyAlpha, in my understanding, means "whether we should pass premultiplied values to glTexImage2D", not "whether image data passed to webgl.texImage2D is assumed to be premultiplied". Indeed, WebGL spec:

> If set, then during any subsequent calls to texImage2D or texSubImage2D, the alpha channel of the source data, if present, is multiplied into the color channels during the data transfer.

Notice that TexImage2D_base then calls ConvertImage with dstPremultiplied=mPixelStorePremultiplyAlpha. So the current code NEVER premultiplies or unpremultiplies DOM elements, because it always calls ConvertImage with dstPremultiplied==srcPremultiplied==mPixelStorePremultiplyAlpha.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2012-03-23 08:48:50 PDT
Though, the fact that only texSubImage2D changed here and texImage2D was already like that, suggests that it's probably me not understanding this stuff correctly...
Comment 11 Jeff Gilbert [:jgilbert] 2012-03-23 12:36:56 PDT
In the _dom calls, we fetch the data in the correct (un)premultiplied state. By passing in mPixelStorePremultiplyAlpha, are really telling the function 'don't mess with this, it's already what we want'.

I'll file a bug to make this behavior more obvious, probably by instead of having a bool param for 'is this premultiplied', have a bool param for 'does this maybe need premuliplication'. For _dom calls, this would be false, else, true.

Note You need to log in before you can comment on or make changes to this bug.