Bug 112359 - Check WEBGL_draw_buffers requirements before exposing the extension
Summary: Check WEBGL_draw_buffers requirements before exposing the extension
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-14 10:20 PDT by Zhenyao Mo
Modified: 2013-03-22 17:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch (16.22 KB, patch)
2013-03-18 11:26 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (16.34 KB, patch)
2013-03-18 17:26 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (17.64 KB, patch)
2013-03-19 11:53 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (17.83 KB, patch)
2013-03-19 15:51 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (18.71 KB, patch)
2013-03-21 17:20 PDT, Zhenyao Mo
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2013-03-14 10:20:08 PDT
A few WebGL requirements on top of EXT_draw_buffers

1) supports a minimum of 4 color attachments
2) MAX_COLOR_ATTACHMENTS >= MAX_DRAW_BUFFERS
3) check attaching n consecutive color attachments starting at COLOR_ATTACHMENT0_WEBGL returns FRAMEBUFFER_COMPLETE
    check attaching n consecutive color attachments starting at COLOR_ATTACHMENT0_WEBGL plus DEPTH returns FRAMEBUFFER_COMPLETE
    check attaching n consecutive color attachments starting at COLOR_ATTACHMENT0_WEBGL plus DEPTH_STENCIL returns FRAMEBUFFER_COMPLETE
Comment 1 Zhenyao Mo 2013-03-18 11:26:28 PDT
Created attachment 193613 [details]
Patch
Comment 2 Zhenyao Mo 2013-03-18 11:27:44 PDT
kbr: please have a look.

I will update the names from EXT_draw_buffers to WEBKIT_WEBGL_draw_buffers in the next patch, together with the enum name change (from _EXT postfix to _WEBGL postfix).
Comment 3 Kenneth Russell 2013-03-18 13:55:52 PDT
Gregg/Brandon, would it be possible for you to do an unofficial review of this?
Comment 4 Brandon Jones 2013-03-18 15:53:38 PDT
Comment on attachment 193613 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193613&action=review

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:107
> +    static bool ok = true;

Wouldn't it be easier to initialize ok to false, and set it to true once we've determined full support? That would remove the necessity for all the "ok = false" below.
Comment 5 Brandon Jones 2013-03-18 15:55:12 PDT
Other than the above nit, which you may want to ignore, looks good.
Comment 6 Gregg Tavares 2013-03-18 15:57:42 PDT
Comment on attachment 193613 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193613&action=review

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:73
> +    if (!n)

this won't work. If you call drawBuffersEXT([]) it should go into the driver which should set all of them to GL_NONE

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:106
> +    static bool computed = false;

these can't be static can it? If the context is lost, when the context is restored this will be stale

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:123
> +    GC3Dint texBinding = 0;

Why not just ask the WebGLRenderingContext to restore these when you're done rather than query them? I'm worried that querying is slow.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:153
> +        if (context->checkFramebufferStatus(GraphicsContext3D::FRAMEBUFFER) != GraphicsContext3D::FRAMEBUFFER_COMPLETE) {

Each call to checkFramebufferStatus is really slow. Maybe in another CL but we should probably have the command buffer do the same tests and add something like "GL_CHROMIUM_draw_buffers_webgl_compatible" so the WebKit code and skip these checks. Kind of like we check GL_CHROMIUM_resource_safe
Comment 7 Zhenyao Mo 2013-03-18 17:24:53 PDT
Comment on attachment 193613 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193613&action=review

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:73
>> +    if (!n)
> 
> this won't work. If you call drawBuffersEXT([]) it should go into the driver which should set all of them to GL_NONE

Fixed.

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:106
>> +    static bool computed = false;
> 
> these can't be static can it? If the context is lost, when the context is restored this will be stale

Fixed.  It will be per context lifetime, handing the caching in WebGLRenderingContext.

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:107
>> +    static bool ok = true;
> 
> Wouldn't it be easier to initialize ok to false, and set it to true once we've determined full support? That would remove the necessity for all the "ok = false" below.

It has to be the other way, because we want to quit when we encounters an error.  If OK init to true, then we need another way to track whether an error happened inside the for loop.

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:123
>> +    GC3Dint texBinding = 0;
> 
> Why not just ask the WebGLRenderingContext to restore these when you're done rather than query them? I'm worried that querying is slow.

Done

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:153
>> +        if (context->checkFramebufferStatus(GraphicsContext3D::FRAMEBUFFER) != GraphicsContext3D::FRAMEBUFFER_COMPLETE) {
> 
> Each call to checkFramebufferStatus is really slow. Maybe in another CL but we should probably have the command buffer do the same tests and add something like "GL_CHROMIUM_draw_buffers_webgl_compatible" so the WebKit code and skip these checks. Kind of like we check GL_CHROMIUM_resource_safe

OK, once we have the commandbuffer side implementation, I'll put this code inside a condition.
Comment 8 Zhenyao Mo 2013-03-18 17:26:26 PDT
Created attachment 193700 [details]
Patch
Comment 9 Zhenyao Mo 2013-03-18 17:28:43 PDT
Revised per reviews from bajones/gman.

kbr: please review.
Comment 10 Kenneth Russell 2013-03-18 17:30:59 PDT
Mo, could you get an LGTM from Gregg first?
Comment 11 Gregg Tavares 2013-03-18 20:08:54 PDT
Comment on attachment 193700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193700&action=review

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:84
> +        m_context->m_backDrawBuffer = bufs[0];

Sorry if this is a dumb question. I don't write enough WebKit code. But I find it strange that EXTDrawBuffers is directly setting fields on WebGLRenderingContext. Does this need a setter or is it okay?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2463
> +            bool satisfiesWebGLRequirements = false;

Isn't WebGLRenderingContext::getSupportedExtensions going to need this same logic? It seems like all of this should be inside EXTDrawBuffers::supported

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2469
> +                bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_framebufferBinding.get(), ec);

It seems like this restoring should be happening in EXTDrawBuffers::satisifiesWebGLRequirements. It would need to call

   context->RestoreCurrentFramebuffer()
   context->RestoreCurrentTexture()

Which are 2 new functions you'd have to add to WebGLRenderingContext.  Only EXTDrawBuffers::satisifiesWebGLRequirements knows what state it's trashing.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2780
> +                if (pname == Extensions3D::DRAW_BUFFER0_EXT && m_backDrawBuffer != GraphicsContext3D::NONE)

If I understand the code correctly, m_backDrawBuffer will always have a correct validated value which means this code seems like it could be

   if (pname == Extensions3D::DRAW_BUFFER0_EXT)
      value = m_backDrawBuffer

Yes?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:6079
> +    return getMaxDrawBuffers();

Is this correct? If so a comment would be good. Otherwise what's m_maxColorAttachments for if it's not actually used?



Sorry, I should have had these comments previously.
Comment 12 Zhenyao Mo 2013-03-19 11:46:27 PDT
Comment on attachment 193700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193700&action=review

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:84
>> +        m_context->m_backDrawBuffer = bufs[0];
> 
> Sorry if this is a dumb question. I don't write enough WebKit code. But I find it strange that EXTDrawBuffers is directly setting fields on WebGLRenderingContext. Does this need a setter or is it okay?

added a setter.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2463
>> +            bool satisfiesWebGLRequirements = false;
> 
> Isn't WebGLRenderingContext::getSupportedExtensions going to need this same logic? It seems like all of this should be inside EXTDrawBuffers::supported

you are correct.  So I added WebGLRenderingContext::supportsDrawBuffers() helper function to make sure EXTDrawBuffers::satisfiesWebGLRequirments() only called once unless context is lost and initialized.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2469
>> +                bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_framebufferBinding.get(), ec);
> 
> It seems like this restoring should be happening in EXTDrawBuffers::satisifiesWebGLRequirements. It would need to call
> 
>    context->RestoreCurrentFramebuffer()
>    context->RestoreCurrentTexture()
> 
> Which are 2 new functions you'd have to add to WebGLRenderingContext.  Only EXTDrawBuffers::satisifiesWebGLRequirements knows what state it's trashing.

Done.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2780
>> +                if (pname == Extensions3D::DRAW_BUFFER0_EXT && m_backDrawBuffer != GraphicsContext3D::NONE)
> 
> If I understand the code correctly, m_backDrawBuffer will always have a correct validated value which means this code seems like it could be
> 
>    if (pname == Extensions3D::DRAW_BUFFER0_EXT)
>       value = m_backDrawBuffer
> 
> Yes?

yes. modified.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:6079
>> +    return getMaxDrawBuffers();
> 
> Is this correct? If so a comment would be good. Otherwise what's m_maxColorAttachments for if it's not actually used?
> 
> 
> 
> Sorry, I should have had these comments previously.

So I changed the code to let m_maxColorAttachments and m_maxDrawBuffers reflects the real values returned by the driver. However, for getMaxDrawBuffers(), I return the min of the two values, whereas getMaxColorAttachments() returns m_maxColorAttachments.

This satisfies the WebGL requirements with minimum emulation.
Comment 13 Zhenyao Mo 2013-03-19 11:53:14 PDT
Created attachment 193887 [details]
Patch
Comment 14 Zhenyao Mo 2013-03-19 11:54:14 PDT
Revised per gman's review on second patch.

Please have another look.
Comment 15 WebKit Review Bot 2013-03-19 12:57:55 PDT
Comment on attachment 193887 [details]
Patch

Attachment 193887 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17257036

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-depth-texture.html
fast/canvas/webgl/webgl-depth-texture.html
Comment 16 Zhenyao Mo 2013-03-19 15:51:21 PDT
Created attachment 193941 [details]
Patch
Comment 17 Zhenyao Mo 2013-03-19 15:52:01 PDT
This one should fix the depth-texture failure on cr-linux bot.
Comment 18 Zhenyao Mo 2013-03-20 10:00:48 PDT
gman: any further comments?  If not, I'd like kbr to finish the official review.
Comment 19 Gregg Tavares 2013-03-20 16:02:04 PDT
lgtm
Comment 20 Zhenyao Mo 2013-03-20 19:18:23 PDT
Ken: please review.
Comment 21 Kenneth Russell 2013-03-21 15:51:28 PDT
Comment on attachment 193941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193941&action=review

Looks good overall, but a few issues.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:111
> +    context->getIntegerv(Extensions3D::MAX_DRAW_BUFFERS_EXT, &maxColorAttachments);

Looks like you're querying the wrong value here.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:114
> +        return ok;

You could just return false here and define "bool ok" later.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:116
> +    GC3Dint max = std::min(maxDrawBuffers, maxColorAttachments);

It may be a bad idea to call this variable "max".

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:125
> +#endif

This seems like a bug that maybe should be filed.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:163
> +            break;

The depth/stencil checks don't follow WebGL's rules. Specifically, WebGL (and GraphicsContext3D) add the DEPTH_STENCIL_ATTACHMENT entry point, forbid attaching textures to both the depth and stencil attachment points, and require certain formats for the depth, stencil, and depth/stencil attachment points. See http://www.khronos.org/registry/webgl/specs/latest/#6.6 . It looks like some ports' GraphicsContext3D implementations aren't doing sufficient checking. I think you need both an optional depth and depth/stencil texture, and need to try attaching and detaching each in turn.
Comment 22 Zhenyao Mo 2013-03-21 17:20:15 PDT
Created attachment 194393 [details]
Patch
Comment 23 Zhenyao Mo 2013-03-21 17:21:12 PDT
Comment on attachment 193941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193941&action=review

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:111
>> +    context->getIntegerv(Extensions3D::MAX_DRAW_BUFFERS_EXT, &maxColorAttachments);
> 
> Looks like you're querying the wrong value here.

done

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:114
>> +        return ok;
> 
> You could just return false here and define "bool ok" later.

done

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:116
>> +    GC3Dint max = std::min(maxDrawBuffers, maxColorAttachments);
> 
> It may be a bad idea to call this variable "max".

change to maxAllowedBuffers

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:125
>> +#endif
> 
> This seems like a bug that maybe should be filed.

I think the behavior is by design.  Chromium port initialize textures, so we can pass NULL to it, but for other ports, we have to provide the data to avoid info leaking.

As for no init data for depth/stencil texture, according to Gregg, it is due to the ANGLE implementation.  We don't have to impose that on other ports.

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:163
>> +            break;
> 
> The depth/stencil checks don't follow WebGL's rules. Specifically, WebGL (and GraphicsContext3D) add the DEPTH_STENCIL_ATTACHMENT entry point, forbid attaching textures to both the depth and stencil attachment points, and require certain formats for the depth, stencil, and depth/stencil attachment points. See http://www.khronos.org/registry/webgl/specs/latest/#6.6 . It looks like some ports' GraphicsContext3D implementations aren't doing sufficient checking. I think you need both an optional depth and depth/stencil texture, and need to try attaching and detaching each in turn.

done
Comment 24 Kenneth Russell 2013-03-22 16:49:41 PDT
Comment on attachment 194393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194393&action=review

Thanks for the updates. Looks good.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:150
> +        context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::STENCIL_ATTACHMENT, GraphicsContext3D::TEXTURE_2D, 0, 0);

Strictly speaking it isn't necessary to touch the DEPTH and STENCIL attachments here any more.

> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:165
> +            context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::STENCIL_ATTACHMENT, GraphicsContext3D::TEXTURE_2D, depthStencil, 0);

OK, I see your point that WebGLRenderingContext implements DEPTH_STENCIL_ATTACHMENT as two separate attachments at the GC3D level.
Comment 25 Zhenyao Mo 2013-03-22 17:22:53 PDT
Comment on attachment 194393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194393&action=review

>> Source/WebCore/html/canvas/EXTDrawBuffers.cpp:150
>> +        context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::STENCIL_ATTACHMENT, GraphicsContext3D::TEXTURE_2D, 0, 0);
> 
> Strictly speaking it isn't necessary to touch the DEPTH and STENCIL attachments here any more.

Removed
Comment 26 Zhenyao Mo 2013-03-22 17:47:47 PDT
Committed r146694: <http://trac.webkit.org/changeset/146694>