Bug 59808 – Add utility for converting SkCanvas to CGContext

• Component: New Bugs
• Status: RESOLVED FIXED

[Cary Clark]

Add utility for converting SkCanvas to CGContext

[Cary Clark]

Created attachment 91699 [details]
Patch

[Cary Clark]

Created attachment 91702 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 91702 [details]
Patch

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

I guess I'm not fully understanding what we're doing here, so my comments below are all mostly style-nits and nothing related to the actual substance of this patch.

This reminds me a little of the LocalCurrentGraphicsContext RIAA which I added to WebCore for the Mac port years ago, or the more recent GraphicsContextStateSaver RIAA which Simon Fraser added for the platform-agnostic GraphicsContext in WebCore.

> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:47
> +static CGAffineTransform SkMatrixToCGAffineTransform(const SkMatrix& matrix)
> +{
> +    // CGAffineTransforms don't support perspective transforms, so make sure
> +    // we don't get those.

Normally we have automatic translation between platform types and WebCore types with implicit constructors and operators.  Don't we already have an SkMatrix constructor for AffineTransform?  AffineTransform certainly knows how to convert to a CGAffineTransform.

> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:75
> +    if (!m_cgContext)
> +        return;

Seems a bit odd to conditionally release.  Should this be called releaseIfNeeded()?

> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:88
> +    const SkBitmap& bitmap = device->accessBitmap(/* changePixels=*/true);

We don't normally document arguments like that.  Instead we use enums instead of bools to make them self-documeting. :)  I guess this is a Skia API?

> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:98
> +    contentsTransform = CGAffineTransformTranslate(contentsTransform, 0,
> +        -device->height());

WebKit doesn't have a 80c wrapping rule like Google does.

> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:106
> +        IntRect rect = iter.rect();
> +        CGPathAddRect(clipPath, 0, rect);

No real need for a local here.  I might have even just made a createClipPath() function to abstract this loop.

> Source/WebCore/platform/graphics/skia/BitLockerSkia.h:41
> +// Converts a SkCanvas temporarily to a CGContext
> +class BitLockerSkia {

It clearly does more than that since its class name talks about locking bits (whatever that means?).

> Source/WebCore/platform/graphics/skia/BitLockerSkia.h:45
> +    CGContextRef cgContext();

I think most places we leave a newline before private:  But it's not documented in our style guidelines and really doesn't matter. :)  Just noticed as I was reading.

[Cary Clark]

Created attachment 92127 [details]
Patch

[Cary Clark]

Comment on attachment 91702 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:47
>> +    // we don't get those.
> 
> Normally we have automatic translation between platform types and WebCore types with implicit constructors and operators.  Don't we already have an SkMatrix constructor for AffineTransform?  AffineTransform certainly knows how to convert to a CGAffineTransform.

No, there is no SkMatrix constructor for AffineTransform currently. I don't recommend adding one since an arbitrary SkMatrix can't be represented as an AffineTransform.

This function is copied from skia/ext/skia_utils_mac.mm in the Chromium tree. I assume that I can't get there from here.

>> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:75
>> +        return;
> 
> Seems a bit odd to conditionally release.  Should this be called releaseIfNeeded()?

Done.

>> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:88
>> +    const SkBitmap& bitmap = device->accessBitmap(/* changePixels=*/true);
> 
> We don't normally document arguments like that.  Instead we use enums instead of bools to make them self-documeting. :)  I guess this is a Skia API?

Correct, this is a Skia API. I copied this from ext/skia again, but I can remove the comment if you prefer. (Done.)

>> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:98
>> +        -device->height());
> 
> WebKit doesn't have a 80c wrapping rule like Google does.

Understood. Does that mean that long lines must be unwrapped? I didn't see that in the style guide. (Done.)

>> Source/WebCore/platform/graphics/skia/BitLockerSkia.cpp:106
>> +        CGPathAddRect(clipPath, 0, rect);
> 
> No real need for a local here.  I might have even just made a createClipPath() function to abstract this loop.

The local is required. The result of iter.rect() is a SkIRect. There is no conversion from SkIRect to CGRect, but there is from SkIRect to IntRect, and from IntRect to CGRect.

>> Source/WebCore/platform/graphics/skia/BitLockerSkia.h:41
>> +class BitLockerSkia {
> 
> It clearly does more than that since its class name talks about locking bits (whatever that means?).

Locking bits means that the pixels underneath the bitmap are locked, and made available to the CGContext to modify. When the bit locker goes out of scope, the bits are unlocked, and the calling SkCanvas is once again the owner.

>> Source/WebCore/platform/graphics/skia/BitLockerSkia.h:45
>> +    CGContextRef cgContext();
> 
> I think most places we leave a newline before private:  But it's not documented in our style guidelines and really doesn't matter. :)  Just noticed as I was reading.

Done.

[Eric Seidel (no email)]

Comment on attachment 92127 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        Each time the CGContext is retrieved, it is rebuilt. 
> +        This permits the caller to modify the same canvas 
> +        between calls. 

So this is a feature?  It seems easy to mis-use this.  I wouldn't expect context() to do any work.  But I guess this allows us to interlace CG and Skia calls?  I thought part of the point of this class was to say that CG was in charge until the class goes out of scope?

Do Skia Canvases have any sort of generation number we could use to avoid the rebuild in the common case?  I'm not sure how much it matters, but it looks like context() does a few mallocs at least.

[Cary Clark]

Comment on attachment 92127 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +        between calls. 
> 
> So this is a feature?  It seems easy to mis-use this.  I wouldn't expect context() to do any work.  But I guess this allows us to interlace CG and Skia calls?  I thought part of the point of this class was to say that CG was in charge until the class goes out of scope?
> 
> Do Skia Canvases have any sort of generation number we could use to avoid the rebuild in the common case?  I'm not sure how much it matters, but it looks like context() does a few mallocs at least.

Correct, this allows interlacing CG and Skia calls, or more specifically, interlacing CG-specific calls and abstract graphics calls that map to Skia. CG does own the bits behind the bitmap until this goes out of scope, but generic calls may modify the Skia matrix or clip, which needs to be reapplied to CoreGraphics state.

While it could be mis-used, the number of call sites is small. BitLockerSkia has five call sites in WebKit and four more in the rest of Chrome.

At present, BitLockerSkia::context() does not show up on any performance profile. When it does, I will optimize it. Compared to the Skia and CG calls around it, the few mallocs in BitLockerSkia is very unlikely to ever matter.

[Eric Seidel (no email)]

Comment on attachment 92127 [details]
Patch

I'm not sure I have any more suggstions.  This code doesn't feel quite right to me, but it's probably best to land it and iterate from there.  Glad to see you're making progress getting Skia up on Mac.

[WebKit Commit Bot]

Comment on attachment 92127 [details]
Patch

Clearing flags on attachment: 92127

Committed r85989: <http://trac.webkit.org/changeset/85989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.