Summary: | [chromium] Change compositor to use GraphicsContext3D rather than GLES2Context | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||
Component: | Layout and Rendering | Assignee: | Kenneth Russell <kbr> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cmarrin, darin, jamesr, senorblanco, vangelis, vrk, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 45913, 45914, 45916, 45921, 45939, 46115 | ||||||||
Bug Blocks: | 45069, 46131 | ||||||||
Attachments: |
|
Description
Kenneth Russell
2010-09-16 11:57:01 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.
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 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 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. Created attachment 68142 [details]
Revised patch
Addressed above code review feedback.
(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. (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 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. Committed r67885: <http://trac.webkit.org/changeset/67885> (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. Committed r67903: <http://trac.webkit.org/changeset/67903> Looks like you had a mis-merged ChangeLog, Darin. Reopening this bug. (In reply to comment #12) > Looks like you had a mis-merged ChangeLog, Darin. Reopening this bug. Yes. webkit-patch is not my friend :-( |