Bug 156173 – Avoid context save/restore in GraphicsContext::drawNativeImage

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

In GraphicsContext::drawNativeImage, we save/restore here because we:
* always turn off antialiasing (iOS only, no idea why)
* always translate and scale the CTM to flip
* clip, sometimes

We could apply/unapply these manually, but it would be nice if save/restore were just faster.

[Said Abou-Hallawa]

Created attachment 275579 [details]
Patch

[Said Abou-Hallawa]

Running the Animometer test "canvas bouncing PNG images" with and without the attached patch gives the following results:

Without the patch:
  1. 2357.74
  2. 2361.20
  3. 2480.94
  4. 2273.73

Avg. = 2368.40

With the patch:
  1. 2448.20
  2. 2623.03
  3. 2627.53
  4. 2453.05

Avg. = 2537.95

[Simon Fraser (smfr)]

Comment on attachment 275579 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:183
>      CGContextStateSaver stateSaver(context);

Isn't this doing a save by default?

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:271
> +        CGContextSetShouldAntialias(context, true);

This should restore the state to before the CGContextSetShouldAntialias() call, because it may have been false before.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:88
> +    bool isSaveAndRestore() const

This name is confusing. It would be better as didSave() or willRestore().

[Said Abou-Hallawa]

Created attachment 275591 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 275591 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:185
> +    CGContextStateSaver stateSaver(context);

Don't make a state saver just for this. Use:
bool wasAntialiased = shouldAntialias();
and restore manually.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:236
> +            stateSaver.ensureSaved();

This could just be save()

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:85
> +    void ensureSaved()
> +    {
> +        if (!m_saveAndRestore)
> +            save();
> +    }

I don't know why you need this change.

[Said Abou-Hallawa]

Comment on attachment 275579 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:183
>>      CGContextStateSaver stateSaver(context);
> 
> Isn't this doing a save by default?

Yes, you are right. I was passing false for saveAndStore, but I wanted to run Animometer without the save/restore. So I removed this parameter but I forgot to return it back.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:271
>> +        CGContextSetShouldAntialias(context, true);
> 
> This should restore the state to before the CGContextSetShouldAntialias() call, because it may have been false before.

There is no CGContextGetShouldAntialias() to get the old 'ShouldAntialias' value. Using GraphicsContext::shouldAntialias() might be incorrect for CG. From the documentation of CGContextSetShouldAntialias: "Anti-aliasing is turned on by default when a window or bitmap context is created. It is turned off for other types of contexts.". But GraphicsContextState::shouldAntialias is always set to true.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:88
>> +    bool isSaveAndRestore() const
> 
> This name is confusing. It would be better as didSave() or willRestore().

Name was changed to didSave().

[Said Abou-Hallawa]

Created attachment 275595 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 275591 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:185
>> +    CGContextStateSaver stateSaver(context);
> 
> Don't make a state saver just for this. Use:
> bool wasAntialiased = shouldAntialias();
> and restore manually.

I think using shouldAntialias() is not correct for CG. We always initialize GraphicsContextState::shouldAntialias with true although CG might initialize its shouldAntialias with false for some GraphicsContexts.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:236
>> +            stateSaver.ensureSaved();
> 
> This could just be save()

Calling CGContextStateSaver::save() after it saved the CG state will fire an assertion. So this function is checking if the CG is saved or not before trying to save it. But since we can call CGContextGetShouldAntialias(), there is no need for this function anymore.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:85
>> +    }
> 
> I don't know why you need this change.

Deleted.

[Jon Lee]

Comment on attachment 275595 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:239
> +    bool wasAntialiased = CGContextGetShouldAntialias(context);

Please fix this iOS build failure.

[Said Abou-Hallawa]

Created attachment 275671 [details]
Patch

[Said Abou-Hallawa]

Created attachment 275675 [details]
Patch

[Simon Fraser (smfr)]

(In reply to comment #8)
> Comment on attachment 275591 [details]

> I think using shouldAntialias() is not correct for CG. We always initialize
> GraphicsContextState::shouldAntialias with true although CG might initialize
> its shouldAntialias with false for some GraphicsContexts.

Agreed. We should use CGContextGetShouldAntialias() the first time.

> 
> >> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:236
> >> +            stateSaver.ensureSaved();
> > 
> > This could just be save()
> 
> Calling CGContextStateSaver::save() after it saved the CG state will fire an
> assertion.

In the patch this looked like the first save().

[Said Abou-Hallawa]

Comment on attachment 275591 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:236
>>>> +            stateSaver.ensureSaved();
>>> 
>>> This could just be save()
>> 
>> Calling CGContextStateSaver::save() after it saved the CG state will fire an assertion. So this function is checking if the CG is saved or not before trying to save it. But since we can call CGContextGetShouldAntialias(), there is no need for this function anymore.
> 
> In the patch this looked like the first save().

In this patch:

#if PLATFORM(IOS)
    CGContextStateSaver stateSaver(context);
#else
   CGContextStateSaver stateSaver(context, false);
#endif

So for iOS, this would be the second save.

[Build Bot]

Comment on attachment 275675 [details]
Patch

Attachment 275675 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1104803

New failing tests:
imported/w3c/web-platform-tests/streams/readable-streams/general.https.html

[Build Bot]

Created attachment 275682 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[WebKit Commit Bot]

Comment on attachment 275675 [details]
Patch

Clearing flags on attachment: 275675

Committed r199071: <http://trac.webkit.org/changeset/199071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

<rdar://problem/25473317>