Bug 40643 – Move ownership of Canvas3DLayer to GraphicsContext3D

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Chris Marrin]

This would make WebGL behave more like the media layer. It would not only clean up the code, but would make it possible to get rid of one GL context and a copy of the pixels. We may also want to rename Canvas3DLayer to WebGLLayer.

[Chris Marrin]

I will do this in 3 phases:

1) Rename GraphicsContext3DMac.cpp to GraphicsContext3DMac.mm to allow handling of CALayer

2) Rename Canvas3DLayer to WebGLLayer

3) Move WebGLLayer ownership to GraphicsContext3D and use a single CGLContext for rendering

[Chris Marrin]

Created attachment 58841 [details]
Patch to rename GraphicsContext3DMac.cpp

[Eric Seidel (no email)]

Attachment 58841 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3324197

[Simon Fraser (smfr)]

Comment on attachment 58841 [details]
Patch to rename GraphicsContext3DMac.cpp

r=me assuming that you also intend to fix the Xcode project.

[Chris Marrin]

Landed GraphicsContext3DMac rename in http://trac.webkit.org/changeset/61262 (along with xcodeproj change)

[Chris Marrin]

Created attachment 58901 [details]
Patch to rename Canvas3DLayer to WebGLLayer

[Simon Fraser (smfr)]

I'd suggesting using a more specific name than WebGLLayer, to avoid Obj-C name conflicts with embedding apps (Obj-C just has one global namespace). I have to rename WebLayer/WebTiledLayer for the same reasons.

[Eric Seidel (no email)]

Comment on attachment 58841 [details]
Patch to rename GraphicsContext3DMac.cpp

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

[Chris Marrin]

Landed WebGLLayer rename in http://trac.webkit.org/changeset/61263

[Chris Marrin]

Created attachment 59413 [details]
Final patch

[WebKit Review Bot]

Attachment 59413 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/mac/GraphicsLayerCA.h:336:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 59413 [details]
Final patch

> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.mm
> ===================================================================
> +#ifndef NDEBUG
> +        [m_webGLLayer.get() setName:@"3D Layer"];
> +#endif    

I think "WebGL Layer" would be a better name.

> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.h
> ===================================================================

>  #if ENABLE(3D_CANVAS)
> -    virtual void setGraphicsContext3DNeedsDisplay();
> +    virtual void setWebGLNeedsDisplay();
>  #endif

Do you really need the setWebGLNeedsDisplay() method? The GraphicsContext3D now holds onto the layer, so maybe it should do the -setNeedsDisplay itself?

However, this will bypass the syncing logic, so may cause flashes with some content.

> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> ===================================================================

> -void GraphicsLayerCA::setGraphicsContext3DNeedsDisplay()
> +void GraphicsLayerCA::setWebGLNeedsDisplay()
>  {
> -    if (m_contentsLayerPurpose == ContentsLayerForGraphicsLayer3D)
> +    if (m_contentsLayerPurpose == ContentsLayerForWebGL)
>          [m_contentsLayer.get() setNeedsDisplay];
>  }

This may cause an early CA commit, which can result in flashes in some content. You should really use a dirty bit for this.

r=me but please reconsider setWebGLNeedsDisplay().

[Chris Marrin]

Created attachment 59418 [details]
replacement patch with setContentsNeedsDisplay

[WebKit Review Bot]

Attachment 59418 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/mac/GraphicsLayerCA.h:335:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 59418 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3280610

[Chris Marrin]

Created attachment 59419 [details]
Replacement patch fixing style issue and Qt build failure

[Chris Marrin]

Landed final patch in http://trac.webkit.org/changeset/61631

[Vangelis Kokkevis]

The changes to RenderLayerBacking.cpp break the Chromium build (and most likely the Qt build as well).

[line: 219] if (context->graphicsContext3D()->platformLayer())

platformLayer() is only defined for PLATFORM(MAC)

[Chris Marrin]

I suppose the bots didn't catch this because they build with 3D_CANVAS off? Are either Qt or Chromium building with both 3D_CANVAS and ACCELERATED_COMPOSITING enabled? I didn't think so. Vangelis, since you have a Chromium build environment, could you make a #else case that defines platformLayer() for other than Mac. I suppose it would just be a stub for now.

[Vangelis Kokkevis]

chromium on windows currently builds with both ACCELERATED_COMPOSITING and 3D_CANVAS but it look like the windows trybot didn't get to run. 

I'll check in a quick fix to get the chrome build green again, however, there's an issue with the new API. The idea that GraphicsContext3D knows about the PlatformLayer it renders into is, I believe, unique to CoreAnimation. In other ports, the PlatformLayer's actually get created by the GraphicsLayer's and are managed by them as well. I found the old API where you provide a pointer to the GraphicsContext3D when you set the contents of a layer more general. Can't you extract the CALayer information from it within GraphicsLayerCA::setContentsToWebGL ?

[Chris Marrin]

(In reply to comment #20)
> chromium on windows currently builds with both ACCELERATED_COMPOSITING and 3D_CANVAS but it look like the windows trybot didn't get to run. 
> 
> I'll check in a quick fix to get the chrome build green again, however, there's an issue with the new API. The idea that GraphicsContext3D knows about the PlatformLayer it renders into is, I believe, unique to CoreAnimation. In other ports, the PlatformLayer's actually get created by the GraphicsLayer's and are managed by them as well. I found the old API where you provide a pointer to the GraphicsContext3D when you set the contents of a layer more general. Can't you extract the CALayer information from it within GraphicsLayerCA::setContentsToWebGL ?

One of the biggest motivators for this change was to make WebGL layers behave like the media layer, which hands ownership of the layer to the client. For video this allows acceleration of the video contents without GraphicsLayer having to know anything about it. The same is true of WebGL layers. There was hackery involved to communicate enough information to GraphicsLayer to get it to be able to render accelerated 3D content. 

With the current design GraphicsLayer doesn't have to know about GraphicsContext3D or WebGLLayer. So I think this is the better design. I think your design would really benefit from contracting out the creation and management of the PlatformLayer. Remember PlatformLayer could be anything you want in Chromium, a GL context, a GraphicsContext3D, or a wrapper with the appropriate functionality. It really does reduce dependencies.

[Simon Fraser (smfr)]

*** Bug 34036 has been marked as a duplicate of this bug. ***