Bug 150146 – Host GraphicsContext's CTM inside GraphicsContextState

• Status: REOPENED
• Component: New Bugs

[Myles C. Maxfield]

Host GraphicsContext's CTM inside GraphicsContextState

[Myles C. Maxfield]

Created attachment 263115 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

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()

[Myles C. Maxfield]

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().

[Myles C. Maxfield]

Created attachment 263241 [details]
Patch

[Myles C. Maxfield]

Created attachment 263274 [details]
Patch

[Myles C. Maxfield]

I should make sure all these edits to callers are necessary.

[Myles C. Maxfield]

Created attachment 263308 [details]
Patch

[Myles C. Maxfield]

Created attachment 263314 [details]
Patch

[Myles C. Maxfield]

Created attachment 263316 [details]
Patch

[Myles C. Maxfield]

Created attachment 263375 [details]
Patch

[Myles C. Maxfield]

Created attachment 263376 [details]
Patch

[Myles C. Maxfield]

Created attachment 263382 [details]
Patch

[Myles C. Maxfield]

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!

[Simon Fraser (smfr)]

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?

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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)

[Myles C. Maxfield]

Created attachment 263426 [details]
Patch for committing

[Martin Robinson]

Comment on attachment 263426 [details]
Patch for committing

The Cairo changes look fine to me.

[Myles C. Maxfield]

Given Martin's consideration of the Cairo changes, and that EWS is all green (excepting bot problems), I'm going to land this.

[Myles C. Maxfield]

Committed r191295: <http://trac.webkit.org/changeset/191295>

[Ryan Haddad]

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

[Myles C. Maxfield]

Created attachment 263536 [details]
Patch

[Myles C. Maxfield]

Committed r191324: <http://trac.webkit.org/changeset/191324>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 150352

[Daniel Bates]

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.