Bug 43362 – Accelerated 2d canvases should get compositing layers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Accelerated 2d canvases should get compositing layers

[James Robinson]

Created attachment 63227 [details]
Patch

[Chris Marrin]

Comment on attachment 63227 [details]
Patch

As I have mentioned in https://bugs.webkit.org/show_bug.cgi?id=43210 I am still disagree with this entire approach. I don't think there should be a GLES2 specific ifdefs. If for some reason you disagree with my suggestion that you use GraphicsContext3D, you should at least create an equivalent 2D API Object, put that in platform and make CanvasRenderingContext2D use it. More comments inline...

>  void HTMLCanvasElement::makeRenderingResultsAvailable()
>  {
> -#if ENABLE(3D_CANVAS)
> -    if (is3D()) {
> -        WebGLRenderingContext* context3d = reinterpret_cast<WebGLRenderingContext*>(renderingContext());
> -        context3d->paintRenderingResultsToCanvas();
> -    }
> -#endif
> +    if (m_context)
> +        m_context->paintRenderingResultsToCanvas();
>  }

This will cause you to always make this call. Isn't your intent to have it happen only when you have an accelerated 2D or 3D canvas?

> diff --git a/WebCore/html/canvas/CanvasRenderingContext2D.cpp b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> index 6c3d372e66ccb7e3f6912596b1f0ea6290bf8a3f..f4e207ef2a80045ac0d679bd797b3f30b355b889 100644
> --- a/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> +++ b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> @@ -58,7 +58,17 @@
>  #include "StrokeStyleApplier.h"
>  #include "TextMetrics.h"
>  
> -#include <stdio.h>
> +#if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
> +#include "Chrome.h"
> +#include "ChromeClient.h"
> +#include "GLES2Context.h"
> +#if USE(ACCELERATED_COMPOSITING)
> +#include "RenderLayer.h"
> +#if PLATFORM(CHROMIUM)
> +#include "LayerChromium.h" // FIXME: this is odd, but we need it to initialize a RefPtr<PlatformLayer>
> +#endif
> +#endif
> +#endif

USE(GLES2_RENDERING) infects platform independent code with an ifdef that is specific to a particular rendering library, which I think it a bad idea. The name is poor no matter what since it seems your intent is to have this work on both desktop and mobile platforms. OpenGL ES 2.0 is only available on mobile platforms.

>  
>  #include <wtf/ByteArray.h>
>  #include <wtf/MathExtras.h>
> @@ -100,6 +110,12 @@ CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement* canvas, bo
>  #if ENABLE(DASHBOARD_SUPPORT)
>      , m_usesDashboardCompatibilityMode(usesDashboardCompatibilityMode)
>  #endif
> +#if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
> +    , m_gles2Context(0)
> +#if USE(ACCELERATED_COMPOSITING)
> +    , m_platformLayer(0)
> +#endif
> +#endif
>  {
>  #if !ENABLE(DASHBOARD_SUPPORT)
>      ASSERT_UNUSED(usesDashboardCompatibilityMode, !usesDashboardCompatibilityMode);
> @@ -108,6 +124,20 @@ CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement* canvas, bo
>      // Make sure that even if the drawingContext() has a different default
>      // thickness, it is in sync with the canvas thickness.
>      setLineWidth(lineWidth());
> +
> +#if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
> +    if (Page* p = canvas->document()->page()) {
> +        if (p->settings()->accelerated2dCanvasEnabled()) {
> +            // Set up our context and layer
> +            m_gles2Context = p->chrome()->client()->getOffscreenGLES2Context();
> +
> +            if (m_gles2Context) {
> +                if (GraphicsContext* c = drawingContext())
> +                    c->platformContext()->setGLES2Context(m_gles2Context.get(), IntSize(canvas->width(), canvas->height()));
> +            }
> +        }
> +    }
> +#endif

Again, having a member variable name that is specific to a particular rendering library seems like a bad idea in this platform independent code.

[James Robinson]

(In reply to comment #2)
> (From update of attachment 63227 [details])
> As I have mentioned in https://bugs.webkit.org/show_bug.cgi?id=43210 I am still disagree with this entire approach. I don't think there should be a GLES2 specific ifdefs. If for some reason you disagree with my suggestion that you use GraphicsContext3D, you should at least create an equivalent 2D API Object, put that in platform and make CanvasRenderingContext2D use it. More comments inline...

Thank you for looking at this patch.

I agree 100% that we should converge on a common abstraction.  The current stack for WebGL looks something like this:

WebGLRenderingContext -> GraphicsContext3D -> (platform layer that talks to some flavor of OpenGL)
Ideally the canvas 2d stack would just be:
CanvasRenderingContext2D -> GraphicsContext3D -> (platform layer that talks to some flavor of OpenGL)

However, GraphicsContext3D is currently too tightly coupled with WebGL to make this practical in the short term.  I've filed https://bugs.webkit.org/show_bug.cgi?id=43221 to fix this so that the canvas 2d work can use GC3D directly, but I don't think this should block canvas 2d work.  There is a large amount of basic functionality work needed to make accelerated 2d canvas work that is orthogonal to this part of the stack.  Refactoring GC3D risks destabilizing WebGL as well (see https://bugs.webkit.org/show_bug.cgi?id=43210#c13).

As a practical matter, it doesn't make too much of a difference whether one uses the GraphicsContext3D APIs or OpenGL ES 2 since GraphicsContext3D exposes the WebGL API, which is intentionally designed to match OpenGL ES 2.0.  After 43221 is fixed GC3D will be a wrapper for Open GL ES and it will be a mechanical matter to switch the calls over.

> 
> >  void HTMLCanvasElement::makeRenderingResultsAvailable()
> >  {
> > -#if ENABLE(3D_CANVAS)
> > -    if (is3D()) {
> > -        WebGLRenderingContext* context3d = reinterpret_cast<WebGLRenderingContext*>(renderingContext());
> > -        context3d->paintRenderingResultsToCanvas();
> > -    }
> > -#endif
> > +    if (m_context)
> > +        m_context->paintRenderingResultsToCanvas();
> >  }
> 
> This will cause you to always make this call. Isn't your intent to have it happen only when you have an accelerated 2D or 3D canvas?

The behavior is the same as before, the logic is just moved into CanvasRenderingContext rather than being on HTMLCanvasElement.  I've declared paintRenderingResultsToCanvas() virtual and made it do nothing by default.  WebGLRenderingContext overrides it to actually make the rendering results available always.  CanvasRenderingContext2D overrides it to make the rendering results available only when accelerated.

The practical difference is trading a downcast for a virtual function call.  I'm happy to put the call behind #ifdefs if performance is a concern.

> 
> > diff --git a/WebCore/html/canvas/CanvasRenderingContext2D.cpp b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > index 6c3d372e66ccb7e3f6912596b1f0ea6290bf8a3f..f4e207ef2a80045ac0d679bd797b3f30b355b889 100644
> > --- a/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > +++ b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > @@ -58,7 +58,17 @@
> >  #include "StrokeStyleApplier.h"
> >  #include "TextMetrics.h"
> >  
> > -#include <stdio.h>
> > +#if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
> > +#include "Chrome.h"
> > +#include "ChromeClient.h"
> > +#include "GLES2Context.h"
> > +#if USE(ACCELERATED_COMPOSITING)
> > +#include "RenderLayer.h"
> > +#if PLATFORM(CHROMIUM)
> > +#include "LayerChromium.h" // FIXME: this is odd, but we need it to initialize a RefPtr<PlatformLayer>
> > +#endif
> > +#endif
> > +#endif
> 
> USE(GLES2_RENDERING) infects platform independent code with an ifdef that is specific to a particular rendering library, which I think it a bad idea.
> The name is poor no matter what since it seems your intent is to have this work on both desktop and mobile platforms. OpenGL ES 2.0 is only available on mobile platforms.

The reason for using OpenGL ES 2.0 is that it is a subset of what is available across a wide set of desktop and mobile platforms (as opposed to using desktop GL features that may not be available on some mobile platforms).  ANGLE and chromium's command buffer implementation both accept direct OpenGL ES 2.0 calls as input and produce the appropriate calls to the native system (D3D 9 for ANGLE, OpenGL ES 2.0 or desktop OpenGL for the command buffer).  GraphicsContext3D is another translation layer in the same vein - it takes WebGL calls (which are really OpenGL ES 2.0 calls) as input and produces the appropriate calls for the platform.  As I mentioned above I want to use GC3D as the translation layer everywhere eventually, but this requires some refactoring work.

> 
> >  
> >  #include <wtf/ByteArray.h>
> >  #include <wtf/MathExtras.h>
> > @@ -100,6 +110,12 @@ CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement* canvas, bo
> >  #if ENABLE(DASHBOARD_SUPPORT)
> >      , m_usesDashboardCompatibilityMode(usesDashboardCompatibilityMode)
> >  #endif
> > +#if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
> > +    , m_gles2Context(0)
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    , m_platformLayer(0)
> > +#endif
> > +#endif
> >  {
> >  #if !ENABLE(DASHBOARD_SUPPORT)
> >      ASSERT_UNUSED(usesDashboardCompatibilityMode, !usesDashboardCompatibilityMode);
> > @@ -108,6 +124,20 @@ CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement* canvas, bo
> >      // Make sure that even if the drawingContext() has a different default
> >      // thickness, it is in sync with the canvas thickness.
> >      setLineWidth(lineWidth());
> > +
> > +#if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
> > +    if (Page* p = canvas->document()->page()) {
> > +        if (p->settings()->accelerated2dCanvasEnabled()) {
> > +            // Set up our context and layer
> > +            m_gles2Context = p->chrome()->client()->getOffscreenGLES2Context();
> > +
> > +            if (m_gles2Context) {
> > +                if (GraphicsContext* c = drawingContext())
> > +                    c->platformContext()->setGLES2Context(m_gles2Context.get(), IntSize(canvas->width(), canvas->height()));
> > +            }
> > +        }
> > +    }
> > +#endif
> 
> Again, having a member variable name that is specific to a particular rendering library seems like a bad idea in this platform independent code.

[Chris Marrin]

Please see https://bugs.webkit.org/show_bug.cgi?id=43210#c14, which I believe is a better approach to this implementation.

[Oliver Hunt]

Comment on attachment 63227 [details]
Patch

WebCore/html/HTMLCanvasElement.cpp:309
 +  }
I'm unsure why this is necessary, given you should never have a canvas that is both 2d and 3d at the same time, it would seem that a better approach would be to have a (names for discussion only, not necessarily actually good...):
enum CanvasType { Canvas2D, Canvas3D };

CanvasType type() { return m_type; }

...
CanvasType m_type;
...

WebCore/html/HTMLCanvasElement.h:114
 +  
As above

WebCore/html/canvas/CanvasRenderingContext2D.cpp:71
 +  #endif
These changes to GraphicsContext seem awful, ifdef's are icky and new code should try not to add them.

WebCore/html/canvas/CanvasRenderingContext2D.cpp:128
 +  #if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
I don't like this ifdefing, especially the ifdeffing on GLES2 (in addition to the ACCELERATED_2D_CANVAS check).  It seems much better to extend GraphicsContext3D to include the necessary APIs, and then the GLES2 ifdef's should go away, and on platforms that use full GL or D3D can "Just Work"

In all honesty I don't believe these changes should go forward as is -- I strongly believe that GraphicsContext3D should be the interface used for all hardware rendering, especially given the current apparent plan to use GLES as the acceleration model.

[Kenneth Russell]

(In reply to comment #5)
> In all honesty I don't believe these changes should go forward as is -- I strongly believe that GraphicsContext3D should be the interface used for all hardware rendering, especially given the current apparent plan to use GLES as the acceleration model.

Please, please, don't require this work to use GraphicsContext3D in order to proceed at the present time. We have a lot of work still remaining to do on WebGL before finalizing the first version of the specification, and large scale refactorings on the GraphicsContext3D class will severely impact our ability to complete this work in the planned time frame.

[Oliver Hunt]

(In reply to comment #6)
> (In reply to comment #5)
> > In all honesty I don't believe these changes should go forward as is -- I strongly believe that GraphicsContext3D should be the interface used for all hardware rendering, especially given the current apparent plan to use GLES as the acceleration model.
> 
> Please, please, don't require this work to use GraphicsContext3D in order to proceed at the present time. We have a lot of work still remaining to do on WebGL before finalizing the first version of the specification, and large scale refactorings on the GraphicsContext3D class will severely impact our ability to complete this work in the planned time frame.

This basically involves duplicating a whole heap of work, and i don't believe is a good design in the first place, landing bad code now on the assumption that "we'll fix it later" has almost always resulted in bunches of bad crufty code living in the tree for large amounts of time.

[Chris Marrin]

(In reply to comment #3)
> ...
> 
> I agree 100% that we should converge on a common abstraction.  The current stack for WebGL looks something like this:
> 
> WebGLRenderingContext -> GraphicsContext3D -> (platform layer that talks to some flavor of OpenGL)
> Ideally the canvas 2d stack would just be:
> CanvasRenderingContext2D -> GraphicsContext3D -> (platform layer that talks to some flavor of OpenGL)
> 
> However, GraphicsContext3D is currently too tightly coupled with WebGL to make this practical in the short term.  I've filed https://bugs.webkit.org/show_bug.cgi?id=43221 to fix this so that the canvas 2d work can use GC3D directly, but I don't think this should block canvas 2d work.  There is a large amount of basic functionality work needed to make accelerated 2d canvas work that is orthogonal to this part of the stack.  Refactoring GC3D risks destabilizing WebGL as well (see https://bugs.webkit.org/show_bug.cgi?id=43210#c13).

Can you post (here and/or in https://bugs.webkit.org/show_bug.cgi?id=43221) what you believe are the problems with GraphicsContext3D that make it WebGL specific and unusable as the API for Canvas 2D? If it's just a performance concern because GraphicsContext3D does extra checks needed by WebGL then it would be better to use GraphicsContext3D now and improve performance later. If not, maybe we can make small changes that will not destabilize WebGL.

> 
> As a practical matter, it doesn't make too much of a difference whether one uses the GraphicsContext3D APIs or OpenGL ES 2 since GraphicsContext3D exposes the WebGL API, which is intentionally designed to match OpenGL ES 2.0.  After 43221 is fixed GC3D will be a wrapper for Open GL ES and it will be a mechanical matter to switch the calls over.

The difference is that you'll be doing the work twice. Once to put it on top of OpenGL, which is a C API and once to put it on top of GraphicsContext3D, which is a C++ API and has a different way of dealing with GL objects. So switching it is more than a mechanical operation.

> 
> > 
> > >  void HTMLCanvasElement::makeRenderingResultsAvailable()
> > >  {
> > > -#if ENABLE(3D_CANVAS)
> > > -    if (is3D()) {
> > > -        WebGLRenderingContext* context3d = reinterpret_cast<WebGLRenderingContext*>(renderingContext());
> > > -        context3d->paintRenderingResultsToCanvas();
> > > -    }
> > > -#endif
> > > +    if (m_context)
> > > +        m_context->paintRenderingResultsToCanvas();
> > >  }
> > 
> > This will cause you to always make this call. Isn't your intent to have it happen only when you have an accelerated 2D or 3D canvas?
> 
> The behavior is the same as before, the logic is just moved into CanvasRenderingContext rather than being on HTMLCanvasElement.  I've declared paintRenderingResultsToCanvas() virtual and made it do nothing by default.  WebGLRenderingContext overrides it to actually make the rendering results available always.  CanvasRenderingContext2D overrides it to make the rendering results available only when accelerated.
> 
> The practical difference is trading a downcast for a virtual function call.  I'm happy to put the call behind #ifdefs if performance is a concern.

No, I see what you're doing now. It's fine the way you have it.

> 
> > 
> > > diff --git a/WebCore/html/canvas/CanvasRenderingContext2D.cpp b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > > index 6c3d372e66ccb7e3f6912596b1f0ea6290bf8a3f..f4e207ef2a80045ac0d679bd797b3f30b355b889 100644
> > > --- a/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > > +++ b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > > @@ -58,7 +58,17 @@
> > >  #include "StrokeStyleApplier.h"
> > >  #include "TextMetrics.h"
> > >  
> > > -#include <stdio.h>
> > > +#if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
> > > +#include "Chrome.h"
> > > +#include "ChromeClient.h"
> > > +#include "GLES2Context.h"
> > > +#if USE(ACCELERATED_COMPOSITING)
> > > +#include "RenderLayer.h"
> > > +#if PLATFORM(CHROMIUM)
> > > +#include "LayerChromium.h" // FIXME: this is odd, but we need it to initialize a RefPtr<PlatformLayer>
> > > +#endif
> > > +#endif
> > > +#endif
> > 
> > USE(GLES2_RENDERING) infects platform independent code with an ifdef that is specific to a particular rendering library, which I think it a bad idea.
> > The name is poor no matter what since it seems your intent is to have this work on both desktop and mobile platforms. OpenGL ES 2.0 is only available on mobile platforms.
> 
> The reason for using OpenGL ES 2.0 is that it is a subset of what is available across a wide set of desktop and mobile platforms (as opposed to using desktop GL features that may not be available on some mobile platforms).  ANGLE and chromium's command buffer implementation both accept direct OpenGL ES 2.0 calls as input and produce the appropriate calls to the native system (D3D 9 for ANGLE, OpenGL ES 2.0 or desktop OpenGL for the command buffer).  GraphicsContext3D is another translation layer in the same vein - it takes WebGL calls (which are really OpenGL ES 2.0 calls) as input and produces the appropriate calls for the platform.  As I mentioned above I want to use GC3D as the translation layer everywhere eventually, but this requires some refactoring work.

But we're not talking about a subset. It would be nice if GL ES 2.0 were a subset of desktop OpenGL, but it's not. There are differences in GLSL and some things that are standard in GL ES 2.0 are extensions in desktop OpenGL. That means some hardware will either not have the required support or will have to have wrappers to make it behave like you expect. These are all things we have been wrestling with in WebGL which you would have to do over if you use your own shim.

[Kenneth Russell]

(In reply to comment #7)
> This basically involves duplicating a whole heap of work, and i don't believe is a good design in the first place, landing bad code now on the assumption that "we'll fix it later" has almost always resulted in bunches of bad crufty code living in the tree for large amounts of time.

It really doesn't involve code duplication. GraphicsContext3D attempts to abstract over the OpenGL API. The changes in this work focus on providing an OpenGL ES 2.0 context abstraction to WebCore without replicating all of the OpenGL ES 2.0 entry points. I think we may want to reconsider how GraphicsContext3D is supported in the future based on the results of this work.

[Chris Marrin]

(In reply to comment #9)
> (In reply to comment #7)
> > This basically involves duplicating a whole heap of work, and i don't believe is a good design in the first place, landing bad code now on the assumption that "we'll fix it later" has almost always resulted in bunches of bad crufty code living in the tree for large amounts of time.
> 
> It really doesn't involve code duplication. GraphicsContext3D attempts to abstract over the OpenGL API. The changes in this work focus on providing an OpenGL ES 2.0 context abstraction to WebCore without replicating all of the OpenGL ES 2.0 entry points. I think we may want to reconsider how GraphicsContext3D is supported in the future based on the results of this work.

I'm not sure what you mean by this. GraphicsContext3D provides an OpenGL ES 2.0 context abstraction. It happens to support most if not all OpenGL ES 2.0 calls one way or another, but the fact that the use in Context 2D doesn't use that full API should not be an issues. 

I also don't know what you mean when you say we should reconsider how GraphicsContext3D is supported. It seems clear that GraphicsContext3D is the 3D wrapper around OpenGL ES 2.0, much as GraphicsContext is a wrapper around Core Graphics. GraphicsContext has interfaces to other 2D libraries, just like GraphicsContext3D has interfaces to other 3D libraries. I agree that there may be logic in GraphicsContext3D that is needed to protect JavaScript authors from misusing the API which add overhead. These additions can be removed to a higher level wrapper specifically for WebGL. But I don't that separation should block this work.

[James Robinson]

(In reply to comment #8)
> (In reply to comment #3)
> > ...
> > 
> > I agree 100% that we should converge on a common abstraction.  The current stack for WebGL looks something like this:
> > 
> > WebGLRenderingContext -> GraphicsContext3D -> (platform layer that talks to some flavor of OpenGL)
> > Ideally the canvas 2d stack would just be:
> > CanvasRenderingContext2D -> GraphicsContext3D -> (platform layer that talks to some flavor of OpenGL)
> > 
> > However, GraphicsContext3D is currently too tightly coupled with WebGL to make this practical in the short term.  I've filed https://bugs.webkit.org/show_bug.cgi?id=43221 to fix this so that the canvas 2d work can use GC3D directly, but I don't think this should block canvas 2d work.  There is a large amount of basic functionality work needed to make accelerated 2d canvas work that is orthogonal to this part of the stack.  Refactoring GC3D risks destabilizing WebGL as well (see https://bugs.webkit.org/show_bug.cgi?id=43210#c13).
> 
> Can you post (here and/or in https://bugs.webkit.org/show_bug.cgi?id=43221) what you believe are the problems with GraphicsContext3D that make it WebGL specific and unusable as the API for Canvas 2D? If it's just a performance concern because GraphicsContext3D does extra checks needed by WebGL then it would be better to use GraphicsContext3D now and improve performance later. If not, maybe we can make small changes that will not destabilize WebGL.

It's mostly around layering - GraphicsContext3D is currently aware of WebGL types and WebGLRenderingContext.  All of those back pointers will have to be abstracted in some way.  This may not actually be too much work - I will investigate and see precisely what is missing.

> 
> > 
> > As a practical matter, it doesn't make too much of a difference whether one uses the GraphicsContext3D APIs or OpenGL ES 2 since GraphicsContext3D exposes the WebGL API, which is intentionally designed to match OpenGL ES 2.0.  After 43221 is fixed GC3D will be a wrapper for Open GL ES and it will be a mechanical matter to switch the calls over.
> 
> The difference is that you'll be doing the work twice. Once to put it on top of OpenGL, which is a C API and once to put it on top of GraphicsContext3D, which is a C++ API and has a different way of dealing with GL objects. So switching it is more than a mechanical operation.

Good point.

[Chris Marrin]

(In reply to comment #11)
> (In reply to comment #8)
> ...
> > 
> > Can you post (here and/or in https://bugs.webkit.org/show_bug.cgi?id=43221) what you believe are the problems with GraphicsContext3D that make it WebGL specific and unusable as the API for Canvas 2D? If it's just a performance concern because GraphicsContext3D does extra checks needed by WebGL then it would be better to use GraphicsContext3D now and improve performance later. If not, maybe we can make small changes that will not destabilize WebGL.
> 
> It's mostly around layering - GraphicsContext3D is currently aware of WebGL types and WebGLRenderingContext.  All of those back pointers will have to be abstracted in some way.  This may not actually be too much work - I will investigate and see precisely what is missing.

Right, that's the layering violations that Ken refers to. But that work doesn't have to block this work, does it?

OBTW, I set the component in https://bugs.webkit.org/show_bug.cgi?id=43221 to WebGL (mostly so I can find it :-)

[James Robinson]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #8)
> > ...
> > > 
> > > Can you post (here and/or in https://bugs.webkit.org/show_bug.cgi?id=43221) what you believe are the problems with GraphicsContext3D that make it WebGL specific and unusable as the API for Canvas 2D? If it's just a performance concern because GraphicsContext3D does extra checks needed by WebGL then it would be better to use GraphicsContext3D now and improve performance later. If not, maybe we can make small changes that will not destabilize WebGL.
> > 
> > It's mostly around layering - GraphicsContext3D is currently aware of WebGL types and WebGLRenderingContext.  All of those back pointers will have to be abstracted in some way.  This may not actually be too much work - I will investigate and see precisely what is missing.
> 
> Right, that's the layering violations that Ken refers to. But that work doesn't have to block this work, does it?

I'll go find out and update the bug.  I hope not!
> 
> OBTW, I set the component in https://bugs.webkit.org/show_bug.cgi?id=43221 to WebGL (mostly so I can find it :-)

[Stephen White]

(In reply to comment #8)
> (In reply to comment #3)
> > ...
> > 
> > I agree 100% that we should converge on a common abstraction.  The current stack for WebGL looks something like this:
> > 
> > WebGLRenderingContext -> GraphicsContext3D -> (platform layer that talks to some flavor of OpenGL)
> > Ideally the canvas 2d stack would just be:
> > CanvasRenderingContext2D -> GraphicsContext3D -> (platform layer that talks to some flavor of OpenGL)
> > 
> > However, GraphicsContext3D is currently too tightly coupled with WebGL to make this practical in the short term.  I've filed https://bugs.webkit.org/show_bug.cgi?id=43221 to fix this so that the canvas 2d work can use GC3D directly, but I don't think this should block canvas 2d work.  There is a large amount of basic functionality work needed to make accelerated 2d canvas work that is orthogonal to this part of the stack.  Refactoring GC3D risks destabilizing WebGL as well (see https://bugs.webkit.org/show_bug.cgi?id=43210#c13).
> 
> Can you post (here and/or in https://bugs.webkit.org/show_bug.cgi?id=43221) what you believe are the problems with GraphicsContext3D that make it WebGL specific and unusable as the API for Canvas 2D? If it's just a performance concern because GraphicsContext3D does extra checks needed by WebGL then it would be better to use GraphicsContext3D now and improve performance later. If not, maybe we can make small changes that will not destabilize WebGL.
> 
> > 
> > As a practical matter, it doesn't make too much of a difference whether one uses the GraphicsContext3D APIs or OpenGL ES 2 since GraphicsContext3D exposes the WebGL API, which is intentionally designed to match OpenGL ES 2.0.  After 43221 is fixed GC3D will be a wrapper for Open GL ES and it will be a mechanical matter to switch the calls over.
> 
> The difference is that you'll be doing the work twice. Once to put it on top of OpenGL, which is a C API and once to put it on top of GraphicsContext3D, which is a C++ API and has a different way of dealing with GL objects. So switching it is more than a mechanical operation.

> 
> > 
> > > 
> > > >  void HTMLCanvasElement::makeRenderingResultsAvailable()
> > > >  {
> > > > -#if ENABLE(3D_CANVAS)
> > > > -    if (is3D()) {
> > > > -        WebGLRenderingContext* context3d = reinterpret_cast<WebGLRenderingContext*>(renderingContext());
> > > > -        context3d->paintRenderingResultsToCanvas();
> > > > -    }
> > > > -#endif
> > > > +    if (m_context)
> > > > +        m_context->paintRenderingResultsToCanvas();
> > > >  }
> > > 
> > > This will cause you to always make this call. Isn't your intent to have it happen only when you have an accelerated 2D or 3D canvas?
> > 
> > The behavior is the same as before, the logic is just moved into CanvasRenderingContext rather than being on HTMLCanvasElement.  I've declared paintRenderingResultsToCanvas() virtual and made it do nothing by default.  WebGLRenderingContext overrides it to actually make the rendering results available always.  CanvasRenderingContext2D overrides it to make the rendering results available only when accelerated.
> > 
> > The practical difference is trading a downcast for a virtual function call.  I'm happy to put the call behind #ifdefs if performance is a concern.
> 
> No, I see what you're doing now. It's fine the way you have it.
> 
> > 
> > > 
> > > > diff --git a/WebCore/html/canvas/CanvasRenderingContext2D.cpp b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > > > index 6c3d372e66ccb7e3f6912596b1f0ea6290bf8a3f..f4e207ef2a80045ac0d679bd797b3f30b355b889 100644
> > > > --- a/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > > > +++ b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> > > > @@ -58,7 +58,17 @@
> > > >  #include "StrokeStyleApplier.h"
> > > >  #include "TextMetrics.h"
> > > >  
> > > > -#include <stdio.h>
> > > > +#if ENABLE(ACCELERATED_2D_CANVAS) && USE(GLES2_RENDERING)
> > > > +#include "Chrome.h"
> > > > +#include "ChromeClient.h"
> > > > +#include "GLES2Context.h"
> > > > +#if USE(ACCELERATED_COMPOSITING)
> > > > +#include "RenderLayer.h"
> > > > +#if PLATFORM(CHROMIUM)
> > > > +#include "LayerChromium.h" // FIXME: this is odd, but we need it to initialize a RefPtr<PlatformLayer>
> > > > +#endif
> > > > +#endif
> > > > +#endif
> > > 
> > > USE(GLES2_RENDERING) infects platform independent code with an ifdef that is specific to a particular rendering library, which I think it a bad idea.
> > > The name is poor no matter what since it seems your intent is to have this work on both desktop and mobile platforms. OpenGL ES 2.0 is only available on mobile platforms.
> > 
> > The reason for using OpenGL ES 2.0 is that it is a subset of what is available across a wide set of desktop and mobile platforms (as opposed to using desktop GL features that may not be available on some mobile platforms).  ANGLE and chromium's command buffer implementation both accept direct OpenGL ES 2.0 calls as input and produce the appropriate calls to the native system (D3D 9 for ANGLE, OpenGL ES 2.0 or desktop OpenGL for the command buffer).  GraphicsContext3D is another translation layer in the same vein - it takes WebGL calls (which are really OpenGL ES 2.0 calls) as input and produces the appropriate calls for the platform.  As I mentioned above I want to use GC3D as the translation layer everywhere eventually, but this requires some refactoring work.
> 
> But we're not talking about a subset. It would be nice if GL ES 2.0 were a subset of desktop OpenGL, but it's not. There are differences in GLSL and some things that are standard in GL ES 2.0 are extensions in desktop OpenGL. That means some hardware will either not have the required support or will have to have wrappers to make it behave like you expect. These are all things we have been wrestling with in WebGL which you would have to do over if you use your own shim.

As I mentioned on the master bug, we had to do that duplication of work anyway (and the work is already done).  We can only make D3D/GL calls from within the GPU process, whose interface is GLES 2.0.  So the only GraphicsContext3D backend that Chrome can use is the one that marshalls to the GPU process via the GLES2 stubs.  From the Chrome point of view, GraphicsContext3D is a extra layer of abstraction we don't need.

Note that GLES2Context is probably badly-named.  It's a simple interface that has no more dependencies on GLES2 than GraphicsContext3D does on 3D.  It provides things such as SwapBuffers, MakeCurrent, etc that any glX or wgl implementation would have to abstract.  (Note that GLES2Context and the marshalled GLES2 calls are also used by Chrome's implementation of accelerated compositing and CSS 3D transforms.)

If the goal is to clean up the #ifdef GLES2_RENDERING (which I agree are ugly -- I tried to hide them in the platform layer at first but failed), we could easily provide a stubbed-out implementation of GLES2Context for the other platforms, and then the #ifdefs could be removed.  The code would still be protected by the ENABLE and the runtime flag.

[Chris Marrin]

(In reply to comment #14)
> ...
> > > The reason for using OpenGL ES 2.0 is that it is a subset of what is available across a wide set of desktop and mobile platforms (as opposed to using desktop GL features that may not be available on some mobile platforms).  ANGLE and chromium's command buffer implementation both accept direct OpenGL ES 2.0 calls as input and produce the appropriate calls to the native system (D3D 9 for ANGLE, OpenGL ES 2.0 or desktop OpenGL for the command buffer).  GraphicsContext3D is another translation layer in the same vein - it takes WebGL calls (which are really OpenGL ES 2.0 calls) as input and produces the appropriate calls for the platform.  As I mentioned above I want to use GC3D as the translation layer everywhere eventually, but this requires some refactoring work.
> > 
> > But we're not talking about a subset. It would be nice if GL ES 2.0 were a subset of desktop OpenGL, but it's not. There are differences in GLSL and some things that are standard in GL ES 2.0 are extensions in desktop OpenGL. That means some hardware will either not have the required support or will have to have wrappers to make it behave like you expect. These are all things we have been wrestling with in WebGL which you would have to do over if you use your own shim.
> 
> As I mentioned on the master bug, we had to do that duplication of work anyway (and the work is already done).  We can only make D3D/GL calls from within the GPU process, whose interface is GLES 2.0.  So the only GraphicsContext3D backend that Chrome can use is the one that marshalls to the GPU process via the GLES2 stubs.  From the Chrome point of view, GraphicsContext3D is a extra layer of abstraction we don't need.

But you're replacing the abstraction with a bunch of ifdefs in CanvasRenderingContext2D, which is a mistake. Currently CanvasRenderingContext2D uses GraphicsContext as its renderer, and it should stay that way. Any of your work should be at the level of GraphicsContext. If that means making a version of GraphicsContext that uses your GLES2Context then that's fine. But you shouldn't be making changes directly to CanvasRenderingContext2D.

But my main point is that I think you would be doing a lot less work if you made a version of GraphicsContext that used GraphicsContext3D as its renderer. We can get the WebGL specifics out of GraphicsContext3D to make it more appropriate for this task. Doing that, you could reasonably argue, is an extra layer that might affect performance, making a direct GLES2Context implementation on GraphicsContext more desirable. I don't think the overhead would be that significant, but it's arguable.

> 
> Note that GLES2Context is probably badly-named.  It's a simple interface that has no more dependencies on GLES2 than GraphicsContext3D does on 3D.  It provides things such as SwapBuffers, MakeCurrent, etc that any glX or wgl implementation would have to abstract.  (Note that GLES2Context and the marshalled GLES2 calls are also used by Chrome's implementation of accelerated compositing and CSS 3D transforms.)

Yes, I understand your point now. But doesn't WebGL use this same API? If so, since WebGL has to go through GraphicsContext3D (eventually getting to your GLES2Context API) shouldn't we try to use GraphicsContext3D as the API for Canvas 2D, as long as it can be done without significant overhead?

> 
> If the goal is to clean up the #ifdef GLES2_RENDERING (which I agree are ugly -- I tried to hide them in the platform layer at first but failed), we could easily provide a stubbed-out implementation of GLES2Context for the other platforms, and then the #ifdefs could be removed.  The code would still be protected by the ENABLE and the runtime flag.

I think the goal has to be to make accelerated 2D rendering possible on all platforms as long as they implement the proper platform-specific classes. So ifdefs having to do with accelerated vs. non-accelerated implementations are fine, but not those having to do with a specific rendering API.

[Stephen White]

(In reply to comment #15)
> (In reply to comment #14)
> > As I mentioned on the master bug, we had to do that duplication of work anyway (and the work is already done).  We can only make D3D/GL calls from within the GPU process, whose interface is GLES 2.0.  So the only GraphicsContext3D backend that Chrome can use is the one that marshalls to the GPU process via the GLES2 stubs.  From the Chrome point of view, GraphicsContext3D is a extra layer of abstraction we don't need.
> 
> But you're replacing the abstraction with a bunch of ifdefs in CanvasRenderingContext2D, which is a mistake. Currently CanvasRenderingContext2D uses GraphicsContext as its renderer, and it should stay that way. Any of your work should be at the level of GraphicsContext. If that means making a version of GraphicsContext that uses your GLES2Context then that's fine. But you shouldn't be making changes directly to CanvasRenderingContext2D.

There are two things that CanvasRenderingContext2D does with GLES2Context inside those #ifdefs:  create it, and tell the GraphicsContext inside the canvas's ImageBuffer to use it.  I tried to push the creation and ownership down into GraphicsContext, but unfortunately it currently needs access to the ChromeClient in order to share resources with the compositor attached to that Page, and passing a Page* or ChromeClient* down into the platform/graphics level felt like a layering violation to me.

The other problem with putting ownership of the GLES2Context (or GraphicsContext3D) inside ImageBuffer's GraphicsContext is that 2D canvas likes to recreate that ImageBuffer a lot -- in particular, when the canvas is reset by doing canvas.width = width.  This is fine for GraphicsContext, but doing so for GLES2Context is expensive (as it is for GraphicsContext3D, I imagine), so we needed a way to keep the GLES2Context alive over those resets.  This might be fixable by having 2D canvas behave a little more like WebGL, and not recreate its ImageBuffer, or to move ownership of the GraphicsContext out of the ImageBuffer.

> But my main point is that I think you would be doing a lot less work if you made a version of GraphicsContext that used GraphicsContext3D as its renderer. We can get the WebGL specifics out of GraphicsContext3D to make it more appropriate for this task. Doing that, you could reasonably argue, is an extra layer that might affect performance, making a direct GLES2Context implementation on GraphicsContext more desirable. I don't think the overhead would be that significant, but it's arguable.

Performance is not worth arguing about:  the only real answer is to try it out and benchmark it.  That's what James is currently doing, if it doesn't take too long.

As to doing a lot less work, maybe I'm being dense, but I still don't see what GraphicsContext3D offers over straight GLES2 (for Chrome; I can see what it offers for other ports).  Using GraphicsContext3D instead would mostly just mean (e.g.) calling ctx->drawElements() instead of glDrawElements(), no?  We'd still have to write the shaders, make the calls to compile them, create the vert buffers, bind them, etc.

You'll notice that GLES2Canvas (not context) actually does bear a striking resemblance to GraphicsContext.  This is not a coincidence -- in fact, I hope that it will one day become a full implementation.   One of the reasons it is not today is that none of the functions on GraphicsContext are virtual, so having two implementations in the same binary is rather difficult.  Making them all virtual I thought might be objectionable to other ports, so for now GLES2Canvas is essentially "bolted onto" GraphicsContextSkia.  We also need it to be switchable on the fly with the software path, in order to support older machines and for those functions we haven't implemented yet.

> > Note that GLES2Context is probably badly-named.  It's a simple interface that has no more dependencies on GLES2 than GraphicsContext3D does on 3D.  It provides things such as SwapBuffers, MakeCurrent, etc that any glX or wgl implementation would have to abstract.  (Note that GLES2Context and the marshalled GLES2 calls are also used by Chrome's implementation of accelerated compositing and CSS 3D transforms.)
> 
> Yes, I understand your point now. But doesn't WebGL use this same API? If so, since WebGL has to go through GraphicsContext3D (eventually getting to your GLES2Context API) shouldn't we try to use GraphicsContext3D as the API for Canvas 2D, as long as it can be done without significant overhead?

It's worth trying, which is what James is doing.  Aside from performance, there's always the risk of bugs that having more code involved brings.  Finally, we have a pretty good infrastructure for surfacing GLES2 extensions from the GPU process, which we planned to start using.  If we use GraphicsContext3D, we'll have to start plumbing those through.

> 
> > 
> > If the goal is to clean up the #ifdef GLES2_RENDERING (which I agree are ugly -- I tried to hide them in the platform layer at first but failed), we could easily provide a stubbed-out implementation of GLES2Context for the other platforms, and then the #ifdefs could be removed.  The code would still be protected by the ENABLE and the runtime flag.
> 
> I think the goal has to be to make accelerated 2D rendering possible on all platforms as long as they implement the proper platform-specific classes. So ifdefs having to do with accelerated vs. non-accelerated implementations are fine, but not those having to do with a specific rendering API.

Here's another possible solution to that problem (that would require less change from our current implementation):

in GraphicsContext.h (or elsewhere in platform/graphics/*.h):

#if PLATFORM(CHROMIUM)
typedef GLES2Context AcceleratedGraphicsContext
#else
typedef GraphicsContext3D AcceleratedGraphicsContext
#endif

in CanvasRenderingContext2D.h:

OwnPtr<GLES2Context> m_gles2Context
becomes
OwnPtr<AcceleratedGraphicsContext> m_acceleratedGraphicsContext;

This would allow us to remove the #ifdefs, and to allow other ports to do a GraphicsContext3D implementation without requiring us to do one today.

[Chris Marrin]

(In reply to comment #16)
> ...
> > But you're replacing the abstraction with a bunch of ifdefs in CanvasRenderingContext2D, which is a mistake. Currently CanvasRenderingContext2D uses GraphicsContext as its renderer, and it should stay that way. Any of your work should be at the level of GraphicsContext. If that means making a version of GraphicsContext that uses your GLES2Context then that's fine. But you shouldn't be making changes directly to CanvasRenderingContext2D.
> 
> There are two things that CanvasRenderingContext2D does with GLES2Context inside those #ifdefs:  create it, and tell the GraphicsContext inside the canvas's ImageBuffer to use it.  I tried to push the creation and ownership down into GraphicsContext, but unfortunately it currently needs access to the ChromeClient in order to share resources with the compositor attached to that Page, and passing a Page* or ChromeClient* down into the platform/graphics level felt like a layering violation to me.
> 
> The other problem with putting ownership of the GLES2Context (or GraphicsContext3D) inside ImageBuffer's GraphicsContext is that 2D canvas likes to recreate that ImageBuffer a lot -- in particular, when the canvas is reset by doing canvas.width = width.  This is fine for GraphicsContext, but doing so for GLES2Context is expensive (as it is for GraphicsContext3D, I imagine), so we needed a way to keep the GLES2Context alive over those resets.  This might be fixable by having 2D canvas behave a little more like WebGL, and not recreate its ImageBuffer, or to move ownership of the GraphicsContext out of the ImageBuffer.

First of all let me say that I think the source of the problem here is ImageBuffer and the fact that it "owns" the GraphicsContext. This is compounded by the fact that the ImageBuffer is owned by the HTMLCanvasElement rather than the CanvasRenderingContext2D (I think, I don't have access to the code at the moment). So I see why you're having such difficulty!

You'll notice that we don't even use the ImageBuffer in HTMLCanvasElement in WebGLRenderingContext. So maybe we can just make GraphicsContext behave more like GraphicsContext3D to solve this problem. It would just be a matter of initializing GraphicsContext to have its own ImageBuffer (conceptually) which gets composited with some sort of paint call(s). Today WebGLRenderingContext works with both accelerated and non-accelerated compositing models, so it should be possible to do the same with the 2D Canvas.

I think if you first focused on duplicating the WebGLRenderingContext compositing model in CanvasRenderingContext2D, I think it will be easier to make a GraphicsContext implementation on top of GLES2Context (whether directly or on top of GraphicsContext3D).

[Stephen White]

(In reply to comment #17)
> (In reply to comment #16)
> > ...
> > > But you're replacing the abstraction with a bunch of ifdefs in CanvasRenderingContext2D, which is a mistake. Currently CanvasRenderingContext2D uses GraphicsContext as its renderer, and it should stay that way. Any of your work should be at the level of GraphicsContext. If that means making a version of GraphicsContext that uses your GLES2Context then that's fine. But you shouldn't be making changes directly to CanvasRenderingContext2D.
> > 
> > There are two things that CanvasRenderingContext2D does with GLES2Context inside those #ifdefs:  create it, and tell the GraphicsContext inside the canvas's ImageBuffer to use it.  I tried to push the creation and ownership down into GraphicsContext, but unfortunately it currently needs access to the ChromeClient in order to share resources with the compositor attached to that Page, and passing a Page* or ChromeClient* down into the platform/graphics level felt like a layering violation to me.
> > 
> > The other problem with putting ownership of the GLES2Context (or GraphicsContext3D) inside ImageBuffer's GraphicsContext is that 2D canvas likes to recreate that ImageBuffer a lot -- in particular, when the canvas is reset by doing canvas.width = width.  This is fine for GraphicsContext, but doing so for GLES2Context is expensive (as it is for GraphicsContext3D, I imagine), so we needed a way to keep the GLES2Context alive over those resets.  This might be fixable by having 2D canvas behave a little more like WebGL, and not recreate its ImageBuffer, or to move ownership of the GraphicsContext out of the ImageBuffer.
> 
> First of all let me say that I think the source of the problem here is ImageBuffer and the fact that it "owns" the GraphicsContext. This is compounded by the fact that the ImageBuffer is owned by the HTMLCanvasElement rather than the CanvasRenderingContext2D (I think, I don't have access to the code at the moment). So I see why you're having such difficulty!
> 
> You'll notice that we don't even use the ImageBuffer in HTMLCanvasElement in WebGLRenderingContext. So maybe we can just make GraphicsContext behave more like GraphicsContext3D to solve this problem. It would just be a matter of initializing GraphicsContext to have its own ImageBuffer (conceptually) which gets composited with some sort of paint call(s). Today WebGLRenderingContext works with both accelerated and non-accelerated compositing models, so it should be possible to do the same with the 2D Canvas.
> 
> I think if you first focused on duplicating the WebGLRenderingContext compositing model in CanvasRenderingContext2D, I think it will be easier to make a GraphicsContext implementation on top of GLES2Context (whether directly or on top of GraphicsContext3D).

It would be nice to do so, but changing this ownership risks destabilizing the canvas implementation other ports, and (as you indicated) is orthogonal to the choice of rendering backend.  So I'd prefer to deal with those two issues separately, if possible.

James is having some trouble getting the GraphicsContext3D implementation to work, so I'm uploading another patch for your consideration.  This one removes the use of #if USE(GLES2_RENDERING)s by using an  AcceleratedGraphicsContext typedef in GraphicsContext.h, as I suggested.  It's typedef'ed to an int for non-Chromium builds.  (I needed something that could be stored as an OwnPtr.  I originally made it a GraphicsContext3D, but I wasn't certain that every port has that class compiled in).

This does allow a GraphicsContext3D implementation for other ports while allowing us to move forward with what we have. Let me know what you think.

[Stephen White]

Created attachment 63456 [details]
Patch

[Chris Marrin]

(In reply to comment #18)
>...
> > I think if you first focused on duplicating the WebGLRenderingContext compositing model in CanvasRenderingContext2D, I think it will be easier to make a GraphicsContext implementation on top of GLES2Context (whether directly or on top of GraphicsContext3D).
> 
> It would be nice to do so, but changing this ownership risks destabilizing the canvas implementation other ports, and (as you indicated) is orthogonal to the choice of rendering backend.  So I'd prefer to deal with those two issues separately, if possible.

Yes, I see your point. I like the approach of your latest patch. But I wonder if we could go one step further. Maybe we should be using GraphicsContext directly at all. Instead of an AcceleratedGraphicsContext, what if you made it a GraphicsContext2D (I know that naming is confusing, but you know what I mean). One implementation can use GraphicsContext like today and another can use your GLES2Context. Then you can put all the stuff in ifdefs down in that platform specific class. That would avoid destablilizing GraphicsContext but would keep ifdefs out of CanvasRenderingContext2D.

> 
> James is having some trouble getting the GraphicsContext3D implementation to work, so I'm uploading another patch for your consideration.  This one removes the use of #if USE(GLES2_RENDERING)s by using an  AcceleratedGraphicsContext typedef in GraphicsContext.h, as I suggested.  It's typedef'ed to an int for non-Chromium builds.  (I needed something that could be stored as an OwnPtr.  I originally made it a GraphicsContext3D, but I wasn't certain that every port has that class compiled in).
> 
> This does allow a GraphicsContext3D implementation for other ports while allowing us to move forward with what we have. Let me know what you think.

Yes, I think we are close. Let me know what you think about a new GraphicsContext2D class.

As far as naming goes, I think we could name it GraphicsRenderer2D, and later rename GraphicsContext3D to GraphicsRenderer3D, or something like that?

[Chris Marrin]

(In reply to comment #20)
> ...
> Yes, I see your point. I like the approach of your latest patch. But I wonder if we could go one step further. Maybe we should be using GraphicsContext directly at all. ...

I mean, maybe we should NOT be using GraphicsContext directly!

[Stephen White]

(In reply to comment #20)

> Yes, I see your point. I like the approach of your latest patch. But I wonder if we could go one step further. Maybe we should be using GraphicsContext directly at all. Instead of an AcceleratedGraphicsContext, what if you made it a GraphicsContext2D (I know that naming is confusing, but you know what I mean). One implementation can use GraphicsContext like today and another can use your GLES2Context. Then you can put all the stuff in ifdefs down in that platform specific class. That would avoid destablilizing GraphicsContext but would keep ifdefs out of CanvasRenderingContext2D.

I'm not sure I understand.  If you get a sec, could you come on #webkit and explain further?

One problem with changing the name specifically for 2D canvas is that longer term, I was hoping to reuse my switchable GraphicsContext elswhere in WebKit, where replacing it in many places with GraphicsContext2D would seem to be somewhat intrusive to the other ports.

> > This does allow a GraphicsContext3D implementation for other ports while allowing us to move forward with what we have. Let me know what you think.
> 
> Yes, I think we are close. Let me know what you think about a new GraphicsContext2D class.
> 
> As far as naming goes, I think we could name it GraphicsRenderer2D, and later rename GraphicsContext3D to GraphicsRenderer3D, or something like that?

Long-term, I was thinking that GraphicsContext would remain the interface for 2D operations, to be most compatible and familiar with the rest of WebKit, and GraphicsContext3D could remain the interface for accelerated renderering.  If it were renamed, perhaps *it* should be called AcceleratedGraphicsContext (to reflect its dimension-neutral status) and my new type named something else.

[James Robinson]

Created attachment 63552 [details]
Current canvas class hierarchy

You can view this diagram here: http://www.diagrammr.com/edit?key=dUgOmTbViyZ (note that editing this edits the live diagram, I dunno how to copy using this tool)

This is a diagram of the class hierarchy and draw paths that currently exist, so it's easier to tell what proposals are changing.  The draw path for 2d calls is:

JavaScript -> CanvasRenderingContext2D -> GraphicsContext (owned by the HTMLCanvasElement's ImageBuffer) -> CoreGraphics, Skia, Qt, what have you

The WebGL rendering path is pretty much completely orthogonal to the 2d path.

[James Robinson]

Created attachment 63554 [details]
Chris' proposal

This is (I believe) what Chris Marrin's proposal looks like.  Please correct me if I've got something wrong.  You can view (and edit) the diagram here: http://www.diagrammr.com/edit?key=dNxSHIjTGvL.

The idea here is that a new rendering API called (tentatively) GraphicsPainter.  The 2d and 3d canvas rendering contexts issue draw commands at the 2d and 3d versions of GraphicsPainter, respectively.  GraphicsPainter2D exposes the set of draw calls needed by CanvasRenderingContext2D and is capable of using either a software GraphicsContext or an accelerated API as needed.  The GraphicsPainter2D owns the ImageBuffer in this scenario, in contrast to the current situation where the ImageBuffer is owned by the HTMLCanvasElement and can get reset many times during the life of a CanvasRenderingContext2D.

One thing I left out of this diagram is exactly how GraphicsPainter2D makes calls to the platform's GPU, but this would happen either by using GraphicsPainter3D or some piece of common infrastructure.

The call path for 2d draw calls in this scenario would be:

JavaScript -> CanvasRenderingContext2D -> GraphicsPainter2D
at this point it can go either:
GraphicsPainter2D -> GraphicsContext -> CoreGraphics, Skia, Qt, etc
or
GraphicsPainter2D -> <some GPU abstraction layer> -> OpenGL

I'll talk about the pros and cons of this approach in my opinion in a moment.

[James Robinson]

Created attachment 63557 [details]
another proposal, switching at platform layer

Here's another possibility that switches at the platform context layer, just below GraphicsContext.  This is roughly how Stephen and I have been building out prototypes.

The key difference is that the decision about whether to accelerate or not is made at the platform context layer.  CanvasRenderingContext2D still issues draws calls to GraphicsContext:

JavaScript -> CanvasRenderingContext2D -> GraphicsContext -> platform context
and then either:
platform context -> GraphicsContext3D -> OpenGL flavor, etc
or:
platform context -> CoreGraphics, Skia, Qt, etc

The CanvasRenderingContext2D holds the GraphicsContext3D alive and passes it down to the platform context layer whenever the ImageBuffer is reset.

[James Robinson]

There's an issue with each of the previous two diagrams that I think is pretty basic.  If the switching is done above the GraphicsContext layer (as in Chris' proposal), then the work can't be reused by any other rendering paths that expect a GraphicsContext interface.  However, if the switching is done below the GraphicsContext layer (as in the latter proposal) then the work becomes inherently platform specific.

I personally feel that while we're working on bringing something up that works at all it's better to push the added complexity into a subset of platforms and then once we have some code that is getting more useful and stable expanding it out to all platforms.  This seems to be the pattern followed by similar features in the past, for example accelerated compositing.

In both cases, however, we end up with a bit of code that accepts a set of platform-agnostic 2d drawing commands (like fillRect) and outputs a series of calls to some cross-platform accelerated calls.  It'd be great if we could just use this piece of software for canvas 2d and have the option of using it everywhere.

[James Robinson]

Created attachment 63559 [details]
virtual GraphicsContext proposal

This one's perhaps a bit out there but it is one way that we could have a cross-platform interface usable everywhere.  If GraphicContext calls were virtual then we could write a GraphicsContextAccelerated class that could implement draw commands via GraphicsContext3D or by deferring to the GraphicsContextCG / GraphicsContextSkia / GraphicsContextQt software implementation.  This could be used anywhere that GraphicsContext is expected now.

This way is more intrusive than the other proposals but IMHO the cleanest overall.

[Chris Marrin]

(In reply to comment #26)
> There's an issue with each of the previous two diagrams that I think is pretty basic.  If the switching is done above the GraphicsContext layer (as in Chris' proposal), then the work can't be reused by any other rendering paths that expect a GraphicsContext interface.  However, if the switching is done below the GraphicsContext layer (as in the latter proposal) then the work becomes inherently platform specific.

All of your diagrams still have ImageBuffer owned by HTMLCanvasElement, which I what we have today. I think this is the biggest problem. ImageBuffer is never used by WebGLRenderingContext. It is an artifact of the software rendering orientation of the current system. 

> 
> I personally feel that while we're working on bringing something up that works at all it's better to push the added complexity into a subset of platforms and then once we have some code that is getting more useful and stable expanding it out to all platforms.  This seems to be the pattern followed by similar features in the past, for example accelerated compositing.

Right. That's my reason for the whole GraphicsPainter hierarchy. HTMLCanvasElement and CanvasRenderingContext2D would no longer use GraphicsContext or ImageBuffer. Those would be buried behind  the GraphicsPainter2D, which is a platform specific interface which has the same API as GraphicsContext. But it does the switching between HW and SW rendering and HW and SW compositing. Actually all those decisions are in the base class, GraphicsPainter.

> 
> In both cases, however, we end up with a bit of code that accepts a set of platform-agnostic 2d drawing commands (like fillRect) and outputs a series of calls to some cross-platform accelerated calls.  It'd be great if we could just use this piece of software for canvas 2d and have the option of using it everywhere.

Right. If we make the new classes, they can take the place of GraphicsContext in Canvas 2D and later in other places in the system.

[Chris Marrin]

(In reply to comment #24)
> Created an attachment (id=63554) [details]
> Chris' proposal
> 
> This is (I believe) what Chris Marrin's proposal looks like.  Please correct me if I've got something wrong.  You can view (and edit) the diagram here: http://www.diagrammr.com/edit?key=dNxSHIjTGvL.

This is close. In my proposal the PlatformLayer is owned by the base class GraphicsPainter since it is (optionally) needed for both 2D and 3D rendering.

[Chris Marrin]

(In reply to comment #29)
> (In reply to comment #24)
> > Created an attachment (id=63554) [details] [details]
> > Chris' proposal
> > 
> > This is (I believe) what Chris Marrin's proposal looks like.  Please correct me if I've got something wrong.  You can view (and edit) the diagram here: http://www.diagrammr.com/edit?key=dNxSHIjTGvL.
> 
> This is close. In my proposal the PlatformLayer is owned by the base class GraphicsPainter since it is (optionally) needed for both 2D and 3D rendering.

And when I say "owned by" I mean it is exposed to the outside (e.g., RenderLayerBacking) through GraphicsPainter. It may be (and probably would be) created by the specific subclasses.

[James Robinson]

(In reply to comment #28)
> (In reply to comment #26)
> > There's an issue with each of the previous two diagrams that I think is pretty basic.  If the switching is done above the GraphicsContext layer (as in Chris' proposal), then the work can't be reused by any other rendering paths that expect a GraphicsContext interface.  However, if the switching is done below the GraphicsContext layer (as in the latter proposal) then the work becomes inherently platform specific.
> 
> All of your diagrams still have ImageBuffer owned by HTMLCanvasElement, which I what we have today. I think this is the biggest problem. ImageBuffer is never used by WebGLRenderingContext. It is an artifact of the software rendering orientation of the current system. 

This isn't quite true - the canvas' ImageBuffer is used by all types of canvas (2d, WebGL, and canvas elements that don't yet have a rendering context) to support drawImage(HTMLCanvasElement, ...).  We could write special-case code for each type of canvas 

> 
> > 
> > I personally feel that while we're working on bringing something up that works at all it's better to push the added complexity into a subset of platforms and then once we have some code that is getting more useful and stable expanding it out to all platforms.  This seems to be the pattern followed by similar features in the past, for example accelerated compositing.
> 
> Right. That's my reason for the whole GraphicsPainter hierarchy. HTMLCanvasElement and CanvasRenderingContext2D would no longer use GraphicsContext or ImageBuffer. Those would be buried behind  the GraphicsPainter2D, which is a platform specific interface which has the same API as GraphicsContext. But it does the switching between HW and SW rendering and HW and SW compositing. Actually all those decisions are in the base class, GraphicsPainter.
> 
> > 
> > In both cases, however, we end up with a bit of code that accepts a set of platform-agnostic 2d drawing commands (like fillRect) and outputs a series of calls to some cross-platform accelerated calls.  It'd be great if we could just use this piece of software for canvas 2d and have the option of using it everywhere.
> 
> Right. If we make the new classes, they can take the place of GraphicsContext in Canvas 2D and later in other places in the system.

I'm not sure how to reuse this code easily if it doesn't actually implement the GraphicsContext interface (either directly or indirectly).

[Chris Marrin]

(In reply to comment #31)
> ...
> > All of your diagrams still have ImageBuffer owned by HTMLCanvasElement, which I what we have today. I think this is the biggest problem. ImageBuffer is never used by WebGLRenderingContext. It is an artifact of the software rendering orientation of the current system. 
> 
> This isn't quite true - the canvas' ImageBuffer is used by all types of canvas (2d, WebGL, and canvas elements that don't yet have a rendering context) to support drawImage(HTMLCanvasElement, ...).  We could write special-case code for each type of canvas 

And this is why trying to use a WebGL canvas as a source for drawImage doesn't work. The assumptions made by the system  that ImageBuffer always contains the correct bits is wrong. The better model would be to have the bits owned by the GraphicsPainter subclasses and made available as the result of a function call. 

> 
> > 
> > > 
> > > I personally feel that while we're working on bringing something up that works at all it's better to push the added complexity into a subset of platforms and then once we have some code that is getting more useful and stable expanding it out to all platforms.  This seems to be the pattern followed by similar features in the past, for example accelerated compositing.
> > 
> > Right. That's my reason for the whole GraphicsPainter hierarchy. HTMLCanvasElement and CanvasRenderingContext2D would no longer use GraphicsContext or ImageBuffer. Those would be buried behind  the GraphicsPainter2D, which is a platform specific interface which has the same API as GraphicsContext. But it does the switching between HW and SW rendering and HW and SW compositing. Actually all those decisions are in the base class, GraphicsPainter.
> > 
> > > 
> > > In both cases, however, we end up with a bit of code that accepts a set of platform-agnostic 2d drawing commands (like fillRect) and outputs a series of calls to some cross-platform accelerated calls.  It'd be great if we could just use this piece of software for canvas 2d and have the option of using it everywhere.
> > 
> > Right. If we make the new classes, they can take the place of GraphicsContext in Canvas 2D and later in other places in the system.
> 
> I'm not sure how to reuse this code easily if it doesn't actually implement the GraphicsContext interface (either directly or indirectly).

Right. I stated above that GraphicsPainter2D would have the same API (as far as drawing calls go) as GraphicsContext.

[James Robinson]

(In reply to comment #32)
> (In reply to comment #31)
> > ...
> > > All of your diagrams still have ImageBuffer owned by HTMLCanvasElement, which I what we have today. I think this is the biggest problem. ImageBuffer is never used by WebGLRenderingContext. It is an artifact of the software rendering orientation of the current system. 
> > 
> > This isn't quite true - the canvas' ImageBuffer is used by all types of canvas (2d, WebGL, and canvas elements that don't yet have a rendering context) to support drawImage(HTMLCanvasElement, ...).  We could write special-case code for each type of canvas 
> 
> And this is why trying to use a WebGL canvas as a source for drawImage doesn't work. The assumptions made by the system  that ImageBuffer always contains the correct bits is wrong. The better model would be to have the bits owned by the GraphicsPainter subclasses and made available as the result of a function call. 

Using a WebGL canvas as the source for drawImage() has worked properly for a few weeks now (since http://trac.webkit.org/changeset/63502).    The design of how this works could be changed but I think that's getting a little bit away from the current topic.

> > > > In both cases, however, we end up with a bit of code that accepts a set of platform-agnostic 2d drawing commands (like fillRect) and outputs a series of calls to some cross-platform accelerated calls.  It'd be great if we could just use this piece of software for canvas 2d and have the option of using it everywhere.
> > > 
> > > Right. If we make the new classes, they can take the place of GraphicsContext in Canvas 2D and later in other places in the system.
> > 
> > I'm not sure how to reuse this code easily if it doesn't actually implement the GraphicsContext interface (either directly or indirectly).
> 
> Right. I stated above that GraphicsPainter2D would have the same API (as far as drawing calls go) as GraphicsContext.

Sure, but we couldn't actually pass a GraphicsPainter2D object to code that expects a GraphicsContext.

[Stephen White]

IMHO, restructuring canvas2D, including the software path, is out-of-scope for this work, due in part to the risk of destabilizing other ports.

Since James has made great strides in re-implementing our stuff on top of GraphicsContext3D, which was the source of your original objection, could we consider that work on its own merits (patch to be uploaded soon, I believe), and look at the restructuring as a separate issue?

[Chris Marrin]

(In reply to comment #34)
> IMHO, restructuring canvas2D, including the software path, is out-of-scope for this work, due in part to the risk of destabilizing other ports.
> 
> Since James has made great strides in re-implementing our stuff on top of GraphicsContext3D, which was the source of your original objection, could we consider that work on its own merits (patch to be uploaded soon, I believe), and look at the restructuring as a separate issue?

Sure. I look forward to seeing the patch. But now that you've opened this can of worms (something you'll come to regret, I'm sure :-) I now feel compelled to do the restructuring to make 2D canvas work like 3D. I'll open a bug to this effect.

[James Robinson]

Created attachment 63692 [details]
Patch

[James Robinson]

New patch attached using GraphicsContext3D as the base rendering primitive.  It follows the approach described in comment #25.

It sounds like we agree we want to change around how canvas works, but aren't entirely sure of how.  I think this patch includes many good properties that we definitely agree on (common interactions with the compositor, no dependency on specific OpenGL, and no risk of destabilizing how software canvases work).  I'm excited about cleaning up the canvas software path as well but would also really like to land this patch so that folks can work on making the accelerated path fast in parallel to the software restructuring.

[Simon Fraser (smfr)]

Comment on attachment 63692 [details]
Patch

> diff --git a/WebCore/html/canvas/CanvasRenderingContext2D.cpp b/WebCore/html/canvas/CanvasRenderingContext2D.cpp

> @@ -100,6 +106,9 @@ CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement* canvas, bo
>  #if ENABLE(DASHBOARD_SUPPORT)
>      , m_usesDashboardCompatibilityMode(usesDashboardCompatibilityMode)
>  #endif
> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +    , m_context3D(0)
> +#endif
>  {
>  #if !ENABLE(DASHBOARD_SUPPORT)
>      ASSERT_UNUSED(usesDashboardCompatibilityMode, !usesDashboardCompatibilityMode);
> @@ -108,6 +117,21 @@ CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement* canvas, bo
>      // Make sure that even if the drawingContext() has a different default
>      // thickness, it is in sync with the canvas thickness.
>      setLineWidth(lineWidth());
> +
> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +    if (Page* p = canvas->document()->page()) {
> +        if (p->settings()->accelerated2dCanvasEnabled()) {
> +            GraphicsContext3D::Attributes attr;
> +            attr.stencil = true;
> +            HostWindow* hostWindow = canvas->document()->view()->root()->hostWindow();
> +            // Set up our context
> +            m_context3D = GraphicsContext3D::create(attr, hostWindow);
> +            if (m_context3D)
> +                if (GraphicsContext* c = drawingContext())
> +                    c->platformContext()->setGraphicsContext3D(m_context3D.get(), IntSize(canvas->width(), canvas->height()));
> +        }
> +    }
> +#endif

Wow, so you access page() and hostWindow() in this method. Are you sure that you don't need any null checking for canvas->document()->view()->root() ?
Does this work for CSS canvases?

It would be nice to find a cleaner way to do this, that doesn't require access to hostWindow.

r=me

[Chris Marrin]

Comment on attachment 63692 [details]
Patch

WebCore/html/canvas/CanvasRenderingContext2D.cpp:134
 +  #endif


Having a setGraphicsContext3D method on the platformContext seems wrong. PlatformGraphicsContext is a platform specific class which often isn't even a C++ class. I think it would be better to just add setGraphicsContext3D() to GraphicsContext itself and have the platform specific code do what it needs to do. This will also allow the default implementation of setGraphicsContext3D to have a compile-time failure so builders will get an indication that in addition to enabling ACCELERATED_2D_CANVAS you need to implement for it in GraphicsContext.



WebCore/html/canvas/CanvasRenderingContext2D.cpp:151
 +  #endif


Why aren't you doing the accelerated2dCanvasEnabled() test here like above?

[James Robinson]

(In reply to comment #38)
> (From update of attachment 63692 [details])
> > diff --git a/WebCore/html/canvas/CanvasRenderingContext2D.cpp b/WebCore/html/canvas/CanvasRenderingContext2D.cpp
> 
> 
> Wow, so you access page() and hostWindow() in this method. Are you sure that you don't need any null checking for canvas->document()->view()->root() ?

I'll check if I need null checks.  Unfortunately this is the way you create GraphicsContext3Ds today.

> Does this work for CSS canvases?

I don't think so, probably for the same reason as WebGL.

> 
> It would be nice to find a cleaner way to do this, that doesn't require access to hostWindow.

Agreed.

> 
> r=me

(In reply to comment #39)
> (From update of attachment 63692 [details])
> WebCore/html/canvas/CanvasRenderingContext2D.cpp:134
>  +  #endif
> 
> 
> Having a setGraphicsContext3D method on the platformContext seems wrong. PlatformGraphicsContext is a platform specific class which often isn't even a C++ class. I think it would be better to just add setGraphicsContext3D() to GraphicsContext itself and have the platform specific code do what it needs to do. This will also allow the default implementation of setGraphicsContext3D to have a compile-time failure so builders will get an indication that in addition to enabling ACCELERATED_2D_CANVAS you need to implement for it in GraphicsContext.

I can move it down - I agree it makes more sense on GraphicsContext.

> 
> 
> 
> WebCore/html/canvas/CanvasRenderingContext2D.cpp:151
>  +  #endif
> 
> 
> Why aren't you doing the accelerated2dCanvasEnabled() test here like above?

Do you mean in reset()?  The invariant I'm using is that m_context3D will always be 0 if accelerated 2d canvas is not enabled, so outside of the constructor I don't have to walk back up to the Page.

[James Robinson]

> 
> > Does this work for CSS canvases?
> 
> I don't think so, probably for the same reason as WebGL.
> 

Looking more closely I think this should work fine for CSS canvases.

[Chris Marrin]

> (In reply to comment #39)
> ...
> > Why aren't you doing the accelerated2dCanvasEnabled() test here like above?
> 
> Do you mean in reset()?  The invariant I'm using is that m_context3D will always be 0 if accelerated 2d canvas is not enabled, so outside of the constructor I don't have to walk back up to the Page.

Yes, of course, that makes sense. Never mind. And sorry about having insufficient context in my comments...

[James Robinson]

Created attachment 63776 [details]
Patch

[James Robinson]

I added extra NULL checks in the GraphicsContext3D initialization path.  Looking at WebGL it seems that they don't have these checks (http://trac.webkit.org/browser/trunk/WebCore/html/canvas/WebGLRenderingContext.cpp#L84) but that seems wrong.  I want to clean up the GraphicsContext3D initialization a bit (using HostWindow* seems odd) but that feels like another patch.

[James Robinson]

Created attachment 63778 [details]
add missing UNUSED_PARAM()

[Simon Fraser (smfr)]

Comment on attachment 63778 [details]
add missing UNUSED_PARAM()


> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +    if (Page* p = canvas->document()->page()) {
> +        if (p->settings()->accelerated2dCanvasEnabled()) {
> +            if (FrameView* view = canvas->document()->view()) {
> +                if (ScrollView* rootView = view->root()) {
> +                    if (HostWindow* hostWindow = view->root()->hostWindow()) {
> +                        // Set up our context
> +                        GraphicsContext3D::Attributes attr;
> +                        attr.stencil = true;
> +                        m_context3D = GraphicsContext3D::create(attr, hostWindow);
> +                        if (m_context3D)
> +                            if (GraphicsContext* c = drawingContext())
> +                                c->setGraphicsContext3D(m_context3D.get(), IntSize(canvas->width(), canvas->height()));
> +                    }
> +                }
> +            }
> +        }
> +    }

Some early returns would help with readability here.

[James Robinson]

Thanks for the review.  I added some early returns, I'll shorten this up in a follow-up (GraphicsContext3D::create() could take a Page* instead of a HostWindow*).

[James Robinson]

Committed r64881: <http://trac.webkit.org/changeset/64881>