973815 - WebGL EXT_blend_minmax may now be implemented

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Florian Bösch]

The WebGL extension EXT_blend_minmax is now in draft and may be implemented: http://www.khronos.org/registry/webgl/extensions/EXT_blend_minmax/

Comment 1

[Florian Bösch]

Chrome ticket: https://code.google.com/p/chromium/issues/detail?id=344451

Comment 2

[Florian Bösch]

Webkit ticket: https://bugs.webkit.org/show_bug.cgi?id=128974

Comment 3

[Guillaume Abadie]

Since I've implemented it in WebGL 2, taking it!

Comment 4

[Guillaume Abadie]

Attachment: ext_blend_minmax.patch

This patch implements the WebGL extension EXT_blend_minmax passing WebGL conformance 1.0.3 tests.

Push to try:
https://tbpl.mozilla.org/?tree=Try&rev=2beffa2cceb0

Comment 5

[Guillaume Abadie]

Attachment: ext_blend_minmax.patch - revision 2

Forgotten to add content/canvas/src/WebGLExtensionBlendMinMax.cpp... Might be useful in the patch! ^^

Push to try:
https://tbpl.mozilla.org/?tree=Try&rev=c7b755d5e201

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8436451 [details] [diff] [review]
ext_blend_minmax.patch - revision 2

Review of attachment 8436451 [details] [diff] [review]:
-----------------------------------------------------------------

BZ: We're adding more WG-approved IDL.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8436451 [details] [diff] [review]
ext_blend_minmax.patch - revision 2

r=me

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8436451 [details] [diff] [review]
ext_blend_minmax.patch - revision 2

Review of attachment 8436451 [details] [diff] [review]:
-----------------------------------------------------------------

BZ: We're adding more WG-approved IDL.

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +159,5 @@
>              return WebGLExtensionDrawBuffers::IsSupported(this);
>          case WebGLExtensionID::EXT_frag_depth:
>              return WebGLExtensionFragDepth::IsSupported(this);
> +        case WebGLExtensionID::EXT_blend_minmax:
> +            return WebGLExtensionBlendMinMax::IsSupported(this);

Actually, I need to you put this in the Draft Extension list.

Comment 9

[Guillaume Abadie]

Attachment: ext_blend_minmax.patch - revision 3

Your are very lucky that inbound was closed last night when I tried to push! =)

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8438061 [details] [diff] [review]
ext_blend_minmax.patch - revision 3

Review of attachment 8438061 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 11

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Comment on attachment 8438061 [details] [diff] [review]
> ext_blend_minmax.patch - revision 3
> 
> Review of attachment 8438061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!

Pleasure is all mine. But I couldn't land it... I had this message:

remote: ************************** ERROR ****************************
remote: 
remote: WebIDL file dom/webidl/WebGLRenderingContext.webidl altered in changeset 67d859377462 without DOM peer review
remote: 
remote: 
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: 
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************

But my commit message is:
Bug 973815 - Implements WebGL extension EXT_blend_minmax - r=jgilbert, bzbarsky 

What did I do wrong?

Comment 12

[Boris Zbarsky [:bzbarsky]]

The space after the ','?  ehsan, what does the hook actually match?

Comment 13

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #12)
> The space after the ','?  ehsan, what does the hook actually match?

It looks for all r=... tokens separated by whitespace and then gets the part after the equal sign and breaks it on the comma character and looks at each token.  For example here the hook sees "r=jgilbert," -> "jgilbert," -> ["jgilbert"], and the "bzbarsky" part is completely invisible to it.

Can you please fix your commit message to drop that whitespace?  I see no sane way of making the hook accept this commit message without adding footguns to it.  Sorry!

Comment 14

[Guillaume Abadie]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #13)
> (In reply to Boris Zbarsky [:bz] from comment #12)
> > The space after the ','?  ehsan, what does the hook actually match?
> 
> It looks for all r=... tokens separated by whitespace and then gets the part
> after the equal sign and breaks it on the comma character and looks at each
> token.  For example here the hook sees "r=jgilbert," -> "jgilbert," ->
> ["jgilbert"], and the "bzbarsky" part is completely invisible to it.
> 
> Can you please fix your commit message to drop that whitespace?  I see no
> sane way of making the hook accept this commit message without adding
> footguns to it.  Sorry!

Thanks Ehsan! =)

Comment 15

[Guillaume Abadie]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5b7ed0dd5a

Comment 16

[(no longer active)]

Sorry, backed this out because of build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f903c0f48969
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ca5b7ed0dd5a

Comment 17

[Guillaume Abadie]

Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a6e235e674

I suspect we must have a trailing "using namespace mozilla:gl;" somewhere in a header file when not building to B2G, but I couldn't find it...

Comment 18

[Boris Zbarsky [:bzbarsky]]

It's probably in a .cpp file and propagating across to other files due to unified builds.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d5a6e235e674

Comment 20

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/33#WebGL

https://developer.mozilla.org/en-US/docs/Web/API/EXT_blend_minmax
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/blendEquation
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/blendEquationSeparate