Summary: | Optimize Canvas fill and drawImage with SourceIn, DestinationIn, SourceOut, and DestinationAtop using transparencyLayer. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||||
Component: | Canvas | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, allan.jensen, benwells, commit-queue, darin, esprehn+autocc, gustavo, gyuyoung.kim, hausmann, kbr, kenneth, kling, krit, mdelaney7, pnormand, rhodovan.u-szeged, rniwa, simon.fraser, webkit.review.bot, xan.lopez, zherczeg | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 79658, 131538 | ||||||||||||||||||
Bug Blocks: | 131303 | ||||||||||||||||||
Attachments: |
|
Description
Dongseong Hwang
2012-02-27 04:10:50 PST
Created attachment 129010 [details]
patch
Attachment 129010 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/GraphicsContext.h:369: The parameter name "op" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/graphics/GraphicsContext.h:547: The parameter name "op" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 129011 [details]
patch v.2
Comment on attachment 129011 [details] patch v.2 Attachment 129011 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11640014 Comment on attachment 129011 [details] patch v.2 Attachment 129011 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11628988 Comment on attachment 129011 [details] patch v.2 View in context: https://bugs.webkit.org/attachment.cgi?id=129011&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:935 > +#if PLATFORM(QT) We try to discourage platform #ifdefs like this in common code. It would be much better if the #ifdef was something like TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR or something. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:990 > -void GraphicsContext::endPlatformTransparencyLayer() > +void GraphicsContext::endPlatformTransparencyLayer(CompositeOperator) I think this patch should at least add an assertion that the operator is srcOver on all platforms where you leave it unimplemented. Thank for guide. Can I define guard in wtf/Platform.h ? Created attachment 129122 [details]
patch v.3
Defined ENABLE_TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR. Build fix for cairo. Comment on attachment 129122 [details] patch v.3 View in context: https://bugs.webkit.org/attachment.cgi?id=129122&action=review > Source/JavaScriptCore/wtf/Platform.h:824 > +#if !defined(ENABLE_TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR) > +#define ENABLE_TRANSPARENCYLAYER_USES_COMPOSITING_OPERATOR 0 > +#endif Ugh, this will force a world rebuild for everyone. Just put this in GraphicsContext.h or somewhere. Created attachment 129128 [details]
patch v.4
I hesitated that it was right, also. This patch is applied to only Qt. However, Qt will do correctly after Bug 79658 is committed. Comment on attachment 129128 [details] patch v.4 View in context: https://bugs.webkit.org/attachment.cgi?id=129128&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:943 > fullCanvasCompositedFill(m_path); Can't this logic go to fullCanvasCompositedFill instead? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1064 > fullCanvasCompositedFill(rect); Ditto. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1305 > fullCanvasCompositedDrawImage(cachedImage->imageForRenderer(image->renderer()), ColorSpaceDeviceRGB, normalizedDstRect, normalizedSrcRect, op); fullCanvasCompositedDrawImage here. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1397 > fullCanvasCompositedDrawImage(buffer, ColorSpaceDeviceRGB, bufferSrcRect, srcRect, state().m_globalComposite); Ditto. What is the current status of this bug? (In reply to comment #15) > What is the current status of this bug? Does it apply and work on the qt5x2 branch https://gitorious.org/qtwebkit/qt5x2/ ? For webkit trunk I guess it would need one of the other ports to be using the code before it would be accepted. Comment on attachment 129128 [details]
patch v.4
The current patch is a Qt-only optimization, and that port is no longer in TOT WebKit. It would be OK to do that optimization for some other platform, or for all platforms.
Created attachment 228730 [details]
Patch
Comment on attachment 228730 [details]
Patch
r=me, awesome!
Maybe you could check in the test first and let the bot chew on it for one round, so we can see the needle jump? ;)
Committed r167123: <http://trac.webkit.org/changeset/167123> Re-opened since this is blocked by bug 131538 Committed r167124: <http://trac.webkit.org/changeset/167124> Created attachment 229128 [details]
Patch for landing
Created attachment 229271 [details]
Patch
Comment on attachment 229271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229271&action=review > Source/WebCore/ChangeLog:18 > + An inline function will create a transparency layer for CG. Cairo graphics > + does not composite correctly when a transparency layer gets created. > + The inline function is just a NOOP for Cairo. Why is this OK for Cairo? Does this behave correctly on Cairo? (In reply to comment #25) > (From update of attachment 229271 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229271&action=review > > > Source/WebCore/ChangeLog:18 > > + An inline function will create a transparency layer for CG. Cairo graphics > > + does not composite correctly when a transparency layer gets created. > > + The inline function is just a NOOP for Cairo. > > Why is this OK for Cairo? Does this behave correctly on Cairo? Yes, I build it with Cairo as well. The existing tests pass now which were skipped before. Comment on attachment 229271 [details] Patch Clearing flags on attachment: 229271 Committed r167248: <http://trac.webkit.org/changeset/167248> All reviewed patches have been landed. Closing bug. |