Bug 47379 – Move drawing buffer logic out of GraphicsContext3D and into DrawingBuffer

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Chris Marrin]

DrawingBuffer is a class which encapsulates an FBO used for drawing and the PlatformLayer used to composite it. It was created for accelerated 2D rendering which uses a shared GraphicsContext3D. But GC3D contains all the logic for supporting a drawing buffer already. So we should remove all that logic and move it into DrawingBuffer. This will make DrawingBuffer a general purpose class that can be used for both 2D and 3D accelerated rendering.

We also need to make the ownership rules between DrawingBuffer and GC3D more clear. Currently you create a DrawingBuffer directly and pass a GC3D to it. The DrawingBuffer keeps a weak pointer to the GC3D which can cause crashes if GC3D is destroyed. DrawingBuffer creation should be moved to GC3D, which should remember all the DrawingBuffers it allocated. When GC3D is destroyed, it should release all the resources of the DrawingBuffer and leave it an empty shell. When DrawingBuffer is destroyed, GC3D should be notified so it can clean up.

[Chris Marrin]

Created attachment 70152 [details]
Patch

[Early Warning System Bot]

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

[Chris Marrin]

Created attachment 70154 [details]
Patch fixing Qt build issue

[James Robinson]

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.

[WebKit Review Bot]

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

[Chris Marrin]

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

[Chris Marrin]

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

[WebKit Review Bot]

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

[Kenneth Russell]

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

[Chris Marrin]

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.