Bug 112359 – Check WEBGL_draw_buffers requirements before exposing the extension

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Zhenyao Mo]

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

[Zhenyao Mo]

Created attachment 193613 [details]
Patch

[Zhenyao Mo]

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).

[Kenneth Russell]

Gregg/Brandon, would it be possible for you to do an unofficial review of this?

[Brandon Jones]

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.

[Brandon Jones]

Other than the above nit, which you may want to ignore, looks good.

[Gregg Tavares]

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

[Zhenyao Mo]

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.

[Zhenyao Mo]

Created attachment 193700 [details]
Patch

[Zhenyao Mo]

Revised per reviews from bajones/gman.

kbr: please review.

[Kenneth Russell]

Mo, could you get an LGTM from Gregg first?

[Gregg Tavares]

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.

[Zhenyao Mo]

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.

[Zhenyao Mo]

Created attachment 193887 [details]
Patch

[Zhenyao Mo]

Revised per gman's review on second patch.

Please have another look.

[WebKit Review Bot]

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

[Zhenyao Mo]

Created attachment 193941 [details]
Patch

[Zhenyao Mo]

This one should fix the depth-texture failure on cr-linux bot.

[Zhenyao Mo]

gman: any further comments?  If not, I'd like kbr to finish the official review.

[Gregg Tavares]

lgtm

[Zhenyao Mo]

Ken: please review.

[Kenneth Russell]

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.

[Zhenyao Mo]

Created attachment 194393 [details]
Patch

[Zhenyao Mo]

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

[Kenneth Russell]

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.

[Zhenyao Mo]

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

[Zhenyao Mo]

Committed r146694: <http://trac.webkit.org/changeset/146694>