Bug 40643 - Move ownership of Canvas3DLayer to GraphicsContext3D
Summary: Move ownership of Canvas3DLayer to GraphicsContext3D
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.6
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
: 34036 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-06-15 16:20 PDT by Chris Marrin
Modified: 2010-06-23 11:27 PDT (History)
7 users (show)

See Also:


Attachments
Patch to rename GraphicsContext3DMac.cpp (93.50 KB, patch)
2010-06-15 17:38 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch to rename Canvas3DLayer to WebGLLayer (28.54 KB, patch)
2010-06-16 10:26 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Final patch (16.52 KB, patch)
2010-06-22 13:52 PDT, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff
replacement patch with setContentsNeedsDisplay (18.44 KB, patch)
2010-06-22 14:55 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Replacement patch fixing style issue and Qt build failure (18.45 KB, patch)
2010-06-22 15:06 PDT, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2010-06-15 16:20:18 PDT
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.
Comment 1 Chris Marrin 2010-06-15 17:33:56 PDT
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
Comment 2 Chris Marrin 2010-06-15 17:38:37 PDT
Created attachment 58841 [details]
Patch to rename GraphicsContext3DMac.cpp
Comment 3 Eric Seidel (no email) 2010-06-15 17:43:28 PDT
Attachment 58841 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3324197
Comment 4 Simon Fraser (smfr) 2010-06-15 18:21:48 PDT
Comment on attachment 58841 [details]
Patch to rename GraphicsContext3DMac.cpp

r=me assuming that you also intend to fix the Xcode project.
Comment 5 Chris Marrin 2010-06-16 10:08:06 PDT
Landed GraphicsContext3DMac rename in http://trac.webkit.org/changeset/61262 (along with xcodeproj change)
Comment 6 Chris Marrin 2010-06-16 10:26:52 PDT
Created attachment 58901 [details]
Patch to rename Canvas3DLayer to WebGLLayer
Comment 7 Simon Fraser (smfr) 2010-06-16 11:07:49 PDT
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.
Comment 8 Eric Seidel (no email) 2010-06-17 15:30:36 PDT
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.
Comment 9 Chris Marrin 2010-06-22 13:50:53 PDT
Landed WebGLLayer rename in http://trac.webkit.org/changeset/61263
Comment 10 Chris Marrin 2010-06-22 13:52:23 PDT
Created attachment 59413 [details]
Final patch
Comment 11 WebKit Review Bot 2010-06-22 13:56:40 PDT
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.
Comment 12 Simon Fraser (smfr) 2010-06-22 14:05:25 PDT
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().
Comment 13 Chris Marrin 2010-06-22 14:55:31 PDT
Created attachment 59418 [details]
replacement patch with setContentsNeedsDisplay
Comment 14 WebKit Review Bot 2010-06-22 14:58:09 PDT
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.
Comment 15 Early Warning System Bot 2010-06-22 15:01:47 PDT
Attachment 59418 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3280610
Comment 16 Chris Marrin 2010-06-22 15:06:58 PDT
Created attachment 59419 [details]
Replacement patch fixing style issue and Qt build failure
Comment 17 Chris Marrin 2010-06-22 15:25:23 PDT
Landed final patch in http://trac.webkit.org/changeset/61631
Comment 18 Vangelis Kokkevis 2010-06-22 17:01:59 PDT
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)
Comment 19 Chris Marrin 2010-06-22 17:11:32 PDT
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.
Comment 20 Vangelis Kokkevis 2010-06-22 17:33:03 PDT
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 ?
Comment 21 Chris Marrin 2010-06-23 11:25:56 PDT
(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.
Comment 22 Simon Fraser (smfr) 2010-06-23 11:27:27 PDT
*** Bug 34036 has been marked as a duplicate of this bug. ***