Bug 45912 - [chromium] Change compositor to use GraphicsContext3D rather than GLES2Context
Summary: [chromium] Change compositor to use GraphicsContext3D rather than GLES2Context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on: 45913 45914 45916 45921 45939 46115
Blocks: 45069 46131
  Show dependency treegraph
 
Reported: 2010-09-16 11:57 PDT by Kenneth Russell
Modified: 2010-09-20 18:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch (76.29 KB, patch)
2010-09-20 13:11 PDT, Kenneth Russell
kbr: commit-queue-
Details | Formatted Diff | Diff
Revised patch (76.32 KB, patch)
2010-09-20 15:04 PDT, Kenneth Russell
jamesr: review+
kbr: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-09-16 11:57:01 PDT
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.
Comment 1 Kenneth Russell 2010-09-20 13:11:38 PDT
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.
Comment 2 WebKit Review Bot 2010-09-20 13:15:23 PDT
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.
Comment 3 James Robinson 2010-09-20 13:42:07 PDT
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.
Comment 4 Stephen White 2010-09-20 13:50:31 PDT
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.
Comment 5 Kenneth Russell 2010-09-20 15:04:01 PDT
Created attachment 68142 [details]
Revised patch

Addressed above code review feedback.
Comment 6 Kenneth Russell 2010-09-20 15:06:05 PDT
(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.
Comment 7 Kenneth Russell 2010-09-20 15:06:24 PDT
(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.
Comment 8 James Robinson 2010-09-20 15:20:09 PDT
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.
Comment 9 Kenneth Russell 2010-09-20 15:28:56 PDT
Committed r67885: <http://trac.webkit.org/changeset/67885>
Comment 10 Kenneth Russell 2010-09-20 15:30:45 PDT
(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.
Comment 11 Darin Adler 2010-09-20 18:06:29 PDT
Committed r67903: <http://trac.webkit.org/changeset/67903>
Comment 12 James Robinson 2010-09-20 18:09:19 PDT
Looks like you had a mis-merged ChangeLog, Darin.  Reopening this bug.
Comment 13 Darin Adler 2010-09-20 18:12:11 PDT
(In reply to comment #12)
> Looks like you had a mis-merged ChangeLog, Darin.  Reopening this bug.

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