1129332 - WebGL 2 extension EXT_color_buffer_float may be implemented

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Florian Bösch]

The WebGL extension EXT_color_buffer_float moved to draft status, it may now be implemented.

Comment 1

[Florian Bösch]

Chrome: https://code.google.com/p/chromium/issues/detail?id=455130
Webkit: https://bugs.webkit.org/show_bug.cgi?id=141240
IE: https://connect.microsoft.com/IE/feedback/details/1114829
ANGLE: https://code.google.com/p/angleproject/issues/detail?id=907

Comment 2

[Florian Bösch]

Implementation is already complete on ANGLE's side.

Comment 3

[Benoit Girard (:BenWa)]

Jeff can you triage this bug?

Comment 4

[Kelsey Gilbert [:jgilbert]]

This has been superseded by WEBGL_color_buffer_float, which we already support.

Comment 5

[Florian Bösch]

WEBGL_color_buffer_float is written against the WebGL 1.0 specification, it supports the formats RGBA32F only and . EXT_color_buffer_float is written against the WebGL 2.0 specification and covers a guarantee that R16F, RG16F, RGBA16F, R32F, RG32F, RGBA32F and R11F_G11F_B10F.

WEBGL_color_buffer_float and EXT_color_buffer_float stand in no direct set/subset/suerpceeding relationship. Reopened.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Florian Bösch from comment #5)
> WEBGL_color_buffer_float is written against the WebGL 1.0 specification, it
> supports the formats RGBA32F only and . EXT_color_buffer_float is written
> against the WebGL 2.0 specification and covers a guarantee that R16F, RG16F,
> RGBA16F, R32F, RG32F, RGBA32F and R11F_G11F_B10F.
> 
> WEBGL_color_buffer_float and EXT_color_buffer_float stand in no direct
> set/subset/suerpceeding relationship. Reopened.

Right, they are not named well.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: Bug 1129332 - r?bz - Add webidl for EXT_color_buffer_float.

From b2a3d26d7878058bf0b696974be78bac110324a8 Mon Sep 17 00:00:00 2001
---
 dom/bindings/Bindings.conf               | 5 +++++
 dom/webidl/WebGL2RenderingContext.webidl | 4 ++++
 2 files changed, 9 insertions(+)

Review commit: https://reviewboard.mozilla.org/r/50653/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50653/

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: MozReview Request: r?jrmuizel - Implement EXT_color_buffer_float.

From a08577ac23b09bda05f8f1da2a9f31c6896d66fa Mon Sep 17 00:00:00 2001
---
 dom/canvas/WebGLContextExtensions.cpp            | 17 +++++++-
 dom/canvas/WebGLExtensionEXTColorBufferFloat.cpp | 51 ++++++++++++++++++++++++
 dom/canvas/WebGLExtensions.h                     | 12 ++++++
 dom/canvas/WebGLProgram.cpp                      |  1 +
 dom/canvas/WebGLTypes.h                          |  1 +
 dom/canvas/moz.build                             |  1 +
 gfx/gl/GLContext.h                               |  1 +
 gfx/gl/GLContextFeatures.cpp                     | 10 +++++
 8 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 dom/canvas/WebGLExtensionEXTColorBufferFloat.cpp

Review commit: https://reviewboard.mozilla.org/r/50655/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50655/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8748959 [details]
MozReview Request: Bug 1129332 - r?bz - Add webidl for EXT_color_buffer_float.

https://reviewboard.mozilla.org/r/50653/#review47401

r=me

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8748960 [details]
MozReview Request: r?jrmuizel - Implement EXT_color_buffer_float.

https://reviewboard.mozilla.org/r/50655/#review47891

::: dom/canvas/WebGLExtensionEXTColorBufferFloat.cpp:19
(Diff revision 1)
> +WebGLExtensionEXTColorBufferFloat::WebGLExtensionEXTColorBufferFloat(WebGLContext* webgl)
> +    : WebGLExtensionBase(webgl)
> +{
> +    MOZ_ASSERT(IsSupported(webgl), "Don't construct extension if unsupported.");
> +
> +    auto& fua = webgl->mFormatUsage;

fua is not a great variable name, please choose something better.

::: dom/canvas/WebGLProgram.cpp:13
(Diff revision 1)
>  #include "GLContext.h"
>  #include "mozilla/CheckedInt.h"
>  #include "mozilla/dom/WebGL2RenderingContextBinding.h"
>  #include "mozilla/dom/WebGLRenderingContextBinding.h"
>  #include "mozilla/RefPtr.h"
> +#include "nsPrintfCString.h"

Is this needed?

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8748960 [details]
MozReview Request: r?jrmuizel - Implement EXT_color_buffer_float.

https://reviewboard.mozilla.org/r/50655/#review47967

Comment 12

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> Comment on attachment 8748960 [details]
> MozReview Request: r?jrmuizel - Implement EXT_color_buffer_float.
> 
> https://reviewboard.mozilla.org/r/50655/#review47891
> 
> ::: dom/canvas/WebGLExtensionEXTColorBufferFloat.cpp:19
> (Diff revision 1)
> > +WebGLExtensionEXTColorBufferFloat::WebGLExtensionEXTColorBufferFloat(WebGLContext* webgl)
> > +    : WebGLExtensionBase(webgl)
> > +{
> > +    MOZ_ASSERT(IsSupported(webgl), "Don't construct extension if unsupported.");
> > +
> > +    auto& fua = webgl->mFormatUsage;
> 
> fua is not a great variable name, please choose something better.
Ok, but I want to update all the duplicates of this issue at once.

> 
> ::: dom/canvas/WebGLProgram.cpp:13
> (Diff revision 1)
> >  #include "GLContext.h"
> >  #include "mozilla/CheckedInt.h"
> >  #include "mozilla/dom/WebGL2RenderingContextBinding.h"
> >  #include "mozilla/dom/WebGLRenderingContextBinding.h"
> >  #include "mozilla/RefPtr.h"
> > +#include "nsPrintfCString.h"
> 
> Is this needed?
Yes. This was needed before, but unified builds was causing it to be included previously.

Comment 13

[Kelsey Gilbert [:jgilbert]]

leave-open because we need to add webgl2 ensure-exts tests.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Got the bug number in the commit wrong:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed2bae2ed64
https://hg.mozilla.org/integration/mozilla-inbound/rev/5268ba1f8114

https://hg.mozilla.org/mozilla-central/rev/5ed2bae2ed64
https://hg.mozilla.org/mozilla-central/rev/5268ba1f8114

Comment 15

[Florian Scholz (Open Web Docs)]

Mentioned in developer release notes for Firefox 49:
https://developer.mozilla.org/en-US/Firefox/Releases/49#WebGL

New reference page:
https://developer.mozilla.org/en-US/docs/Web/API/EXT_color_buffer_float

Updated to mention the new formats:
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/renderbufferStorage

Linked from:
https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API#Extensions
https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/Using_Extensions
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getSupportedExtensions
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getExtension

Changed the extension availability info for these two extensions to WebGL 1 only:
https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_color_buffer_float
https://developer.mozilla.org/en-US/docs/Web/API/EXT_color_buffer_half_float

See also https://github.com/KhronosGroup/WebGL/pull/1647. I think the extension availability change in Gecko happens in bug 1271830 actually, but I assume it will land in 49 as well. I think it is quite complex to grasp when which extension is available to which WebGL version, so whenever we change this, we should update the docs (please mark bugs as dev-doc-needed)! :-)

Review appreciated.

Comment 16

[Kelsey Gilbert [:jgilbert]]

I clarified the docs. (or tried to!)

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: The EXT_color_buffer_float WebGL 2 extension has been implemented.
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/Firefox/Releases/49#WebGL

Comment 18

[Marcia Knous [:marcia]]

Adding 49+ for the release notes - the release notes now point to the section on MDN where this implementation has been noted.