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
Brent Fulgham
2016-10-12 11:58:52 PDT
Created attachment 291396 [details]
Patch
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 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 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 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. Created attachment 291452 [details]
Patch
Created attachment 291493 [details]
Patch
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 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? Created attachment 291561 [details]
Patch
Created attachment 291642 [details]
Patch
Created attachment 291646 [details]
Patch
Created attachment 291650 [details]
Patch
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 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 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); Created attachment 291666 [details]
Patch for landing.
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 Created attachment 291670 [details]
Patch for landing.
Created attachment 291671 [details]
Patch for landing.
Comment on attachment 291671 [details] Patch for landing. Clearing flags on attachment: 291671 Committed r207357: <http://trac.webkit.org/changeset/207357> All reviewed patches have been landed. Closing bug. |