Bug 201862 – [FTW] Correct ImageBufferData and clear operations

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 378941 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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({ });  }

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 379021 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 379062 [details]
Patch

[Brent Fulgham]

Created attachment 379087 [details]
Patch

[Brent Fulgham]

(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.

[Fujii Hironori]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 379100 [details]
Patch

[Fujii Hironori]

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.

[WebKit Commit Bot]

Comment on attachment 379100 [details]
Patch

Clearing flags on attachment: 379100

Committed r250085: <https://trac.webkit.org/changeset/250085>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55506453>