| Summary: | [Win][Direct2D] Add initial D2D Bitmap Image handling implementation | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | achristensen, bfulgham, buildbot, dino, pvollan, rniwa | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 161817, 162953 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Brent Fulgham
2016-09-29 16:13:04 PDT
Created attachment 290268 [details]
Patch
Comment on attachment 290268 [details] Patch Attachment 290268 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2171637 New failing tests: fast/images/pixel-crack-image-background-webkit-transform-scale.html Created attachment 290276 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 290268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290268&action=review > Source/WebCore/platform/graphics/ImageBuffer.cpp:84 > +#if !USE(CG) && !USE(DIRECT2D) Argh! This should be an or! Created attachment 290291 [details]
Patch
Created attachment 290293 [details]
Patch
Comment on attachment 290293 [details] Patch Attachment 290293 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2173092 New failing tests: fast/images/pixel-crack-image-background-webkit-transform-scale.html Created attachment 290304 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 290293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290293&action=review I'm skeptical. I think we should use existing code more here. > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:42 > +RefPtr<Uint8ClampedArray> ImageBufferData::getData(const IntRect& rect, const IntSize& size, bool /* accelerateRendering */, bool unmultiplied, float resolutionScale) const Do we really want to implement our own bitmap parser here? Doesn't such a thing already exist? > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:100 > + srcRows = reinterpret_cast<unsigned char*>(data) + originy * srcBytesPerRow + originx * 4; I see no definition for data. Where is this coming from? > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:157 > + ASSERT(destx.unsafeGet() >= 0); > + ASSERT(destx.unsafeGet() < size.width()); > + ASSERT(originx.unsafeGet() >= 0); > + ASSERT(originx.unsafeGet() <= sourceRect.maxX()); It seems like something could go wrong here. Is there no better way? Created attachment 290406 [details]
Patch
Comment on attachment 290406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290406&action=review > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:43 > + std::unique_ptr<GraphicsContext> context; Do we need to make a new GraphicsContext for each ImageBufferData? > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:44 > + ID2D1RenderTarget* m_compatibleTarget; { nullptr }; > Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:56 > + fastFree(const_cast<void*>(data)); could we make data a void* or make fastFree take a const void*? Were malloc and free defined before const was in the C language? > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:62 > + // Set to match value used for CoreGraphics. > + return 13088; These values should be united. They should probably also be remeasured. Comment on attachment 290406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290406&action=review >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:43 >> + std::unique_ptr<GraphicsContext> context; > > Do we need to make a new GraphicsContext for each ImageBufferData? Yes, it's just a wrapper around the bitmap so we can use general WebKit drawing functions. It it's exactly the same as our CG implementation. >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:44 >> + ID2D1RenderTarget* m_compatibleTarget; > > { nullptr }; OK! >> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:56 >> + fastFree(const_cast<void*>(data)); > > could we make data a void* or make fastFree take a const void*? Were malloc and free defined before const was in the C language? Sorry - I will delete this. It's some leftover cruft from the CG version I should have removed. >> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:62 >> + return 13088; > > These values should be united. They should probably also be remeasured. Sure. I'll lift this into the general C++ file. Comment on attachment 290406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290406&action=review >>> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:62 >>> + return 13088; >> >> These values should be united. They should probably also be remeasured. > > Sure. I'll lift this into the general C++ file. Actually, I can't. There is no common 'ImageDecoder.cpp' file I can use to hold this. It's likely these values might be different on Windows and CG, so I suggest leaving this placeholder for now. Created attachment 290491 [details]
Patch
Comment on attachment 290491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290491&action=review > Source/WebCore/platform/graphics/ImageBuffer.h:157 > void flushContext() const; > +#elif USE(DIRECT2D) > + void flushContext() const; Wonder if it is better to have this separate, so it's only declared once. (In reply to comment #15) > Comment on attachment 290491 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290491&action=review > > > Source/WebCore/platform/graphics/ImageBuffer.h:157 > > void flushContext() const; > > +#elif USE(DIRECT2D) > > + void flushContext() const; > > Wonder if it is better to have this separate, so it's only declared once. Yeah -- let's leave it for now. I haven't finished fleshing out the Canvas backing store yet, and might share more of these routines. I just didn't need them yet. Committed r206742: <http://trac.webkit.org/changeset/206742> Comment on attachment 290491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290491&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:146 > +#if USE(DIRECT2D) > + setRenderTarget(context); > +#endif This change is a little bit hacky. I understand you do not want to make changes specific to a single port which affect all the other ports. But to move the RenderTarget form BitmapImage to ImageSource and then to the ImageDecoder to be used by one single function looks weird. The ImageDecoder uses m_renderTarget only in ImageDecoder::createFrameImageAtIndex(). Is there any guarantee the render target GraphicsContext be valid if createFrameImageAtIndex() is called outside BitmapImage::draw()? > Source/WebCore/platform/graphics/ImageFrameCache.cpp:388 > +#if USE(DIRECT2D) > +void ImageFrameCache::setRenderTarget(GraphicsContext& context) > +{ > + if (m_decoder) > + m_decoder->setRenderTarget(context.platformContext()); > +} > +#endif > + Accessing the decoder should not be through the ImageFrameCache. It can accessed directly from the ImageSource: #if USE(DIRECT2D) void ImageSource::setRenderTarget(GraphicsContext& context) { if (!isDecoderAvailable()) return; m_decoder->setRenderTarget(context.platformContext()); } #endif > Source/WebCore/platform/graphics/ImageFrameCache.h:48 > + ImageDecoder* decoder() const { return m_decoder; } > + Why this function was added to ImageFrameCache? No one is calling it. > Source/WebCore/platform/graphics/ImageFrameCache.h:88 > +#if USE(DIRECT2D) > + void setRenderTarget(GraphicsContext&); > +#endif > + This definition should be deleted from this class. > Source/WebCore/platform/graphics/ImageSource.h:102 > +#if USE(DIRECT2D) > + void setRenderTarget(GraphicsContext& context) { m_frameCache.setRenderTarget(context); } > +#endif > + The implementation should be moved to the source file because it can access the decoder directly without having to go trough m_frameCache. > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:125 > + COMPtr<IWICBitmapFrameDecode> frame; > + HRESULT hr = m_nativeDecoder->GetFrame(index, &frame); > + if (!SUCCEEDED(hr)) > + return IntSize(); Do we know whether windows caches the returned frame to be reused when it is requested or not? If not just getting the frames for every single metadata is very costly. To fill the metadata of an ImageFrame, GetFrame() will be called multiple times. > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:141 > + HRESULT hr = m_nativeDecoder->GetFrame(index, &frame); This implementation does not seem right to me. isComplete() is intended to check whether the frame is complete or partially loaded. Getting the frame should succeed regardless whether it's complete or not. Also getting the frame is expensive operation but isComplete should be easy and fast. > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:145 > + if (!SUCCEEDED(hr)) > + return false; > + > + return true; Can't we just return SUCCEEDED(hr)? > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:187 > + if (!m_nativeDecoder) > + return 0; > + > + COMPtr<IWICBitmapFrameDecode> frame; > + HRESULT hr = m_nativeDecoder->GetFrame(index, &frame); > + if (!SUCCEEDED(hr)) > + return 0; > + > + UINT width, height; > + hr = frame->GetSize(&width, &height); > + if (!SUCCEEDED(hr)) > + return 0; > + > + return width * height * 4; Can't this be written like this: IntSize frameSize = frameSizeAtIndex(index, subsamplingLevel); return frameSize.area() * 4; > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:216 > + ASSERT(m_renderTarget); > + if (!m_renderTarget) > + return nullptr; Should not this assertion and if-statment moved to the beginning of this function? > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:219 > + hr = m_renderTarget->CreateBitmapFromWicBitmap(converter.get(), nullptr, &bitmap); Since this is the only place that uses m_renderTarget, it can be passed to this function form ImageSource::createFrameImageAtIndex(). The RenderTarget can be set in ImageSource and only be used when calling this function. > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:241 > + m_nativeDecoder = nullptr; Why do we have to reset m_nativeDecoder to null every time we set the data? How can this work for incremental data setting? > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:39 > +class ImageDecoder { It is unfortunate that we are adding the third header file for ImageDecoder. I am planning to combine the three headers in one header file with different three sources. > Source/WebCore/platform/graphics/win/ImageDirect2D.cpp:85 > + size_t currentFrame = m_currentFrame; > + m_currentFrame = i; > + draw(ctxt, dstRect, FloatRect(0.0f, 0.0f, srcSize.width(), srcSize.height()), compositeOp, BlendModeNormal, ImageOrientationDescription()); > + m_currentFrame = currentFrame; This part is a little bit hacky? Can't we add a new function BitmapImage::drawInternal(... size_t frameIndex) and use it here and in BitmapImage::draw()? (In reply to comment #18) Thanks for looking at this change. Since this is already landed, why don't I file a new bug and address your comments there? I think I can incorporate pretty much all of them just as you described. Comment on attachment 290491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290491&action=review >> Source/WebCore/platform/graphics/ImageFrameCache.cpp:388 >> + > > Accessing the decoder should not be through the ImageFrameCache. It can accessed directly from the ImageSource: > > #if USE(DIRECT2D) > void ImageSource::setRenderTarget(GraphicsContext& context) > { > if (!isDecoderAvailable()) > return; > m_decoder->setRenderTarget(context.platformContext()); > } > #endif Done. >> Source/WebCore/platform/graphics/ImageFrameCache.h:48 >> + > > Why this function was added to ImageFrameCache? No one is calling it. Removed. >> Source/WebCore/platform/graphics/ImageFrameCache.h:88 >> + > > This definition should be deleted from this class. Done. >> Source/WebCore/platform/graphics/ImageSource.h:102 >> + > > The implementation should be moved to the source file because it can access the decoder directly without having to go trough m_frameCache. Done. >> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:125 >> + return IntSize(); > > Do we know whether windows caches the returned frame to be reused when it is requested or not? If not just getting the frames for every single metadata is very costly. To fill the metadata of an ImageFrame, GetFrame() will be called multiple times. GetFrame, contrary to the name, doesn't actually decode the image or return the bits. It returns a pointer to an object that knows how to do this work, and that can answer questions about the image contents. I believe this is a low-cost object. >> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:141 >> + HRESULT hr = m_nativeDecoder->GetFrame(index, &frame); > > This implementation does not seem right to me. isComplete() is intended to check whether the frame is complete or partially loaded. Getting the frame should succeed regardless whether it's complete or not. Also getting the frame is expensive operation but isComplete should be easy and fast. I think you are right, but I'll have to do more research to determine how it assesses whether the load is complete. The IWICBitmapFrameDecode doesn't seem to provide any status information. It may be that we have to provide a custom implementation that provides this data. >> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:145 >> + return true; > > Can't we just return SUCCEEDED(hr)? Yes. >> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:187 >> + return width * height * 4; > > Can't this be written like this: > > IntSize frameSize = frameSizeAtIndex(index, subsamplingLevel); > return frameSize.area() * 4; Yep! >> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:216 >> + return nullptr; > > Should not this assertion and if-statment moved to the beginning of this function? Good idea. >> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:219 >> + hr = m_renderTarget->CreateBitmapFromWicBitmap(converter.get(), nullptr, &bitmap); > > Since this is the only place that uses m_renderTarget, it can be passed to this function form ImageSource::createFrameImageAtIndex(). The RenderTarget can be set in ImageSource and only be used when calling this function. It will require threading this state through a few functions, but that might be a better approach. I'll give it a try and see. >> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.h:39 >> +class ImageDecoder { > > It is unfortunate that we are adding the third header file for ImageDecoder. I am planning to combine the three headers in one header file with different three sources. That shouldn't be a problem! >> Source/WebCore/platform/graphics/win/ImageDirect2D.cpp:85 >> + m_currentFrame = currentFrame; > > This part is a little bit hacky? Can't we add a new function BitmapImage::drawInternal(... size_t frameIndex) and use it here and in BitmapImage::draw()? Sure -- I think that would be great! (In reply to comment #20) > > Since this is the only place that uses m_renderTarget, it can be passed to this function form ImageSource::createFrameImageAtIndex(). The RenderTarget can be set in ImageSource and only be used when calling this function. I don't think this will work well, since this one method is the basis of a set of template functions that are spread through a number of methods in ImageSource. We would have to add the render target argument to all of those functions. An alternative might be to store the render target argument in ImageSource (rather than ImageDecoder), and pass it in each function. However, the fact that the render target is needed by the decoder (not ImageSource) makes me prefer to set it when the decoder is created. Comment on attachment 290491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290491&action=review >> Source/WebCore/platform/graphics/BitmapImage.cpp:146 >> +#endif > > This change is a little bit hacky. I understand you do not want to make changes specific to a single port which affect all the other ports. But to move the RenderTarget form BitmapImage to ImageSource and then to the ImageDecoder to be used by one single function looks weird. The ImageDecoder uses m_renderTarget only in ImageDecoder::createFrameImageAtIndex(). Is there any guarantee the render target GraphicsContext be valid if createFrameImageAtIndex() is called outside BitmapImage::draw()? It might be better to pass the render target at the time we access the frame data itself. However, we create a render target for the on-screen view, and another "internal" context for our backing store. Both live for the life of the program. There are cases where the render target may get wedged into a bad state, which is usually resolved by destroying and recreating it. So your approach is superior in this regard. However, it would require threading the render target through a dozen or more functions as I outlined in my other notes. Comment on attachment 290491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290491&action=review >>> Source/WebCore/platform/graphics/win/ImageDirect2D.cpp:85 >>> + m_currentFrame = currentFrame; >> >> This part is a little bit hacky? Can't we add a new function BitmapImage::drawInternal(... size_t frameIndex) and use it here and in BitmapImage::draw()? > > Sure -- I think that would be great! I took a shot at this, and realized it would work so well. Several subroutines called by 'draw' also depend on the state of "m_currentFrame", so we would have to modify those routines as well. |