Summary: | Avoid context save/restore in GraphicsContext::drawNativeImage | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, simon.fraser | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2016-04-04 11:20:35 PDT
Created attachment 275579 [details]
Patch
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 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(). Created attachment 275591 [details]
Patch
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 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(). Created attachment 275595 [details]
Patch
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 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. Created attachment 275671 [details]
Patch
Created attachment 275675 [details]
Patch
(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 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 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 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 on attachment 275675 [details] Patch Clearing flags on attachment: 275675 Committed r199071: <http://trac.webkit.org/changeset/199071> All reviewed patches have been landed. Closing bug. |