Bug 46007 – Make 2D accelerated canvas rendering build on Mac

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Marrin]

In order to enable 2D accelerated canvas rendering for Mac we need to add files from the Chromium implementation, add the infrastructure for enabling the compile time flag, add platform specific files for Mac and fix some build issues.

[Chris Marrin]

Created attachment 67967 [details]
Patch

[James Robinson]

Comment on attachment 67967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67967&action=prettypatch

> WebCore/platform/graphics/gpu/DrawingBuffer.h:72
> +#if PLATFORM(CHROMIUM)
>      class WillPublishCallback : public Noncopyable {

This callback needed for correctness in the current implementation.  We can chat about how to make this work properly with the CoreAnimation compositor but I don't think you want to just #ifdef it out.

> WebCore/platform/graphics/gpu/DrawingBuffer.h:96
> +#if PLATFORM(MAC)
> +    RetainPtr<WebGLLayer> m_platformLayer;
> +#endif

The point of having DrawingBufferInternal is that it can include any platform-specific objects.  I'd rather you follow that pattern and put all of the mac-specific objects in the DrawingBufferInternal implementation used for mac rather than polluting this file with a bunch of #ifdef gunk.

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:29
> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +

I'm not sure that ENABLE(ACCELERATED_2D_CANVAS) is the correct guard for this and other files in platform/graphics/gpu/ - those are guards about a feature, but these files are not necessarily specific to that feature.  Once we add (for instance) ENABLE(ACCELERATED_SVG) or something then it'll get very awkward to update all of these guards.

> WebCore/platform/graphics/gpu/mac/DrawingBufferMac.mm:67
> +    m_context->texImage2D(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, m_size.width(), m_size.height(), 0, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, 0);

I dunno how the mac compositor works, but this is probably not doing what you want.  In DrawingBufferChromium we have a texture that we're using as the color attachment to the FBO and use texImage2D to resize this backing texture.

[WebKit Review Bot]

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

[Chris Marrin]

(In reply to comment #2)
> (From update of attachment 67967 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67967&action=prettypatch
> 
> > WebCore/platform/graphics/gpu/DrawingBuffer.h:72
> > +#if PLATFORM(CHROMIUM)
> >      class WillPublishCallback : public Noncopyable {
> 
> This callback needed for correctness in the current implementation.  We can chat about how to make this work properly with the CoreAnimation compositor but I don't think you want to just #ifdef it out.

I prefer the terminology used by GraphicsLayer. We have a GraphicsLayerClient which is implemented by whoever wants to handle the callback. But it does more than just callbacks. It can provide info back to the GraphicsLayer, etc. So I'd like to switch to that terminology. For now I will just remove the ifdef and stub out our use of it.

> 
> > WebCore/platform/graphics/gpu/DrawingBuffer.h:96
> > +#if PLATFORM(MAC)
> > +    RetainPtr<WebGLLayer> m_platformLayer;
> > +#endif
> 
> The point of having DrawingBufferInternal is that it can include any platform-specific objects.  I'd rather you follow that pattern and put all of the mac-specific objects in the DrawingBufferInternal implementation used for mac rather than polluting this file with a bunch of #ifdef gunk.

I agree with this. I will switch to this technique.

> 
> > WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:29
> > +#if ENABLE(ACCELERATED_2D_CANVAS)
> > +
> 
> I'm not sure that ENABLE(ACCELERATED_2D_CANVAS) is the correct guard for this and other files in platform/graphics/gpu/ - those are guards about a feature, but these files are not necessarily specific to that feature.  Once we add (for instance) ENABLE(ACCELERATED_SVG) or something then it'll get very awkward to update all of these guards.

Ok. The point of the guards is to avoid having unused code in the binary. Do you think we need a new flag? 
> 
> > WebCore/platform/graphics/gpu/mac/DrawingBufferMac.mm:67
> > +    m_context->texImage2D(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, m_size.width(), m_size.height(), 0, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, 0);
> 
> I dunno how the mac compositor works, but this is probably not doing what you want.  In DrawingBufferChromium we have a texture that we're using as the color attachment to the FBO and use texImage2D to resize this backing texture.

Yes, I know that. That will be how we do it on Mac, but not necessarily on other platforms.

[Chris Marrin]

James, can you help me understand the build failure in cr-linux? It's probably just that some part of your build is not turning on the ACCELERATED_2D_CANVAS flag that I've added to all the support files. But I don't know your build well enough to understand where it would go.

Also, I realize that we will change the ifdefs in all those files. But they ultimately need some kind of ENABLE flag, so we need to fix this in either case.

[Chris Marrin]

Comment on attachment 67967 [details]
Patch

I will have another patch soon

[Chris Marrin]

Comment on attachment 67967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67967&action=review

> WebCore/ChangeLog:19
> +        enabled, and add a skeleton of the mac specific file so it builds
> +
> +        DrawingBufferMac.mm, the Mac specific file does nothing right now
> +        other than initialize and create a layer. The rest will be added
> +        when I do the actual implementation. 
> +
> +        The most significant change was to SharedGraphicsContext3D, which
> +        had a static local OwnPtr in one of the methods. This was causing
> +        the exit time destructor warning. Getting rid of the OwnPtr fixed
> +        the problem. Since the pointer that was being wrapped was to a

foo

[Stephen White]

(In reply to comment #4)
> (In reply to comment #2)
> > > WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:29
> > > +#if ENABLE(ACCELERATED_2D_CANVAS)
> > > +
> > 
> > I'm not sure that ENABLE(ACCELERATED_2D_CANVAS) is the correct guard for this and other files in platform/graphics/gpu/ - those are guards about a feature, but these files are not necessarily specific to that feature.  Once we add (for instance) ENABLE(ACCELERATED_SVG) or something then it'll get very awkward to update all of these guards.
> 
> Ok. The point of the guards is to avoid having unused code in the binary. Do you think we need a new flag? 

I second James' comment.  I think we need a new flag.  We used to have USE(GLES2_RENDERING) for this, until we turned it on for all Chromium builds by default.  Some ideas:  GPU_RENDERING?  (Kind of generic).  GPU_2D_RENDERING?  ACCELERATED_2D_RENDERING?  ACCELERATED_2D?  The last two are in keeping with the existing ACCELERATED* flags, so I kind of like those.  Probably should be an ENABLE rather than a USE, since it's not strictly a third-party library.

[James Robinson]

Looks like this fails on cr-linux due to a misconfiguration on some of our bots, I'll address that separately.

I think it's valid to leave it ENABLE(ACCELERATED_2D_CANVAS) until we get a second user of this code, and then to pick the most appropriate guard.  I'm never 100% sure what the correct use of ENABLE() vs USE() is for these things.  FWIW in chromium we rely more heavily on runtime flags rather than compile time settings for things like this, so we typically want to compile support code in even for features that aren't on by default.  The linker can remove code that is actually unreachable (at some cost to compile+link time and memory use).

[Chris Marrin]

(In reply to comment #9)
> Looks like this fails on cr-linux due to a misconfiguration on some of our bots, I'll address that separately.
> 
> I think it's valid to leave it ENABLE(ACCELERATED_2D_CANVAS) until we get a second user of this code, and then to pick the most appropriate guard.  I'm never 100% sure what the correct use of ENABLE() vs USE() is for these things.  FWIW in chromium we rely more heavily on runtime flags rather than compile time settings for things like this, so we typically want to compile support code in even for features that aren't on by default.  The linker can remove code that is actually unreachable (at some cost to compile+link time and memory use).

ENABLE() is for a feature that can be turned on and off, a user facing feature. build-webkit has command line flags that can turn these on and off. USE() is for something that some platforms use and others don't. So ACCELERATED_COMPOSITING is a USE and 3D_CANVAS (which is the poorly named flag for WebGL) is an ENABLE. Which brings up the point, ACCELERATED_2D_CANVAS should really be a USE.

But I agree with James. We should leave the flag name as ACCELERATED_2D_CANVAS and deal with generalizing it when needed. Maybe the right thing to do is to add a bug to change the flag to a USE and to change it's name to make it more generic, and fix both later on. We really could just call it ACCELERATED_2D. Is there every a reason to just want to accelerate canvas and not svg or html (or some other combination)?

[James Robinson]

(In reply to comment #10)
> (In reply to comment #9)
> > Looks like this fails on cr-linux due to a misconfiguration on some of our bots, I'll address that separately.
> > 
> > I think it's valid to leave it ENABLE(ACCELERATED_2D_CANVAS) until we get a second user of this code, and then to pick the most appropriate guard.  I'm never 100% sure what the correct use of ENABLE() vs USE() is for these things.  FWIW in chromium we rely more heavily on runtime flags rather than compile time settings for things like this, so we typically want to compile support code in even for features that aren't on by default.  The linker can remove code that is actually unreachable (at some cost to compile+link time and memory use).
> 
> ENABLE() is for a feature that can be turned on and off, a user facing feature. build-webkit has command line flags that can turn these on and off. USE() is for something that some platforms use and others don't. So ACCELERATED_COMPOSITING is a USE and 3D_CANVAS (which is the poorly named flag for WebGL) is an ENABLE. Which brings up the point, ACCELERATED_2D_CANVAS should really be a USE.

I think of ACCELERATED_2D_CANVAS as a feature and the files in platform/graphics/gpu/ as infrastructure used to support that feature.  The code in html/canvas/ and rendering/ that deal with how the Canvas element renders all have to do exclusively with the feature (ACCELERATED_2D_CANVAS) whereas stuff in platform/graphics/gpu/ is not explicitly aware of HTML concepts like canvas.  By that logic, ACCELERATED_2D_CANVAS should stay an ENABLE() and be used by code that is talking about the HTML canvas element and code in platform/graphics/gpu should probably be a USE() - since in theory anyway somebody might want to implement the feature of accelerated 2d canvas using a different approach, for example with a Direct 2D backend or by interfacing with some other platform-specific API directly.

> 
> But I agree with James. We should leave the flag name as ACCELERATED_2D_CANVAS and deal with generalizing it when needed. Maybe the right thing to do is to add a bug to change the flag to a USE and to change it's name to make it more generic, and fix both later on. We really could just call it ACCELERATED_2D. Is there every a reason to just want to accelerate canvas and not svg or html (or some other combination)?

Canvas requires a different set functionality compared to what you would need to correctly render arbitrary SVG or HTML (in general a subset of it), so it makes sense to turn the feature on just for canvas while implementing the necessary functionality for SVG/HTML/etc.  In fact, that's kind of what we are doing already.

[Chris Marrin]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Looks like this fails on cr-linux due to a misconfiguration on some of our bots, I'll address that separately.
> > > 
> > > I think it's valid to leave it ENABLE(ACCELERATED_2D_CANVAS) until we get a second user of this code, and then to pick the most appropriate guard.  I'm never 100% sure what the correct use of ENABLE() vs USE() is for these things.  FWIW in chromium we rely more heavily on runtime flags rather than compile time settings for things like this, so we typically want to compile support code in even for features that aren't on by default.  The linker can remove code that is actually unreachable (at some cost to compile+link time and memory use).
> > 
> > ENABLE() is for a feature that can be turned on and off, a user facing feature. build-webkit has command line flags that can turn these on and off. USE() is for something that some platforms use and others don't. So ACCELERATED_COMPOSITING is a USE and 3D_CANVAS (which is the poorly named flag for WebGL) is an ENABLE. Which brings up the point, ACCELERATED_2D_CANVAS should really be a USE.
> 
> I think of ACCELERATED_2D_CANVAS as a feature and the files in platform/graphics/gpu/ as infrastructure used to support that feature.  The code in html/canvas/ and rendering/ that deal with how the Canvas element renders all have to do exclusively with the feature (ACCELERATED_2D_CANVAS) whereas stuff in platform/graphics/gpu/ is not explicitly aware of HTML concepts like canvas.  By that logic, ACCELERATED_2D_CANVAS should stay an ENABLE() and be used by code that is talking about the HTML canvas element and code in platform/graphics/gpu should probably be a USE() - since in theory anyway somebody might want to implement the feature of accelerated 2d canvas using a different approach, for example with a Direct 2D backend or by interfacing with some other platform-specific API directly.

But still ACCELERATED_2D_CANVAS is not a user facing feature. It's just an implementation detail. Run build-webkit --help to see all the enable toggles. They are all things that have some API associated with them. ACCELERATED_2D_CANVAS has no API that is specific to it. Turning it on or off won't make WebKit have any different functionality. It will just (hopefully) make some things faster. In fact, if turning it on causes any difference in behavior at all, it's a bug. So I really think it ultimately needs to be a USE flag. But as I said, I don't think we need to deal with that now.

It may be true that the things down in platform/gpu should be under a different flag, but that should be a USE flag as well. All the USE flags go in WTF in Platform.h. There you can do things like saying "if I turn on ACCELERATED_2D_CANVAS I also have to turn on ACCELERATED_2D" (or whatever we call the flag to enable the support logic). So maybe we ultimately want ACCELERATED_2D for the support, and then ACCELERATED_2D_{CANVAS,SVG,HTML} for each renderer. But they should all be USE flags.

[Stephen White]

(In reply to comment #10)
> (In reply to comment #9)
> > Looks like this fails on cr-linux due to a misconfiguration on some of our bots, I'll address that separately.
> > 
> > I think it's valid to leave it ENABLE(ACCELERATED_2D_CANVAS) until we get a second user of this code, and then to pick the most appropriate guard.  I'm never 100% sure what the correct use of ENABLE() vs USE() is for these things.  FWIW in chromium we rely more heavily on runtime flags rather than compile time settings for things like this, so we typically want to compile support code in even for features that aren't on by default.  The linker can remove code that is actually unreachable (at some cost to compile+link time and memory use).
> 
> ENABLE() is for a feature that can be turned on and off, a user facing feature. build-webkit has command line flags that can turn these on and off. USE() is for something that some platforms use and others don't. So ACCELERATED_COMPOSITING is a USE and 3D_CANVAS (which is the poorly named flag for WebGL) is an ENABLE. Which brings up the point, ACCELERATED_2D_CANVAS should really be a USE.
> 
> But I agree with James. We should leave the flag name as ACCELERATED_2D_CANVAS and deal with generalizing it when needed. Maybe the right thing to do is to add a bug to change the flag to a USE and to change it's name to make it more generic, and fix both later on. We really could just call it ACCELERATED_2D. Is there every a reason to just want to accelerate canvas and not svg or html (or some other combination)?

According to this page:  http://trac.webkit.org/wiki/Porting%20Macros%20plan
USE() means "use a particular third-party library or optional OS service".

So originally, I had USE(GLES2_RENDERING) since this was supposed to indicate the use of the OpenGLES2 library (for 2D rendering).  Now that it's built on top of GraphicsContext3D, and is not (directly) dependent on a third-party library or OS service, I think it should be an ENABLE.

And actually, I have always thought USE(ACCELERATED_COMPOSITING) is wrong, since it doesn't refer to a particular library or OS service but a feature that may be enabled or disabled, and whose particular implementations can (and do) use OpenGL or OpenGLES2 or marshall the calls out to a separate process.  And correct me if I'm wrong, but that one really is user-facing, in that turning it off disables CSS 3D transforms.  You're right that the canvas 2D case is less clear -- disabling it shouldn't affect anything except performance.

Anyway, all that said, I'm fine with leaving everything under the ENABLE(ACCELERATED_2D_CANVAS) umbrella for now.

[Chris Marrin]

Created attachment 68486 [details]
Replacement patch, gets rid of ifdefs in DrawingBuffer

[James Robinson]

Cool!  Looks good, gonna let the bots cycle before touching review flag however.  I think this might fail again on cr-linux due to a configuration error that https://bugs.webkit.org/show_bug.cgi?id=46329 fixes.

[Chris Marrin]

(In reply to comment #15)
> Cool!  Looks good, gonna let the bots cycle before touching review flag however.  I think this might fail again on cr-linux due to a configuration error that https://bugs.webkit.org/show_bug.cgi?id=46329 fixes.

Actually, this has one issue that will make it fail on chrome. I change the struct DrawingBufferInternal to class. Struct is just a class with an implied 'public:' at the beginning. So it's legal, but I don't like applying 'new' to structs. It seems much cleaner to use call it a class and just declare its members public. In fact you could do anything inside the implementation. I just think in the interface it should be a class.

[Chris Marrin]

Created attachment 68628 [details]
Patch to fix merge conflict

[Kenneth Russell]

(In reply to comment #16)
> (In reply to comment #15)
> > Cool!  Looks good, gonna let the bots cycle before touching review flag however.  I think this might fail again on cr-linux due to a configuration error that https://bugs.webkit.org/show_bug.cgi?id=46329 fixes.
> 
> Actually, this has one issue that will make it fail on chrome. I change the struct DrawingBufferInternal to class. Struct is just a class with an implied 'public:' at the beginning. So it's legal, but I don't like applying 'new' to structs. It seems much cleaner to use call it a class and just declare its members public. In fact you could do anything inside the implementation. I just think in the interface it should be a class.

I've learned that WebKit style is that if all of the members are intended to be public, the entity should be a struct, not a class.

[Chris Marrin]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Cool!  Looks good, gonna let the bots cycle before touching review flag however.  I think this might fail again on cr-linux due to a configuration error that https://bugs.webkit.org/show_bug.cgi?id=46329 fixes.
> > 
> > Actually, this has one issue that will make it fail on chrome. I change the struct DrawingBufferInternal to class. Struct is just a class with an implied 'public:' at the beginning. So it's legal, but I don't like applying 'new' to structs. It seems much cleaner to use call it a class and just declare its members public. In fact you could do anything inside the implementation. I just think in the interface it should be a class.
> 
> I've learned that WebKit style is that if all of the members are intended to be public, the entity should be a struct, not a class.

Yes, I  think that would be true if this were simply a data structure used by the implementation of a class, but that's not what this is. It's part of a customizable interface. Making it a class allows the implementation the flexibility of structuring it any way it likes. I might want to make DrawingBufferInternal have public and private members just to make it easier to understand. So I think class is the right structure to use here.

[Simon Fraser (smfr)]

Comment on attachment 68628 [details]
Patch to fix merge conflict

View in context: https://bugs.webkit.org/attachment.cgi?id=68628&action=review

> WebCore/Configurations/FeatureDefines.xcconfig:40
> +ENABLE_ACCELERATED_2D_CANVAS = $(ENABLE_ACCELERATED_2D_CANVAS_$(REAL_PLATFORM_NAME));
> +ENABLE_ACCELERATED_2D_CANVAS_macosx = $(ENABLE_ACCELERATED_2D_CANVAS_macosx_$(TARGET_MAC_OS_X_VERSION_MAJOR));
> +ENABLE_ACCELERATED_2D_CANVAS_macosx_1060 = ;
> +ENABLE_ACCELERATED_2D_CANVAS_macosx_1070 = ;
> +

You need to add this to the FeatureDefines.xcconfig files for JSCore and WebKit too.

> WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:221
> +    static HashSet<SharedGraphicsContext3D*>* allContextsSet = new HashSet<SharedGraphicsContext3D*>;
> +    return allContextsSet;

This should use DEFINE_STATIC_LOCAL

> WebCore/platform/graphics/gpu/mac/DrawingBufferMac.mm:46
> +class DrawingBufferInternal {
> +public:
> +    DrawingBufferInternal() { }
> +    ~DrawingBufferInternal() { }
> +    
> +    RetainPtr<PlatformLayer> m_platformLayer;
> +};

Normally we avoid this kind of thing by just putting #ifdefs in the header file, which is much simpler (as long as there aren't too many of them).

[Chris Marrin]

Created attachment 69277 [details]
Updated patch, with Simon's issues addressed

Even though Simon gave this an r+ I feel the need to submit another patch which addresses his issues and goes back to the #ifdefs in DrawingBuffer.h. He's right that there's no precedent for a generic "Internal" interface. All other interfaces like this use platform specific ifdefs.

[Chris Marrin]

Created attachment 69280 [details]
Update patch, 2 more small changes

I failed to deal with the setWillPublishCallback() function correctly (in the Chromium case) in the last patch. I also missed one bit in the 2 newly changed FeatureDefines files.

[Kenneth Russell]

(In reply to comment #21)
> Created an attachment (id=69277) [details]
> Updated patch, with Simon's issues addressed
> 
> Even though Simon gave this an r+ I feel the need to submit another patch which addresses his issues and goes back to the #ifdefs in DrawingBuffer.h. He's right that there's no precedent for a generic "Internal" interface. All other interfaces like this use platform specific ifdefs.

On the contrary, there are places both in shared code and the Chromium port where members are encapsulated in an opaque "Internal" class. Examples include WebCore/platform/network/ResourceHandle{Internal}.h and WebCore/platform/graphics/GraphicsContext3D.h . It is strongly preferable to use this pattern rather than platform-specific #ifdefs in order to reduce the number of files that need to be recompiled when a new data member is added.

[Chris Marrin]

(In reply to comment #23)
> (In reply to comment #21)
> > Created an attachment (id=69277) [details] [details]
> > Updated patch, with Simon's issues addressed
> > 
> > Even though Simon gave this an r+ I feel the need to submit another patch which addresses his issues and goes back to the #ifdefs in DrawingBuffer.h. He's right that there's no precedent for a generic "Internal" interface. All other interfaces like this use platform specific ifdefs.
> 
> On the contrary, there are places both in shared code and the Chromium port where members are encapsulated in an opaque "Internal" class. Examples include WebCore/platform/network/ResourceHandle{Internal}.h and WebCore/platform/graphics/GraphicsContext3D.h . It is strongly preferable to use this pattern rather than platform-specific #ifdefs in order to reduce the number of files that need to be recompiled when a new data member is added.

Those two examples you give are exactly what we're now doing in DrawingBuffer. Some implementations choose to use a xxxInternal class, so it's enclosed in #ifdefs. The objection is doing a xxxInternal class on all platforms.

[Eric Seidel (no email)]

Comment on attachment 68628 [details]
Patch to fix merge conflict

Cleared Simon Fraser's review+ from obsolete attachment 68628 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/68802