Bug 45912 – [chromium] Change compositor to use GraphicsContext3D rather than GLES2Context

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kenneth Russell]

In order to improve the testability of Chromium's accelerated compositing code, as well as to re-enable multi-DLL builds on Windows, we need to change the compositor (LayerRendererChromium and associated code) to build on top of GraphicsContext3D instead of GLES2Context. Once this effort is complete, the GLES2Context and WebGLES2Context will be removed from the WebKit tree. This master bug will track the multiple required checkins.

[Kenneth Russell]

Created attachment 68127 [details]
Patch

From the ChangeLog:

Switched Chromium's compositor to use GraphicsContext3D to issue its OpenGL rendering calls rather than the Chromium-specific GLES2Context and command buffer OpenGL implementation.

The in-process software rendering path for GraphicsContext3D does not yet work with the compositor, at least not on Mac OS X. This will be worked on in subsequent bugs.

Tested manually with 3D CSS, WebGL and video content on Mac OS X and Linux. No new tests.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:2447:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 20 files


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

[James Robinson]

Comment on attachment 68127 [details]
Patch

Looking pretty good overall, some nits.  The style bot's nit is accurate.

Not touching review? so the rest of the bots get a chance to chug on it.

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

> WebCore/platform/graphics/chromium/CanvasLayerChromium.h:50
> +        SharedValues(GraphicsContext3D*);

explicit

> WebCore/platform/graphics/chromium/ContentLayerChromium.h:58
> +        SharedValues(GraphicsContext3D*);

explicit

> WebCore/platform/graphics/chromium/LayerChromium.h:174
> +        SharedValues(GraphicsContext3D*);

explicit

> WebCore/platform/graphics/chromium/LayerRendererChromium.h:106
> +    void noticeResize(const IntSize&);

Name is odd, it's not really telling the LayerRendererChromium to do something. I liked resizeOnscreenContent().

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:178
> +        void* mem = context->mapTexSubImage2DCHROMIUM(GraphicsContext3D::TEXTURE_2D, 0, updateRect.x(), updateRect.y(), updateRect.width(), updateRect.height(), GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, GraphicsContext3D::WRITE_ONLY);

I see that you aren't changing behavior here, but could you file a bug on the video guys to check for a NULL return here and handle it?  The mapTexSub..() extension may return null if it can't get a suitable shared memory segment.

[Stephen White]

Comment on attachment 68127 [details]
Patch

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

> WebKit/chromium/src/GraphicsContext3D.cpp:807
> +    if (!internal->initialize(attrs, hostWindow, renderStyle == RenderDirectlyToHostWindow ? true : false)) {

Nit:  you can omit the ternary.  A relational expression in C++ is type bool already.

> WebKit/chromium/src/GraphicsContext3D.cpp:810
> +    PassOwnPtr<GraphicsContext3D> result = new GraphicsContext3D(attrs, hostWindow, renderStyle == RenderDirectlyToHostWindow ? true : false);

Same as above.

[Kenneth Russell]

Created attachment 68142 [details]
Revised patch

Addressed above code review feedback.

[Kenneth Russell]

(In reply to comment #3)
> (From update of attachment 68127 [details])
> Looking pretty good overall, some nits.  The style bot's nit is accurate.

Revised patch fixes style error.

> Not touching review? so the rest of the bots get a chance to chug on it.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=68127&action=prettypatch
> 
> > WebCore/platform/graphics/chromium/CanvasLayerChromium.h:50
> > +        SharedValues(GraphicsContext3D*);
> 
> explicit

Done.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.h:58
> > +        SharedValues(GraphicsContext3D*);
> 
> explicit

Done.

> > WebCore/platform/graphics/chromium/LayerChromium.h:174
> > +        SharedValues(GraphicsContext3D*);
> 
> explicit
> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.h:106
> > +    void noticeResize(const IntSize&);
> 
> Name is odd, it's not really telling the LayerRendererChromium to do something. I liked resizeOnscreenContent().

That's fine; changed it to resizeOnscreenContent.

> > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:178
> > +        void* mem = context->mapTexSubImage2DCHROMIUM(GraphicsContext3D::TEXTURE_2D, 0, updateRect.x(), updateRect.y(), updateRect.width(), updateRect.height(), GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, GraphicsContext3D::WRITE_ONLY);
> 
> I see that you aren't changing behavior here, but could you file a bug on the video guys to check for a NULL return here and handle it?  The mapTexSub..() extension may return null if it can't get a suitable shared memory segment.

Their new patch at https://bugs.webkit.org/show_bug.cgi?id=45069 handles the NULL return value. It will be best if they can also handle the absence of the GL_CHROMIUM_map_sub extension, which I'll file after this lands.

[Kenneth Russell]

(In reply to comment #4)
> (From update of attachment 68127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68127&action=review
> 
> > WebKit/chromium/src/GraphicsContext3D.cpp:807
> > +    if (!internal->initialize(attrs, hostWindow, renderStyle == RenderDirectlyToHostWindow ? true : false)) {
> 
> Nit:  you can omit the ternary.  A relational expression in C++ is type bool already.

Good point. Done.

> > WebKit/chromium/src/GraphicsContext3D.cpp:810
> > +    PassOwnPtr<GraphicsContext3D> result = new GraphicsContext3D(attrs, hostWindow, renderStyle == RenderDirectlyToHostWindow ? true : false);
> 
> Same as above.

Done.

[James Robinson]

Comment on attachment 68142 [details]
Revised patch

R=me

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

> WebCore/ChangeLog:66
> +        (WebCore::LayerRendererChromium::noticeResize):

nit: changelog out of date, this function is called resizeOnscreenContent() now.

[Kenneth Russell]

Committed r67885: <http://trac.webkit.org/changeset/67885>

[Kenneth Russell]

(In reply to comment #8)
> (From update of attachment 68142 [details])
> R=me
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=68142&action=prettypatch
> 
> > WebCore/ChangeLog:66
> > +        (WebCore::LayerRendererChromium::noticeResize):
> 
> nit: changelog out of date, this function is called resizeOnscreenContent() now.

Fixed in landed patch.

[Darin Adler]

Committed r67903: <http://trac.webkit.org/changeset/67903>

[James Robinson]

Looks like you had a mis-merged ChangeLog, Darin.  Reopening this bug.

[Darin Adler]

(In reply to comment #12)
> Looks like you had a mis-merged ChangeLog, Darin.  Reopening this bug.

Yes. webkit-patch is not my friend :-(