Bug 156173 - Avoid context save/restore in GraphicsContext::drawNativeImage
Summary: Avoid context save/restore in GraphicsContext::drawNativeImage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-04-04 11:20 PDT by Said Abou-Hallawa
Modified: 2016-04-05 14:10 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.69 KB, patch)
2016-04-04 15:00 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.38 KB, patch)
2016-04-04 16:56 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.76 KB, patch)
2016-04-04 17:21 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.94 KB, patch)
2016-04-05 09:50 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.47 KB, patch)
2016-04-05 11:03 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (823.47 KB, application/zip)
2016-04-05 12:23 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2016-04-04 11:20:35 PDT
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.
Comment 1 Said Abou-Hallawa 2016-04-04 15:00:24 PDT
Created attachment 275579 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-04-04 15:01:08 PDT
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
Comment 3 Simon Fraser (smfr) 2016-04-04 15:06:50 PDT
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().
Comment 4 Said Abou-Hallawa 2016-04-04 16:56:28 PDT
Created attachment 275591 [details]
Patch
Comment 5 Simon Fraser (smfr) 2016-04-04 17:03:08 PDT
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.
Comment 6 Said Abou-Hallawa 2016-04-04 17:07:54 PDT
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().
Comment 7 Said Abou-Hallawa 2016-04-04 17:21:25 PDT
Created attachment 275595 [details]
Patch
Comment 8 Said Abou-Hallawa 2016-04-04 17:28:30 PDT
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.
Comment 9 Jon Lee 2016-04-04 21:50:23 PDT
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.
Comment 10 Said Abou-Hallawa 2016-04-05 09:50:24 PDT
Created attachment 275671 [details]
Patch
Comment 11 Said Abou-Hallawa 2016-04-05 11:03:25 PDT
Created attachment 275675 [details]
Patch
Comment 12 Simon Fraser (smfr) 2016-04-05 11:52:20 PDT
(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().
Comment 13 Said Abou-Hallawa 2016-04-05 12:03:25 PDT
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.
Comment 14 Build Bot 2016-04-05 12:23:00 PDT
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
Comment 15 Build Bot 2016-04-05 12:23:02 PDT
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
Comment 16 WebKit Commit Bot 2016-04-05 13:24:05 PDT
Comment on attachment 275675 [details]
Patch

Clearing flags on attachment: 275675

Committed r199071: <http://trac.webkit.org/changeset/199071>
Comment 17 WebKit Commit Bot 2016-04-05 13:24:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Said Abou-Hallawa 2016-04-05 14:10:45 PDT
<rdar://problem/25473317>