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
Created attachment 193613 [details] Patch
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).
Gregg/Brandon, would it be possible for you to do an unofficial review of this?
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.
Other than the above nit, which you may want to ignore, looks good.
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 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.
Created attachment 193700 [details] Patch
Revised per reviews from bajones/gman. kbr: please review.
Mo, could you get an LGTM from Gregg first?
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 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.
Created attachment 193887 [details] Patch
Revised per gman's review on second patch. Please have another look.
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
Created attachment 193941 [details] Patch
This one should fix the depth-texture failure on cr-linux bot.
gman: any further comments? If not, I'd like kbr to finish the official review.
lgtm
Ken: please review.
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.
Created attachment 194393 [details] Patch
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 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 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
Committed r146694: <http://trac.webkit.org/changeset/146694>