Bug 43398 – Port Chromium's accelerated compositing to Mac OS X

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kenneth Russell]

The Chromium port of the accelerated compositing feature needs to be ported to Mac OS X.

[Kenneth Russell]

Created attachment 63289 [details]
Patch

From the ChangeLog:

Fixed compilation problems with gcc. Ported to Core Graphics and adjusted scrolling and incremental updating code, taking into account lower-left coordinate system origin. Added WebGLES2Context::resizeOnscreenContent, needed on Mac OS X to report window size changes. Sent resize notifications to WebGLES2Context.

Note that this patch must land after http://codereview.chromium.org/3067026 .

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:855:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


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

[Kenneth Russell]

Created attachment 63292 [details]
Revised patch

Fixed style error in previous patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 63292 [details]
Revised patch

WebCore/platform/graphics/chromium/LayerChromium.cpp:190
 +      OwnPtr<GraphicsContext> graphicsContext(new GraphicsContext(contextCG.get()));
nit: why not just allocate this GraphicsContext on the stack?

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:254
 +      m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, 0);
nit: you can also write m_rootLayerCGContext.adoptCF(0), which I think is more conventional.

WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:117
 +          colorSpace = RetainPtr<CGColorSpaceRef>(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGBLinear));
colorSpace.adoptCF(CGColorSpaceCreateWithName(...));

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:260
 +      m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, CGBitmapContextCreate(m_rootLayerBackingStore.data(),
ditto

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:390
 +          scrolledLayerMatrix.translate3d((int)floorf(0.5 * visibleRect.width() + 0.5) - scrollDelta.x(),
nit: slightly unfortunate to copy/paste this fairly intricate statement.
you could just use a local variable that has a value of either +1 or -1
and include that local variable in the final expression.

WebKit/chromium/public/WebGLES2Context.h:64
 +  #if defined(__APPLE__)
is this the best define, or would it make sense to use WEBKIT_USING_CG instead?
would this code make sense if CG were used on other platforms (hypothetically
speaking).

WebKit/chromium/src/WebViewImpl.cpp:915
 +  #if OS(DARWIN)
DARWIN or CG?

WebKit/chromium/src/WebViewImpl.cpp:2230
 +  #if OS(DARWIN)
ditto

[Kenneth Russell]

Created attachment 63396 [details]
Revised patch

Addressed review feedback.

[Kenneth Russell]

(In reply to comment #4)
> (From update of attachment 63292 [details])
> WebCore/platform/graphics/chromium/LayerChromium.cpp:190
>  +      OwnPtr<GraphicsContext> graphicsContext(new GraphicsContext(contextCG.get()));
> nit: why not just allocate this GraphicsContext on the stack?

Done. I had just copied code from earlier in the file.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:254
>  +      m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, 0);
> nit: you can also write m_rootLayerCGContext.adoptCF(0), which I think is more conventional.

Done.

> WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:117
>  +          colorSpace = RetainPtr<CGColorSpaceRef>(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGBLinear));
> colorSpace.adoptCF(CGColorSpaceCreateWithName(...));

Done.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:260
>  +      m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, CGBitmapContextCreate(m_rootLayerBackingStore.data(),
> ditto

Done.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:390
>  +          scrolledLayerMatrix.translate3d((int)floorf(0.5 * visibleRect.width() + 0.5) - scrollDelta.x(),
> nit: slightly unfortunate to copy/paste this fairly intricate statement.
> you could just use a local variable that has a value of either +1 or -1
> and include that local variable in the final expression.

Good idea. Done.

> WebKit/chromium/public/WebGLES2Context.h:64
>  +  #if defined(__APPLE__)
> is this the best define, or would it make sense to use WEBKIT_USING_CG instead?
> would this code make sense if CG were used on other platforms (hypothetically
> speaking).

The associated #ifdef on the Chrome side is OS_MACOSX, so I think we should use this #ifdef here rather than WEBKIT_USING_CG.

> WebKit/chromium/src/WebViewImpl.cpp:915
>  +  #if OS(DARWIN)
> DARWIN or CG?
> 
> WebKit/chromium/src/WebViewImpl.cpp:2230
>  +  #if OS(DARWIN)
> ditto

In both of these cases again the associated #ifdef in Chrome is OS_MACOSX, so I think this is the right #if.

[Dimitri Glazkov (Google)]

Comment on attachment 63396 [details]
Revised patch

ok. I have no strong feelings with DARWIN vs. USING_WEBKIT_CG. If fishd objects, then we can change.

[Kenneth Russell]

Committed r64656: <http://trac.webkit.org/changeset/64656>

[Nico Weber]

>  107     RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGImageGetColorSpace(cgImage));

I think this is wrong: "CGImageGetColorSpace" doesn't contain "Copy" or "Create", so you're not supposed to release it. I'll send out a patch to fix this.

[Darin Fisher (:fishd, Google)]

Comment on attachment 63396 [details]
Revised patch

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

> WebKit/chromium/public/WebGLES2Context.h:65
> +    virtual void resizeOnscreenContent(const WebSize&) = 0;
nit: the first part of the comment is redundant with the use of the
__APPLE__ macro.  one thing that would be helpful to know is whether
this method does its work synchronously or not.  i notice that the
call to this function happens before we have actually generated new
pixels.  has consideration been given to how we will synchronize
"re-compositing" and "re-sizing" so that if things are fast that
resizing will be smooth without tearing along the edges?

[Kenneth Russell]

Comment on attachment 63396 [details]
Revised patch

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

> WebKit/chromium/public/WebGLES2Context.h:65
> +    virtual void resizeOnscreenContent(const WebSize&) = 0;
This method does its work asynchronously with respect to the renderer, but serially with respect to other graphics calls issued from the renderer. It is required that it runs before we perform the next render cycle because that render occurs into the backing store which is reallocated by this method. More work will be needed to make resizing work well on Mac OS X. See http://crbug.com/53165 .

[Darin Fisher (:fishd, Google)]

Comment on attachment 63396 [details]
Revised patch

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

> WebKit/chromium/public/WebGLES2Context.h:65
> +    virtual void resizeOnscreenContent(const WebSize&) = 0;
OK, that makes sense.  My next question:  why does our OSX port need to be different in this respect?  On Windows at least, I think the resize of the native widget happens before we call WebViewImpl::resize().  Why can't we do the same on OSX?  Why does this need to round-trip through WebViewImpl::resize(), which is an API call?

[Kenneth Russell]

(In reply to comment #12)
> (From update of attachment 63396 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=63396&action=prettypatch
> 
> > WebKit/chromium/public/WebGLES2Context.h:65
> > +    virtual void resizeOnscreenContent(const WebSize&) = 0;
> OK, that makes sense.  My next question:  why does our OSX port need to be different in this respect?  On Windows at least, I think the resize of the native widget happens before we call WebViewImpl::resize().  Why can't we do the same on OSX?  Why does this need to round-trip through WebViewImpl::resize(), which is an API call?

In the Mac port of the compositor we manage the backing store into which the compositor renders, because there is no such concept as sharing window handles or XIDs across processes. This is the fundamental difference between the Mac port and the Windows/Linux ports and why this must be done with an API call which trampolines over to the GPU process, getting it to reallocate its IOSurface and sending the browser a notification of this fact.