Bug 47379

Created attachment 70152 [details] Patch

Attachment 70152 [details] did not build on qt: Build output: http://queues.webkit.org/results/4187137

Created attachment 70154 [details] Patch fixing Qt build issue

This is a pretty big change in how back buffers are managed for WebGL. It also puts a lot of mac-specific logic in (for handling multisampling) that I think could be done in a more cross-platform way. I'd rather we break this up into a few smaller pieces and handle them one at a time: a.) Add multisampling support to DrawingBuffer. This should really all be cross-platform code implemented in terms of GraphicsContext3D, not in terms of direct GL calls, and shouldn't be in platform-specific code. One particular issue with this patch is it'll break multisampling for WebGL on all non-mac platforms, which would be a regression for Chromium. b.) Switch the model of having a default back buffer and compositing layer per GraphicsContext3D to having explicit back buffers (implemented as DrawingBuffers) with their own layers. This depends on multisampling (or it'd regress behavior) but can be done as a separate patch to make things clearer. c.) Change ownership models for DB / GC3D I'm not sure exactly when (c) should happen relative to (a) or (b). I would like to make any functional changes to DrawingBuffer before changing the WebGL or compositor paths at all to avoid regressions. The back buffer management code is actually quite subtle.

Attachment 70154 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4155144

(In reply to comment #4) > This is a pretty big change in how back buffers are managed for WebGL. It also puts a lot of mac-specific logic in (for handling multisampling) that I think could be done in a more cross-platform way. I'd rather we break this up into a few smaller pieces and handle them one at a time: > > a.) Add multisampling support to DrawingBuffer. This should really all be cross-platform code implemented in terms of GraphicsContext3D, not in terms of direct GL calls, and shouldn't be in platform-specific code. One particular issue with this patch is it'll break multisampling for WebGL on all non-mac platforms, which would be a regression for Chromium. Yeah, I didn't do it that way because it is pretty platform specific. But I think I can make GC3D calls and do the platform specifics there. How does this regress Chromium if the original code was in Mac specific files? > > b.) Switch the model of having a default back buffer and compositing layer per GraphicsContext3D to having explicit back buffers (implemented as DrawingBuffers) with their own layers. This depends on multisampling (or it'd regress behavior) but can be done as a separate patch to make things clearer. > > c.) Change ownership models for DB / GC3D > > I'm not sure exactly when (c) should happen relative to (a) or (b). I would like to make any functional changes to DrawingBuffer before changing the WebGL or compositor paths at all to avoid regressions. The back buffer management code is actually quite subtle. You mean my rewrite of DrawingBuffer management? I think the way I have it now is much safer that before. It should protect DrawingBuffers when their GC3D goes away and vice versa. I'm not sure doing it piecemeal will be helpful for regressions. I admit that this will change more Chromium code that I would like (since I can't debug Chromium code). What if I make all the changes PLATFORM(MAC) for now, so Chromium is not regressed? Then you can make the appropriate changes on the Chromium side so we are sharing code again. On the subject of sharing code, are you using GraphicsContext3DOpenGL.cpp? I split that file out with the hopes that it could be used (with a small number of ifdefs) on OpenGL, OpenGL ES and ANGLE implementations. So hopefully we'll be able to share that code, too.

Created attachment 70259 [details] Patch fixing Chromium build issue (does not address jamesr comments)

Attachment 70259 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4267018

(In reply to comment #6) > (In reply to comment #4) > > This is a pretty big change in how back buffers are managed for WebGL. It also puts a lot of mac-specific logic in (for handling multisampling) that I think could be done in a more cross-platform way. I'd rather we break this up into a few smaller pieces and handle them one at a time: > > > > a.) Add multisampling support to DrawingBuffer. This should really all be cross-platform code implemented in terms of GraphicsContext3D, not in terms of direct GL calls, and shouldn't be in platform-specific code. One particular issue with this patch is it'll break multisampling for WebGL on all non-mac platforms, which would be a regression for Chromium. > > Yeah, I didn't do it that way because it is pretty platform specific. But I think I can make GC3D calls and do the platform specifics there. How does this regress Chromium if the original code was in Mac specific files? Chromium already implements multisampling for its back buffer, but in its GraphicsContext3D implementation. Chromium's DrawingBuffer doesn't yet have multisampling support; adding it is more involved as we need to add extensions to GC3D (or, more precisely, the proposed Extensions class in https://bugs.webkit.org/show_bug.cgi?id=46894) and plumb them through to Chromium. > > b.) Switch the model of having a default back buffer and compositing layer per GraphicsContext3D to having explicit back buffers (implemented as DrawingBuffers) with their own layers. This depends on multisampling (or it'd regress behavior) but can be done as a separate patch to make things clearer. > > > > c.) Change ownership models for DB / GC3D > > > > I'm not sure exactly when (c) should happen relative to (a) or (b). I would like to make any functional changes to DrawingBuffer before changing the WebGL or compositor paths at all to avoid regressions. The back buffer management code is actually quite subtle. > > You mean my rewrite of DrawingBuffer management? I think the way I have it now is much safer that before. It should protect DrawingBuffers when their GC3D goes away and vice versa. > > I'm not sure doing it piecemeal will be helpful for regressions. I admit that this will change more Chromium code that I would like (since I can't debug Chromium code). What if I make all the changes PLATFORM(MAC) for now, so Chromium is not regressed? Then you can make the appropriate changes on the Chromium side so we are sharing code again. I think that if just the changes to WebGLRenderingContext were made #if PLATFORM(MAC), that would be sufficient to avoid regressions in this patch. James can confirm. This would be my preference. > On the subject of sharing code, are you using GraphicsContext3DOpenGL.cpp? I split that file out with the hopes that it could be used (with a small number of ifdefs) on OpenGL, OpenGL ES and ANGLE implementations. So hopefully we'll be able to share that code, too. No, it isn't possible for Chromium to use that file, at least at this time, because our OpenGL calls are serialized out to a separate process for execution. Instead we impose a virtual call under GraphicsContext3D (via WebGraphicsContext3D) in order to call our command buffer OpenGL library.

The drawing buffer lifetime management part of this is handled in https://bugs.webkit.org/show_bug.cgi?id=47501. The other 2 parts: adding multisampling support to DrawingBuffer, and changing GraphicsContext3D to use DrawingBuffers, are handled in https://bugs.webkit.org/show_bug.cgi?id=49206 and https://bugs.webkit.org/show_bug.cgi?id=47379. Closing this bug as obsolete.