Host GraphicsContext's CTM inside GraphicsContextState
Created attachment 263115 [details] Patch
Comment on attachment 263115 [details] Patch Attachment 263115 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/288285 New failing tests: fast/canvas/canvas-composite-image.html fast/canvas/canvas-composite-canvas.html
Created attachment 263118 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 263115 [details] Patch Attachment 263115 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/288310 New failing tests: fast/canvas/canvas-composite-image.html fast/canvas/canvas-composite-canvas.html
Created attachment 263120 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 263115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263115&action=review > Source/WebCore/ChangeLog:3 > + Host GraphicsContext's CTM inside GraphicsContextState It looks like this patch is quite inadequate. GraphicsContext::applyDeviceScaleFactor(() GraphicsContext::platformApplyDeviceScaleFactor() GraphicsContext::rotate() GraphicsContext::scale() GraphicsContext::translate()
Comment on attachment 263115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263115&action=review >> Source/WebCore/ChangeLog:3 >> + Host GraphicsContext's CTM inside GraphicsContextState > > It looks like this patch is quite inadequate. > > GraphicsContext::applyDeviceScaleFactor(() > GraphicsContext::platformApplyDeviceScaleFactor() > GraphicsContext::rotate() > GraphicsContext::scale() > GraphicsContext::translate() While we're doing this, let's get rid of wkSetBaseCTM().
Created attachment 263241 [details] Patch
Created attachment 263274 [details] Patch
I should make sure all these edits to callers are necessary.
Created attachment 263308 [details] Patch
Created attachment 263314 [details] Patch
Created attachment 263316 [details] Patch
Created attachment 263375 [details] Patch
Created attachment 263376 [details] Patch
Created attachment 263382 [details] Patch
mrobinson: gyuyoung: do you think you could give this a port-specific review? There are some places where the EFL / GTK ports may need to be update, that I haven't found yet. Thanks!
Comment on attachment 263382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263382&action=review > Source/WebCore/platform/graphics/GraphicsContext.cpp:202 > +AffineTransform GraphicsContext::getCTM(IncludeDeviceScale includeScale) const > +{ > + if (paintingDisabled()) > + return AffineTransform(); > + > + AffineTransform result; > + if (includeScale == DefinitelyIncludeDeviceScale) > + result = m_state.userToDeviceSpaceCTM * m_state.ctm; > + else > + result = m_state.ctm; > + > + return result; > +} I think this should assert that m_state.ctm is approx. equal to the platform CTM (if there's a platformContext). > Source/WebCore/platform/graphics/GraphicsContext.h:160 > + AffineTransform userToDeviceSpaceCTM; > + AffineTransform ctm; These are quite big. Can we use unique_ptr and only allocate when necessary? > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:199 > + m_state.ctm = getPlatformCTM(); This is confusing. What if we have no platform context to get the CTM from? > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:111 > + m_state.userToDeviceSpaceCTM = static_cast<AffineTransform>(CGContextGetUserSpaceToDeviceSpaceTransform(platformContext())) * m_state.ctm.inverse(); Why does windows have the inverse?
Comment on attachment 263382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263382&action=review >> Source/WebCore/platform/graphics/GraphicsContext.h:160 >> + AffineTransform ctm; > > These are quite big. Can we use unique_ptr and only allocate when necessary? In my testing, I've found that these matrices are almost never the identity, which means we would immediately be allocating them and keeping them around. Given that we'd be paying the memory cost anyway, but with an additional allocation hit, I don't think this is a good idea. >> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:111 >> + m_state.userToDeviceSpaceCTM = static_cast<AffineTransform>(CGContextGetUserSpaceToDeviceSpaceTransform(platformContext())) * m_state.ctm.inverse(); > > Why does windows have the inverse? CGContextGetDefaultUserSpaceToDeviceSpaceTransform() doesn't exist on Windows, but CGContextGetUserSpaceToDeviceSpaceTransform() does.
Comment on attachment 263382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263382&action=review >>> Source/WebCore/platform/graphics/GraphicsContext.h:160 >>> + AffineTransform ctm; >> >> These are quite big. Can we use unique_ptr and only allocate when necessary? > > In my testing, I've found that these matrices are almost never the identity, which means we would immediately be allocating them and keeping them around. Given that we'd be paying the memory cost anyway, but with an additional allocation hit, I don't think this is a good idea. The transform is the identity 0.4% of the time (based on running layout tests)
Created attachment 263426 [details] Patch for committing
Comment on attachment 263426 [details] Patch for committing The Cairo changes look fine to me.
Given Martin's consideration of the Cairo changes, and that EWS is all green (excepting bot problems), I'm going to land this.
Committed r191295: <http://trac.webkit.org/changeset/191295>
This change causes the following tests to crash in debug builds: canvas/philip/tests/2d.transformation.rotate.wrap.html canvas/philip/tests/2d.transformation.rotate.wrapnegative.html imported/w3c/canvas/2d.transformation.rotate.wrap.html mported/w3c/canvas/2d.transformation.rotate.wrapnegative.html <https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r191295%20(457)/results.html> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010d4c4307 WTFCrash + 39 (Assertions.cpp:321) 1 com.apple.WebCore 0x000000010f9d8624 WebCore::GraphicsContext::getCTM(WebCore::GraphicsContext::IncludeDeviceScale) const + 324 (GraphicsContext.cpp:221) 2 com.apple.WebCore 0x000000010f9dafbf WebCore::GraphicsContext::checkCTMInvariants() const + 47 (GraphicsContext.h:436) 3 com.apple.WebCore 0x000000010f9d83c1 WebCore::GraphicsContext::rotate(float) + 129 (GraphicsContext.cpp:182) 4 com.apple.WebCore 0x000000010f1f7d3c WebCore::CanvasRenderingContext2D::rotate(float) + 508 (CanvasRenderingContext2D.cpp:720) 5 com.apple.WebCore 0x000000010febdd7b WebCore::jsCanvasRenderingContext2DPrototypeFunctionRotate(JSC::ExecState*) + 571 (JSCanvasRenderingContext2D.cpp:1195) 6 ??? 0x00002b2c3ac01028 0 + 47468964220968
Created attachment 263536 [details] Patch
Committed r191324: <http://trac.webkit.org/changeset/191324>
Re-opened since this is blocked by bug 150352
Comment on attachment 263536 [details] Patch Clearing review flag so that this patch does not appear in the review queue. This patch was committed in <http://trac.webkit.org/changeset/191324> per comment #27.