539771 - Add support for context attribs to canvas

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Attachment: add support for getContext attribs

WebGL wants an optional arg to getContext(), which is a generic object property bag with some options for the context.  This implements that in the most generic way possible; we can probably replace moz-opaque and friends with this as well.

This patch implements support for WebGL context attribs on windows; other platforms to follow.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 421681 [details] [diff] [review]
add support for getContext attribs

You could implement operator!= using operator== to avoid having to keep the two conditions in sync. But it probably doesn't matter that much.

What does the premultiplied alpha option do?

Comment 2

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

premultiplied is supposed to inform the browser whether the GL color buffer is already premultiplied or not... that is, if premultipliedAlpha is false, then the browser must do the premultiplication step before compositing.  It's not implemented yet for us anywhere.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Does opengl support non-premultiplied alpha color buffers?

Comment 4

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

OpenGL has no concept of premultiplied alpha -- it just provides RGBA channels that can be written to.  With the right combination of blending modes/fragment shaders you can get either premultiplied or non-premultiplied data in the color buffer.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Perhaps I'm misremembering, but I didn't think there was a blend mode that let you get non-premultiplied results...

Comment 8

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

Attachment: one more time, for real

One more time.  Largely the same patch as before, just rearranged for new context config stuff.  Also fixed missing stencil param on WGL backend.

Comment 9

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

(Needed for WebGL)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

My question about a non-premultiplied color buffer still stands. I don't think these are possible. I'd be happy to see evidence to the contrary.

Comment 11

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

Same answer as I gave above, but it's basically not relevant -- the premultiplied alpha flag specifies how the contents of the color buffer should be interpreted.  How the bits get there isn't really relevant, no?  That's up to the WebGL app, doing whatever it wants.  If it's not possible to do non-premultiplied blending, that's fine, but that doesn't mean that you can't have a color buffer that has non-premultiplied alpha values in it.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #11)
> Same answer as I gave above, but it's basically not relevant -- the
> premultiplied alpha flag specifies how the contents of the color buffer should
> be interpreted.  How the bits get there isn't really relevant, no?  That's up
> to the WebGL app, doing whatever it wants.  If it's not possible to do
> non-premultiplied blending, that's fine, but that doesn't mean that you can't
> have a color buffer that has non-premultiplied alpha values in it.

True, but what's the use case for interpreting the color buffer as non-premultiplied alpha then? If no one is going to use the non-premultiplied interpretation why bother supporting it?

Comment 13

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

The use case is "the spec requires it".  (This stuff isn't some random mozilla extension, it's all in the WebGL spec.)

But, it's here because, as I said, you can write whatever you want to the r g b a channels.  It's not possible to enforce "you must only write premultiplied alpha."  So we either had to say that the color buffer is composited as if it always had premultiplied alpha, or to provide a way for the author to tell us otherwise.  We went with the latter route, because there was no reason not to.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #13)
> The use case is "the spec requires it".  (This stuff isn't some random mozilla
> extension, it's all in the WebGL spec.)

I figured that was the case.

> So we either had to say that the color buffer is composited as if it
> always had premultiplied alpha,

I would've chosen that.

> or to provide a way for the author to tell us
> otherwise.  We went with the latter route, because there was no reason not to.

Simplicity is the only reason I have. But if it's too late or not worth it to change the spec, then I have no objections.

Comment 15

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

Yeah, it's pretty much too late.  I don't think it hurts anything, really.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #15)
> Yeah, it's pretty much too late.  I don't think it hurts anything, really.

It does mean we can't assume premultiplied input, like our other sources, in our shaders so we'll need to have multiple versions of them or have branching.

Comment 19

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

Attachment: for realsies

Ok, for real this time, without test failures and with additional fixes.  Three parts that got mostly merged into the same patch due to dependencies.

1 - Jeff, can you review the general canvas context API parts?  It's largely identical to what it was before, though now it uses jsval instead of a property bag from xpcom

2 - Waldo, can you review the js API usage in nsHTMLCanvasElement::GetContext?

3 - Benoit, can you review the changes in the WebGL code itself?  Specifically, there's fixup here for ReadPixels to always read 0xff as the alpha value if the source doesn't have an alpha channel, as per the spec -- various drivers don't actually follow this (e.g. nvidia returns whatever was written there, it just ignores it when doing compositing or reading from it in a texture sampler).

Comment 20

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

Oh, and ignore the js/src/xpconnect/src/xpcconvert.cpp change -- that's for bug 504198/604196 to fix.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 484216 [details] [diff] [review]
for realsies

Looks fine, although I'd think NULL would be more idiomatic than 0 for pointer argument values, and at least in JS-land we wouldn't bother with the ok temporary (just one massive if-condition).

Also, I noticed the misspelling of "mipmap" as "mipamp" in a new comment while skimming other bits of the patch, for what it's worth.  :-)

Comment 22

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

Comment on attachment 484216 [details] [diff] [review]
for realsies

Yeah, I can change those -- not sure why I even wrote the ok temporary, it was leftover from some previous code style.  A big if sounds much better (and easier to read).

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 484216 [details] [diff] [review]
for realsies

r=me assuming you ran the webgl test suite and there's no regression;

just one comment: in this inner loop:

+    for (GLint i = 0; i < width; ++i) {
+        *rowp = 0xff;
+        rowp += 4;
+    }

at every iteration you're addressing and incrementing two variables, rowp and i, so this would be more efficient:

     PRUint8* endrowp = rowp + 4 * width;
     while (rowp != endrowp) {
         *rowp = 0xff;
         rowp += 4;
     }

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Why do you use a jsval for getter and a property bag for the setter? Can't you use a jsval for both?

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 484216 [details] [diff] [review]
for realsies

Vlad convinced me this was fine.

Comment 26

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

Finally, finally banged this in: http://hg.mozilla.org/mozilla-central/rev/941c3d14e14d