Bug 163349 – [Win][Direct2D] Implement basic SVG support

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

This patch adds missing drawing primitives and SVGImage support to allow more of the SVG test cases to run.

[Brent Fulgham]

Created attachment 291396 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291396&action=review

> Source/WebCore/platform/graphics/Image.cpp:98
> +#if USE(DIRECT2D)
> +    setRenderTarget(ctxt.platformContext());
> +#endif

I don't like these here in cross-platform code. Why are they necessary?

> Source/WebCore/platform/graphics/Image.cpp:215
> +#if USE(DIRECT2D)
> +    setRenderTarget(ctxt.platformContext());
> +#endif

Ditto.

> Source/WebCore/platform/graphics/ImageBuffer.h:89
> +        std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(size, resolutionScale, colorSpace, renderingMode, renderTarget, success));

We don't use bare "new". This should go through a create function.

[Brent Fulgham]

Comment on attachment 291396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291396&action=review

>> Source/WebCore/platform/graphics/ImageBuffer.h:89
>> +        std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(size, resolutionScale, colorSpace, renderingMode, renderTarget, success));
> 
> We don't use bare "new". This should go through a create function.

This *is* the create function! I can move it (and the original) to the implementation file, though.

[Simon Fraser (smfr)]

Comment on attachment 291396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291396&action=review

>>> Source/WebCore/platform/graphics/ImageBuffer.h:89
>>> +        std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(size, resolutionScale, colorSpace, renderingMode, renderTarget, success));
>> 
>> We don't use bare "new". This should go through a create function.
> 
> This *is* the create function! I can move it (and the original) to the implementation file, though.

Oh right, sorry!

[Brent Fulgham]

Comment on attachment 291396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291396&action=review

>> Source/WebCore/platform/graphics/Image.cpp:98
>> +#endif
> 
> I don't like these here in cross-platform code. Why are they necessary?

At the point we actually do a draw, I need access to the underlying Direct2D render target backing the drawing context passed into the method. We often create the Image well before the draw operation happens, so I can't reliably set it at construction time.

I could duplicate all of the GraphicsContext::draw methods related to image, and set the render target to the image there, but this seemed simpler and required less duplication.

I could make a generic "platformSetRenderTarget" that just takes the graphics context and defaults to a no-op for all non-D2D platforms, if you think that would be preferable. It would at least get rid of the #if/def in these two locations.

[Brent Fulgham]

Created attachment 291452 [details]
Patch

[Brent Fulgham]

Created attachment 291493 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291493&action=review

> Source/WebCore/platform/graphics/Image.cpp:96
> +    setPlatformRenderTarget(ctxt);

Not sure this really helps. How does someone writing generic GraphicsContext code know if they have to call this?

[Brent Fulgham]

Comment on attachment 291493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291493&action=review

>> Source/WebCore/platform/graphics/Image.cpp:96
>> +    setPlatformRenderTarget(ctxt);
> 
> Not sure this really helps. How does someone writing generic GraphicsContext code know if they have to call this?

This is more about the nuances of image drawing in Direct2D. The D2D image decode API requires the render target to which we are drawing. The "setPlatformRenderTarget" only exists to thread this value down to the image decoder.

I need this the first time we call "frameImageAtIndex" so that the decode is done properly.

"singlePixelSolidColor" calls this function behind the scenes, so I need a way to get this knowledge passed to the decoder.

AN API like BitmapImage::setRenderTargetForDecoder() might make this more clear. Would that be better?

[Brent Fulgham]

Created attachment 291561 [details]
Patch

[Brent Fulgham]

Created attachment 291642 [details]
Patch

[Brent Fulgham]

Created attachment 291646 [details]
Patch

[Brent Fulgham]

Created attachment 291650 [details]
Patch

[Alex Christensen]

Comment on attachment 291650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291650&action=review

> Source/WebCore/platform/graphics/ImageBuffer.cpp:43
> +    std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(size, resolutionScale, colorSpace, renderingMode, success));

Why no std::make_unique?

> Source/WebCore/platform/graphics/ImageBuffer.cpp:53
> +    std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(size, resolutionScale, colorSpace, renderingMode, graphicsContext, success));

ditto

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:342
> +        WTFLogAlways("Crashed in EndDraw: hr=%ld, first=%ld, second=%ld", hr, first, second);

Crashed -> Failed?
Or we should actually crash.
EndDraw -> endDraw

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:1364
> +            renderTarget->SetTags(1, __LINE__);

Should we assert renderTarget? Should these SetTags calls be debug-only?

[Simon Fraser (smfr)]

Comment on attachment 291650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291650&action=review

> Source/WebCore/platform/graphics/ImageBuffer.cpp:50
> +std::unique_ptr<ImageBuffer> ImageBuffer::create(const FloatSize& size, RenderingMode renderingMode, const GraphicsContext* graphicsContext, float resolutionScale, ColorSpace colorSpace)

Can you rename graphicsContext to targetContext.

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:740
> -    auto ellipse = D2D1::Ellipse(D2D1::Point2F(rect.x(), rect.y()), rect.width(), rect.height());
> +    auto ellipse = D2D1::Ellipse(rect.center(), 0.5 * rect.width(), 0.5 * rect.height());

Is this a bug fix?

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:1334
> +        FloatRect renderTargetSize(FloatPoint(), renderTarget->GetSize());

This is a rect, not a "Size".

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:1337
> +        if (visibleRect.contains(renderTargetSize)) {

I don't understand the geometry here.

[Said Abou-Hallawa]

Comment on attachment 291650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291650&action=review

Please I would urge you to pass the targetContext all the way from the BitmapImage to the ImageDecoder. It looks really ugly to call setRenderTarget() instead of passing the GraphicsContext to frameImageAtIndex().

> Source/WebCore/platform/graphics/BitmapImage.cpp:83
> -NativeImagePtr BitmapImage::frameImageAtIndex(size_t index, SubsamplingLevel subsamplingLevel)
> +NativeImagePtr BitmapImage::frameImageAtIndex(size_t index, const GraphicsContext* targetContext, SubsamplingLevel subsamplingLevel)

Please move the targetContext to the end of the argument list.

> Source/WebCore/platform/graphics/BitmapImage.cpp:93
> +    if (targetContext)
> +        m_source.setDecoderTargetContext(*targetContext);
> +
>      return m_source.frameImageAtIndex(index, subsamplingLevel);

Why do not you pass the targetContext instead:

    return m_source.frameImageAtIndex(index, subsamplingLevel, targetContext);

[Brent Fulgham]

Created attachment 291666 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 291666 [details]
Patch for landing.

Rejecting attachment 291666 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
eCommand.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/editing/BreakBlockquoteCommand.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/BreakBlockquoteCommand.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/BitmapImage.o platform/graphics/BitmapImage.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/2286532

[Brent Fulgham]

Created attachment 291670 [details]
Patch for landing.

[Brent Fulgham]

Created attachment 291671 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 291671 [details]
Patch for landing.

Clearing flags on attachment: 291671

Committed r207357: <http://trac.webkit.org/changeset/207357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.