Bug 62213 – Create local CG context for Mac UI elements when Skia is renderer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Cary Clark]

Create local CG context for Mac UI elements when Skia is renderer

[Cary Clark]

Created attachment 96248 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96248 [details]
Patch

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

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:40
> +    CGContextRef cgContext = this->cgContext();

Were this free, I would have just used cgContext() everywhere in the function, so the name is misleading here, since cgContext() sounds like a no-work direct-access function.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:61
> +CGContextRef LocalCurrentGraphicsContext::cgContext()

You might want to name this createCGContext to note that it's creating one.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:64
> +    CGContextRef cgContext = m_skiaBitLocker.cgContext();

We should probably rename this similarly.  Otherwise callers will think that calling cgContext is free()... which it's not IIRC.  I should have picked on you about that before.

> Source/WebCore/rendering/RenderThemeMac.mm:952
> +#if USE(SKIA)
> +    gfx::SkiaBitLocker bitLocker(imageBuffer->context()->platformContext()->canvas());
> +    CGContextRef cgContext = bitLocker.cgContext();
> +#else
> +    CGContextRef cgContext = imageBuffer->context()->platformContext();
> +#endif

This is really ugly.  We need a better way to get these bit-locked contexts out of a platform context.

I think these changes would all be better if we put them in some sort of local unconditional RAII object which was not named bitlocker, but optionally used a bitlocker on Skia and otherwise was just a raw pointer.  These #if USE(SKIA) are super-ugly.

> Source/WebCore/rendering/RenderThemeMac.mm:1066
> +#if USE(SKIA)
> +        context = bitLocker.cgContext();
> +#endif

This is confusing that we grab a new cgContext every time.

> Source/WebCore/rendering/RenderThemeMac.mm:1303
> +#if USE(SKIA)
> +    CGColorSpaceRef cspace = CGColorSpaceCreateDeviceRGB();
> +#else
>      CGColorSpaceRef cspace = deviceRGBColorSpaceRef();
> +#endif

This was a big perf win when we added deviceRGBColorSpaceRef.  I'm not sure you want to remove it for the skia case.

> Source/WebCore/rendering/RenderThemeMac.mm:1770
> -        wkDrawMediaUIPart(btn->displayType(), mediaControllerTheme(), paintInfo.context->platformContext(), r, getMediaUIPartStateFlags(node));
> +        wkDrawMediaUIPart(btn->displayType(), mediaControllerTheme(), localContext.cgContext(), r, getMediaUIPartStateFlags(node));

You're changing the use of this LocalCurrentGraphicsContext, giving it two purposes here.  1.  To do what I mention above, about the local to help hide the SKIA ifdef 2. to do what it already does which is set the current graphics context for the scope.

I'm not sure it's OK to entangle the two like that.  This code clearly wants a helper graphics context holder to abstract the bitlocking.  It's possible that LocalCurrentGraphicsContext could do that as well, but it's certainly not its real job, and re-using it as such just makes the code confusing.

So your local helper.... What shall it be named?
BitLocker?  PaintLock?  What sort of name would make sense that CG would take one too?  or would make clear that it's used to abstract away how to access the underlying CGContext to allow sharing of this code between CG and Skia?

As is this patch is just makign the code ugly and will make others who hack on this file very sad.

[Eric Seidel (no email)]

Comment on attachment 96248 [details]
Patch

To make clear what LocalCurrentGraphicsContext does.  It it a way of setting the [NSGraphicsContext currentContext] which is used by AppKit and other high-level drawing functions above CG.  We have to do that locally before calling up into AppKit, or AppKit gets confused and draws to the wrong place.  We *only* need LocalCurrentGraphicsContext in those cases and should never use it where not absolutely necessary (as iirc it's expensive/slow).

[Cary Clark]

Created attachment 96276 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96276 [details]
Patch

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

Now that I'm writing all this, I realizing how confusing this is.

create methods are normally paired with ref pointers for memory management.  But that's not how bitlocker works, is it?  It throws away the previous context when a new one is created?

I think I need to stare at bitlocker again.  I think that's the real problem with the naming and memory managmenet confusion.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.h:57
> +    CGContextRef createCGContext() { return m_skiaBitLocker.cgContext(); }

We should add a comment that cgCongtext on SkiaBitLocker does a create.  Otherwise it's confusing as to why this method is named create if both variants just provide direct access.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:39
> +    CGContextRef cgContext = this->createCGContext();

this-> isn't needed anymore

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:63
> +    CGContextRef cgContext = m_skiaBitLocker.cgContext();

Again a "why this is create" comment is needed, or we need to rename SkiaBitLocker to follow The Create Rule:
http://developer.apple.com/library/mac/#documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:72
> +        : m_skiaBitLocker(graphicsContext->canvas())

funny indent.

[Cary Clark]

Comment on attachment 96276 [details]
Patch

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

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:63
>> +    CGContextRef cgContext = m_skiaBitLocker.cgContext();
> 
> Again a "why this is create" comment is needed, or we need to rename SkiaBitLocker to follow The Create Rule:
> http://developer.apple.com/library/mac/#documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html

Perhaps updateCGContext() would be a better choice, with a comment that in the case of !USE(SKIA), the CGContext never changes and 'update' is a no-op.

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:72
>> +        : m_skiaBitLocker(graphicsContext->canvas())
> 
> funny indent.

Fixed in next patch.

[Cary Clark]

Created attachment 96412 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96412 [details]
Patch

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

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:64
> +    // this synchronizes the CGContext to reflect the current SkCanvas state
> +    CGContextRef cgContext = m_skiaBitLocker.cgContext();

Capital and period.  It more than just updates the state, it creates a new CGContext, doesn't it?  It of course owns the CGContext.

I'm trying to convey two points here.  1.  that there is a large cost to calling bitlocker::cgContext, and 2. that callers don't need to own the pointer.

[Eric Seidel (no email)]

Comment on attachment 96412 [details]
Patch

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

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:64

> 
> Capital and period.  It more than just updates the state, it creates a new CGContext, doesn't it?  It of course owns the CGContext.
> 
> I'm trying to convey two points here.  1.  that there is a large cost to calling bitlocker::cgContext, and 2. that callers don't need to own the pointer.

We could split these out into two methods.  one ensureCGContext() which creates a new one.  and cgContext() which just gives you access.

right now with cgContext() doing both, things are very confusing.

We could also make it so that there is only ever one cgContext per bitlocker, and if you need to create a new one, you create a new bitlocker.  BUt the current system is too confusing to use, as demonstrated by this patch and my continued confusion. :(

[Eric Seidel (no email)]

Comment on attachment 96412 [details]
Patch

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

I spoke with Cary over the phone and he explained better what we're doing here.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.h:58
> +    // this synchronizes the CGContext to reflect the current SkCanvas state
> +    CGContextRef createCGContext() { return m_skiaBitLocker.cgContext(); }

OK, so I was wrong to tell you to make this createCGContext().  As you previously noted, the "creation" is not the importnat part of this call. 

I would name that back to cgContext() (like your second patch) and just make a note that m_skiaBitLocker.cgContext() may not always return the *same* CGContext object, but it will always represent the same bit, freshly synced with the Skia context.

It's important to comment on this in some way, as otherwise future hackers here may get confused. :)

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:40
> +    CGContextRef cgContext = createCGContext();
> +    if (cgContext == [[NSGraphicsContext currentContext] graphicsPort]) {

Hmmm... Isn't this gonna get us in trouble?  If we're getting multiple different CGContexts to represent the same skia bits, this comparison could fail at times we don't expect it to?

>>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:64
>>> +    CGContextRef cgContext = m_skiaBitLocker.cgContext();
>> 
>> Capital and period.  It more than just updates the state, it creates a new CGContext, doesn't it?  It of course owns the CGContext.
>> 
>> I'm trying to convey two points here.  1.  that there is a large cost to calling bitlocker::cgContext, and 2. that callers don't need to own the pointer.
> 
> 

This comment is good, btw.  That's the side-effect of calling this method, which is good to document.  BUt the other thing we need to document is that the cgContext returned is not guarenteed to be the same every time.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:72
> +CoreGraphicsContainer::CoreGraphicsContainer(PlatformGraphicsContext* graphicsContext) 

I might have made this take a GraphicsContext instead, just to make the callers have to do one less pointer indirection.

> Source/WebCore/rendering/RenderThemeMac.mm:1053
> +        context = cgContextContainer.createCGContext();
>          CGContextDrawShading(context, mainShading.get());

This is one example of why it's strange to not use a sync method.

Why not just change all calls to context to be contextContainer.context() ?

[Eric Seidel (no email)]

I think this is still a little strange, but I think we can move forward with this per our discussion on the phone.

I'm happy to discuss further over irc, email or phone.  I look forward to seeing Skia on the Mac. :)

[Cary Clark]

Created attachment 96590 [details]
Patch

[Cary Clark]

Comment on attachment 96412 [details]
Patch

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

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.h:58
>> +    CGContextRef createCGContext() { return m_skiaBitLocker.cgContext(); }
> 
> OK, so I was wrong to tell you to make this createCGContext().  As you previously noted, the "creation" is not the importnat part of this call. 
> 
> I would name that back to cgContext() (like your second patch) and just make a note that m_skiaBitLocker.cgContext() may not always return the *same* CGContext object, but it will always represent the same bit, freshly synced with the Skia context.
> 
> It's important to comment on this in some way, as otherwise future hackers here may get confused. :)

Done.

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:40
>> +    if (cgContext == [[NSGraphicsContext currentContext] graphicsPort]) {
> 
> Hmmm... Isn't this gonna get us in trouble?  If we're getting multiple different CGContexts to represent the same skia bits, this comparison could fail at times we don't expect it to?

I agree that this is a concern. I think it is correct to save and restore the current state if the context changes. As written, every time the constructor is called, the context has changed. However, in the future, if it is necessary to optimize the conversion code to speed it up, this should be considered very carefully.

I don't know what else to do to improve this for now. Do you have any ideas?

>>>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:64
>>>> +    CGContextRef cgContext = m_skiaBitLocker.cgContext();
>>> 
>>> Capital and period.  It more than just updates the state, it creates a new CGContext, doesn't it?  It of course owns the CGContext.
>>> 
>>> I'm trying to convey two points here.  1.  that there is a large cost to calling bitlocker::cgContext, and 2. that callers don't need to own the pointer.
>> 
>> 
> 
> This comment is good, btw.  That's the side-effect of calling this method, which is good to document.  BUt the other thing we need to document is that the cgContext returned is not guarenteed to be the same every time.

Done.

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:72
>> +CoreGraphicsContainer::CoreGraphicsContainer(PlatformGraphicsContext* graphicsContext) 
> 
> I might have made this take a GraphicsContext instead, just to make the callers have to do one less pointer indirection.

Done.

>> Source/WebCore/rendering/RenderThemeMac.mm:1053
>>          CGContextDrawShading(context, mainShading.get());
> 
> This is one example of why it's strange to not use a sync method.
> 
> Why not just change all calls to context to be contextContainer.context() ?

Done (if I understand your suggestion correctly).

[Eric Seidel (no email)]

Comment on attachment 96590 [details]
Patch

Seems OK.  Adam pointed out we could also call the cgContext method refreshCGContext() (either on these local classes or on bitlocker).  That would allow us to have a separate cgContext() method which did not refresh but only returned the last context (or errored if there was none).  refreshContext() could still return the context pointer of course.

Anyway, this looks OK.

[WebKit Review Bot]

Comment on attachment 96590 [details]
Patch

Clearing flags on attachment: 96590

Committed r88469: <http://trac.webkit.org/changeset/88469>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 96590 [details]
Patch

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

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.h:42
> +    CGContextRef cgContext();

Should be const.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.h:51
> +class ContextContainer {

This name is too generic. I don't know what a ContextContainer is.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:66
> +    CGContextRef cgContext = m_skiaBitLocker.cgContext();

Who owns this context? Does it need to be released?

[Cary Clark]

Comment on attachment 96590 [details]
Patch

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

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.h:42
>> +    CGContextRef cgContext();
> 
> Should be const.

The local m_skiaBitLocker state is changed by cgContext(). Since it acts as a cache, it could be mutable, but leaving it un-const signals that the cgContext function does work.

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.h:51
>> +class ContextContainer {
> 
> This name is too generic. I don't know what a ContextContainer is.

I had named it CGContextContainer. I may have misunderstood Eric's feedback to name it ContextContainer.

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:66
>> +    CGContextRef cgContext = m_skiaBitLocker.cgContext();
> 
> Who owns this context? Does it need to be released?

m_skiaBitLocker manages the CGContextRef and releases it when it goes out of scope.