Closed Bug 1102667 Opened 5 years ago Closed 5 years ago

texture_float extensions should imply either full or no color_buffer_float activation

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(4 files, 1 obsolete file)

Attached file testcase
Right now, everyone seems to allow RTT with float textures when the relevant extension is active. The extensions say that they should implicitly activate their respective color_buffer_float extensions, though.

That includes support for ReadPixels and renderbufferStorage.
Attachment #8527083 - Flags: review?(dglastonbury)
I forgot to add the mochitest to the manifest.
Attachment #8527083 - Attachment is obsolete: true
Attachment #8527083 - Flags: review?(dglastonbury)
Attachment #8527113 - Flags: review?(dglastonbury)
Comment on attachment 8527113 [details] [diff] [review]
0001-Fix-float-texture-rb-fb-support.patch

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

It wasn't clear from the bug comment that the intention is to keep implicitly enabling float RTT.

::: dom/canvas/WebGLContextExtensions.cpp
@@ +190,5 @@
> +WebGLContext::EnableSupportedExtension(JSContext* js, WebGLExtensionID ext)
> +{
> +    if (!IsExtensionEnabled(ext)) {
> +        if (!IsExtensionSupported(js, ext))
> +            return nullptr;

if (!IsExtensionSupported(js, ext))
    return nullptr;

if (!IsExtensionEnabled(ext))
    EnableExtension(ext);

return mExtensions[ext];

(I hate nesting)
Attachment #8527113 - Flags: review?(dglastonbury) → review+
Attachment #8527128 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #4)
> Comment on attachment 8527113 [details] [diff] [review]
> 0001-Fix-float-texture-rb-fb-support.patch
> 
> Review of attachment 8527113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It wasn't clear from the bug comment that the intention is to keep
> implicitly enabling float RTT.
As we sit currently, this disables float RTT. We should be getting the extension we need promoted to Community Approved soon. When this happens, color_buffer_float comes out from behind the draft flag, and will be picked up here.
> 
> ::: dom/canvas/WebGLContextExtensions.cpp
> @@ +190,5 @@
> > +WebGLContext::EnableSupportedExtension(JSContext* js, WebGLExtensionID ext)
> > +{
> > +    if (!IsExtensionEnabled(ext)) {
> > +        if (!IsExtensionSupported(js, ext))
> > +            return nullptr;
> 
> if (!IsExtensionSupported(js, ext))
>     return nullptr;
> 
> if (!IsExtensionEnabled(ext))
>     EnableExtension(ext);
> 
> return mExtensions[ext];
> 
> (I hate nesting)

That's fine in general, but this is just a modification of a common pattern:
EnsureFoo:
  if (!foo)
    foo = Foo
  return foo

In our case, it's just not infallible. I think it's better how it is now.
Depends on: 1105870
Depends on: 1118284
Blocks: 1105535
You need to log in before you can comment on or make changes to this bug.