Treat RGBA images as premultiplied alpha

[jamienicol]

Fixes #889.

Use blend function for premultiplied alpha. Make wrench unpremultiply when saving and repremultiply when loading. Saving images with the json writer seems to be broken, but I added an unpremultiply call anyway.

---

This change is 

[glennw]

@jamienicol Are you able to run the WPT/CSS tests in Servo with this patch, just to make sure we catch all the cases in Servo (I think we'll need to handle the basic image case that @jrmuizel mentioned, and also the canvas readback path). If you're unable to run the Servo tests, I can test this locally next week.

[bors-servo]

☔ The latest upstream changes (presumably #912) made this pull request unmergeable. Please resolve the merge conflicts.

[bors-servo]

☔ The latest upstream changes (presumably #897) made this pull request unmergeable. Please resolve the merge conflicts.

[bors-servo]

☔ The latest upstream changes (presumably #942) made this pull request unmergeable. Please resolve the merge conflicts.

[jamienicol]

Finally have this passing servo tests. (Servo change is here. The one that took me so long was the mozilla webgl tests - I eventually reached to the conclusion that it is reasonable for webrender to expect premultiplied alpha when compositing a webgl canvas, and therefore it is the test's responsibility to ensure that is the case.)

@glennw I think this is ready for review now. Should I create a servo pull request too?

[glennw]

No need to worry about the Servo PR - I'll cherry pick that commit into the WR update once this is landed. Just going to get @emilio to check that the webgl test changes above seem reasonable, and then we can proceed with this. Thanks for following it up!

[emilio]

The WebGL test changes look reasonable to me :)

[emilio]

(Let's wait for me to check if the reftest as written before the change also failed in other browsers though)

[emilio]

Hmm, upon reflection I don't think those test changes should be needed at all? What does the pixel difference with your test look like? (you can check it out with http://hoppipolla.co.uk/410/reftest-analyser-structured.xhtml, uploading the log from the reftest output)

I guess it'd be ok to land those changes (since those tests aren't really testing premultiplication, and it should be covered by the WebGL conformance test suite), but at least opening an issue mentioning this and showing the different rendering should be appropriate.

[emilio]

Oh, I guess you might be running the test suite on mac maybe? We readback there, and what should be done instead of changing the tests is premultiplying here: https://github.com/servo/servo/blob/master/components/canvas/webgl_paint_thread.rs#L224

[jamienicol]

The pixels around the edges of the rust logo (where the source image has transparency) are a different color than the reference output. This is because the test renders the unpremultiplied pixel data to the canvas with webgl, then webrender composites the canvas as if the pixel data is premultiplied. Rather than premultiplying when uploading, the test could instead enable blending for the draw call which would have the same effect.

As I see it, webrender can either expect webgl canvas data to be premultiplied or unpremultiplied, then the test / random website's code is responsible for sticking to that. I couldn't see any specifications, so I am interested in what other browsers do.

[jamienicol]

I get the same results on Ubuntu and Mac.

[emilio]

As I see it, webrender can either expect webgl canvas data to be premultiplied or unpremultiplied, then the test / random website's code is responsible for sticking to that.

How could the website be responsible of how the renderer interprets the WebGL canvas? That doesn't seem right to me.

I couldn't see any specifications, so I am interested in what other browsers do.

Reading https://www.khronos.org/registry/webgl/specs/latest/1.0/, under texImage2D:

If the original HTMLImageElement contains an alpha channel and the UNPACK_PREMULTIPLY_ALPHA_WEBGL pixel storage parameter is false, then the RGB values are guaranteed to never have been premultiplied by the alpha channel, whether those values are derived directly from the original file format or converted from some other color format.

That indeed doesn't specify how the renderer should render it, but it seems the intention is to render as-if it wasn't premultiplied.

I get the same results on Ubuntu and Mac.

Note that we can also fallback to readback on Ubuntu. Even without that, I guess we're still sending the texture unpremultiplied, and WR with these changes is treating it as premultiplied, so the result may be the same anyway... Is it possible to allow both behaviors via the API?

If it's too inconvenient, it may be ok to file an issue and land that with those changes I guess, though it's not ideal.

Also, let's make sure that the test case at servo/servo#15578 works with servo, and land it with your changes.

[jamienicol]

How could the website be responsible of how the renderer interprets the WebGL canvas? That doesn't seem right to me.

What I mean is that the website has full control over the alpha values in the canvas. Therefore there must be some agreement between it and the browser over how those values are interpreted.

I'm not sure I read that intention from that quote. It's about a texture's data, not the canvas itself. I think our answer is in section 2.4 however:

The OpenGL API allows the application to modify the blending modes used during rendering, and for this reason allows control over how alpha values in the drawing buffer are interpreted; see the premultipliedAlpha parameter in the WebGLContextAttributes section.

It seems there is a context creation parameter "premultipliedAlpha", which defaults to true. Our current implementation is correct only if this parameter is false, and this change makes it work only if this parameter is true.

I think this change is still a good one, since it makes the default case correct. But it seems that webrender may indeed need an api to specify whether an image is premultiplied or unpremultiplied.

[emilio]

I think this change is still a good one, since it makes the default case correct. But it seems that webrender may indeed need an api to specify whether an image is premultiplied or unpremultiplied.

Makes sense. If this can't be done as part of this PR, could an issue be opened in servo/servo about this?

[jamienicol]

Okay, I'll open a servo issue. I'd prefer to get this merged as is since it fixes gecko.

Thanks for looking at this @emilio. Did you check whether the webgl tests work in different browsers?

[glennw]

@bors-servo r=emilio,glennw

@jamienicol or @emilio Would you mind opening a Servo issue with the relevant details for follow up work?

[bors-servo]

📌 Commit 821b789 has been approved by emilio,glennw

[bors-servo]

⌛ Testing commit 821b789 with merge 94f255d...

[bors-servo]

☀️ Test successful - status-travis
Approved by: emilio,glennw
Pushing 94f255d to master...