The implementation of ImageBufferDataDirect2D did not properly handle the case of a 'getData' request for less than the size of the full bitmap. The implementation of 'Clear' was not correct when the size of the region to be cleared is less than the size of the render target. This patch corrects both problems.
Created attachment 378941 [details] Patch
Comment on attachment 378941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378941&action=review > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:122 > + HRESULT hr = d2dDeviceContext->CreateBitmap(rect.size(), nullptr, bytesPerRowInData, bitmapProperties2, &cpuBitmap); If you copy only the desired rect instead of whole rect, you need to keep the previous rect and check it with current rect. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp?rev=249839#L113 - if (bitmapBufferSync == BitmapBufferSync::BufferOutOfSync || data.size() != numBytes.unsafeGet()) { + if (bitmapBufferSync == BitmapBufferSync::BufferOutOfSync || rect != m_savedRect) { Checking only data size is not sufficient because 2x3 == 3x2. I don't think it is no so bad idea copying the whole buffer every time.
Comment on attachment 378941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378941&action=review >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:122 >> + HRESULT hr = d2dDeviceContext->CreateBitmap(rect.size(), nullptr, bytesPerRowInData, bitmapProperties2, &cpuBitmap); > > If you copy only the desired rect instead of whole rect, you need to keep the previous rect and check it with current rect. > https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp?rev=249839#L113 > - if (bitmapBufferSync == BitmapBufferSync::BufferOutOfSync || data.size() != numBytes.unsafeGet()) { > + if (bitmapBufferSync == BitmapBufferSync::BufferOutOfSync || rect != m_savedRect) { > > Checking only data size is not sufficient because 2x3 == 3x2. > I don't think it is no so bad idea copying the whole buffer every time. Remove the variable size if it becomes unused.
Comment on attachment 378941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378941&action=review >>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:122 >>> + HRESULT hr = d2dDeviceContext->CreateBitmap(rect.size(), nullptr, bytesPerRowInData, bitmapProperties2, &cpuBitmap); >> >> If you copy only the desired rect instead of whole rect, you need to keep the previous rect and check it with current rect. >> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp?rev=249839#L113 >> - if (bitmapBufferSync == BitmapBufferSync::BufferOutOfSync || data.size() != numBytes.unsafeGet()) { >> + if (bitmapBufferSync == BitmapBufferSync::BufferOutOfSync || rect != m_savedRect) { >> >> Checking only data size is not sufficient because 2x3 == 3x2. >> I don't think it is no so bad idea copying the whole buffer every time. > > Remove the variable size if it becomes unused. By removing BitmapBufferSync::BufferOutOfSync, the condition can be simplified: if (rect != m_savedRect) { Then, markBufferOutOfSync should be changed: https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp?rev=249839#L149 - void markBufferOutOfSync() { bitmapBufferSync = BitmapBufferSync::BufferOutOfSync; } + void markBufferOutOfSync() { m_savedRect.setSize({ }); }
Comment on attachment 378941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378941&action=review >>>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:122 >>>> + HRESULT hr = d2dDeviceContext->CreateBitmap(rect.size(), nullptr, bytesPerRowInData, bitmapProperties2, &cpuBitmap); >>> >>> If you copy only the desired rect instead of whole rect, you need to keep the previous rect and check it with current rect. >>> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp?rev=249839#L113 >>> - if (bitmapBufferSync == BitmapBufferSync::BufferOutOfSync || data.size() != numBytes.unsafeGet()) { >>> + if (bitmapBufferSync == BitmapBufferSync::BufferOutOfSync || rect != m_savedRect) { >>> >>> Checking only data size is not sufficient because 2x3 == 3x2. >>> I don't think it is no so bad idea copying the whole buffer every time. >> >> Remove the variable size if it becomes unused. > > By removing BitmapBufferSync::BufferOutOfSync, the condition can be simplified: > if (rect != m_savedRect) { > > Then, markBufferOutOfSync should be changed: > https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp?rev=249839#L149 > - void markBufferOutOfSync() { bitmapBufferSync = BitmapBufferSync::BufferOutOfSync; } > + void markBufferOutOfSync() { m_savedRect.setSize({ }); } I think your idea about the backing buffer is correct. We should maintain an in-memory version of the GPU bitmap, and always copy the entire buffer to memory, not just the bit being queried. Then, the read/write operations need to be adjusted to make sure we only read the relevant portion of the in-memory data to the ImageBuffer.getData caller, and only updated the relevant portions of the backing buffer during calls to ImageBuffer.putData. I don't think I agree with the idea to remove the sync state flags, because I am trying to avoid GPU read-backs in cases where we have not uploaded to the GPU (or modified the GPU version of the image). This is common when a chain of filter operations are performed on the memory representation of an image.
Created attachment 379021 [details] Patch
Comment on attachment 379021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379021&action=review > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:89 > + unsigned srcStride = sourceSize.width() * 4; width of sourceRect is not srcStride. copyRectFromSourceToDest should take sourceBufferSize. > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:125 > + return copyRectFromSourceToDest(rect, data.data(), rect.size(), IntPoint(), result->data()); The first argument 'rect' is a desired rect, not a data canvans size. You need to pass data canvas size. > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:272 > { ImageBufferData::putData needs to take BitmapBufferSync::BufferOutOfSync into account. In case of BitmapBufferSync::BufferOutOfSync and partial putData, putData need to call readDataFromBitmapIfNeeded before copying data. > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:326 > + } else if (!copyRectFromSourceToData(sourceRect, source)) ImageBufferData::putData needs to convert if byteFormat doens't match with sourceFormat.
Comment on attachment 379021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379021&action=review > Source/WebCore/ChangeLog:16 > + This patch corrects all three problems. DRT and WKTR patches aren't landed yet. Please lant them if you have. > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:106 > + } Use memcpy. Call memcpy sourceSize.height() times.
Comment on attachment 379021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379021&action=review > Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:1172 > + save(platformContext); I think this is very bad. This ends up to stack mismatch. If you need at least one render state should be pushed, You should push a render state in PlatformContextDirect2D ctor.
Comment on attachment 379021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379021&action=review >> Source/WebCore/ChangeLog:16 >> + This patch corrects all three problems. > > DRT and WKTR patches aren't landed yet. > Please lant them if you have. These don't exist yet. I've been running tests manually (in MiniBrowser). >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:89 >> + unsigned srcStride = sourceSize.width() * 4; > > width of sourceRect is not srcStride. copyRectFromSourceToDest should take sourceBufferSize. Fixed. >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:106 >> + } > > Use memcpy. Call memcpy sourceSize.height() times. Will do. >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:125 >> + return copyRectFromSourceToDest(rect, data.data(), rect.size(), IntPoint(), result->data()); > > The first argument 'rect' is a desired rect, not a data canvans size. You need to pass data canvas size. Fixed. >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:272 >> { > > ImageBufferData::putData needs to take BitmapBufferSync::BufferOutOfSync into account. > In case of BitmapBufferSync::BufferOutOfSync and partial putData, putData need to call readDataFromBitmapIfNeeded before copying data. Oh! Good point. >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:326 >> + } else if (!copyRectFromSourceToData(sourceRect, source)) > > ImageBufferData::putData needs to convert if byteFormat doens't match with sourceFormat. Will fix.
Created attachment 379062 [details] Patch
Created attachment 379087 [details] Patch
(In reply to Fujii Hironori from comment #9) > Comment on attachment 379021 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379021&action=review > > > Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:1172 > > + save(platformContext); > > I think this is very bad. This ends up to stack mismatch. > If you need at least one render state should be pushed, You should push a > render state in PlatformContextDirect2D ctor. Sorry -- I forgot to address this. I've removed this from the patch, and uploaded it. I added some assertions to save/restore to help identify where these might get out of sync.
Comment on attachment 379087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379087&action=review > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:84 > + if (sourceRect.width() > destBufferSize.width() || sourceRect.height() > destBufferSize.height()) This is not a right condition. It should be: sourceRect.maxX() + destBufferPosition.x() > destBufferSize.width() However, I think it's easy to understand to use IntRect::contains. if (!IntRect({ }, destBufferSize).contains(IntRect(destBufferPosition, sourceRect.size()))) return false; if (!IntRect({ }, sourceBufferSize).contains(sourceRect)) return false; > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:113 > + if (sourceRect.width() > destBufferSize.width() || sourceRect.height() > destBufferSize.height()) Ditto > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:431 > + copyRectFromSourceToDestAndSetPremultiplication<AlphaPremultiplication::Premultiplied>(sourceRect, sourceRect.size(), source.data(), backingStoreSize, data.data(), destBufferPosition); You specify sourceFormat instead of desiredFormat for the template argument.
Comment on attachment 379087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379087&action=review >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:84 >> + if (sourceRect.width() > destBufferSize.width() || sourceRect.height() > destBufferSize.height()) > > This is not a right condition. It should be: > sourceRect.maxX() + destBufferPosition.x() > destBufferSize.width() > > However, I think it's easy to understand to use IntRect::contains. > > if (!IntRect({ }, destBufferSize).contains(IntRect(destBufferPosition, sourceRect.size()))) > return false; > if (!IntRect({ }, sourceBufferSize).contains(sourceRect)) > return false; Yes, this looks better. Done! >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:113 >> + if (sourceRect.width() > destBufferSize.width() || sourceRect.height() > destBufferSize.height()) > > Ditto Okay. Also: I wish there was an easy way to consolidate these two templates. >> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:431 >> + copyRectFromSourceToDestAndSetPremultiplication<AlphaPremultiplication::Premultiplied>(sourceRect, sourceRect.size(), source.data(), backingStoreSize, data.data(), destBufferPosition); > > You specify sourceFormat instead of desiredFormat for the template argument. Oh, no! I mixed these two up. I'll fix.
Created attachment 379100 [details] Patch
Comment on attachment 379100 [details] Patch Looks good to me. There is a question in my mind. Converting strait alpha to premultiplied alpha is no problem, but lossy. However, Converting premultiplied alpha to strait alpha make me feel bad because it is lossy. But, it seems that Cairo port is also doing premultiplied alpha → strait alpha conversion.
Comment on attachment 379100 [details] Patch Clearing flags on attachment: 379100 Committed r250085: <https://trac.webkit.org/changeset/250085>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55506453>