Bug 163349

Summary: [Win][Direct2D] Implement basic SVG support
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebCore Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, dino, pvollan, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Windows 10   
Bug Depends on:    
Bug Blocks: 162953    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing.
none
Patch for landing.
none
Patch for landing. none

Description Brent Fulgham 2016-10-12 11:58:52 PDT
This patch adds missing drawing primitives and SVGImage support to allow more of the SVG test cases to run.
Comment 1 Brent Fulgham 2016-10-12 14:40:50 PDT
Created attachment 291396 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-10-12 18:35:19 PDT
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.
Comment 3 Brent Fulgham 2016-10-12 20:00:23 PDT
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.
Comment 4 Simon Fraser (smfr) 2016-10-12 20:10:22 PDT
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!
Comment 5 Brent Fulgham 2016-10-12 20:20:59 PDT
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.
Comment 6 Brent Fulgham 2016-10-12 21:20:17 PDT
Created attachment 291452 [details]
Patch
Comment 7 Brent Fulgham 2016-10-13 10:49:45 PDT
Created attachment 291493 [details]
Patch
Comment 8 Simon Fraser (smfr) 2016-10-13 11:56:02 PDT
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?
Comment 9 Brent Fulgham 2016-10-13 13:03:55 PDT
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?
Comment 10 Brent Fulgham 2016-10-13 20:29:20 PDT
Created attachment 291561 [details]
Patch
Comment 11 Brent Fulgham 2016-10-14 10:16:23 PDT
Created attachment 291642 [details]
Patch
Comment 12 Brent Fulgham 2016-10-14 10:34:44 PDT
Created attachment 291646 [details]
Patch
Comment 13 Brent Fulgham 2016-10-14 10:53:38 PDT
Created attachment 291650 [details]
Patch
Comment 14 Alex Christensen 2016-10-14 11:45:55 PDT
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?
Comment 15 Simon Fraser (smfr) 2016-10-14 11:48:11 PDT
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.
Comment 16 Said Abou-Hallawa 2016-10-14 11:54:04 PDT
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);
Comment 17 Brent Fulgham 2016-10-14 14:22:28 PDT
Created attachment 291666 [details]
Patch for landing.
Comment 18 WebKit Commit Bot 2016-10-14 14:46:54 PDT
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
Comment 19 Brent Fulgham 2016-10-14 14:58:20 PDT
Created attachment 291670 [details]
Patch for landing.
Comment 20 Brent Fulgham 2016-10-14 15:14:01 PDT
Created attachment 291671 [details]
Patch for landing.
Comment 21 WebKit Commit Bot 2016-10-14 15:44:53 PDT
Comment on attachment 291671 [details]
Patch for landing.

Clearing flags on attachment: 291671

Committed r207357: <http://trac.webkit.org/changeset/207357>
Comment 22 WebKit Commit Bot 2016-10-14 15:44:58 PDT
All reviewed patches have been landed.  Closing bug.