Bug 201594 – [FTW] ImageBuffer/ImageBufferData is highly inefficient

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

My initial implementation of ImageBuffer and ImageBufferData for Direct2D involves too much moving of data from GPU to CPU and back. We only need to make a renderable version of the ImageBuffer when ImageBuffer::sinkIntoNativeImage or ImageBuffer::copyNativeImage are called.

Currently, each ImageBuffer putData operation uploads the data to the GPU, and each ImageBuffer getData pulls the data from the GPU.

This patch does the following:

1. It makes the assumption that the ID2D1Bitmap it holds is under its control (i.e., external draw operations do not manipulate the bitmap without marking it dirty).
2. It holds a CPU copy of the data from the ID2D1Bitmap originally used to create it. It uses this data for all manipulations, and uploads to the bitmap only when
   an ID2D1Bitmap is requested for drawing.
3. It does not read back from the ID2D1Bitmap unless it is told that it is dirty.

[Brent Fulgham]

Created attachment 378572 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 378572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378572&action=review

> Source/WebCore/ChangeLog:25
> +           they are costly, and don't help in release builds.

I think I am missing something here. The only place that copies the image data from the CPU memory to the GPU memory is ImageBufferData::compatibleBitmap() when imageTruth == ImageTruth::BitmapOutOfDate. The scenario I could not see clearly from this patch is if putData() is called and then any drawing function is called, I would expect to see loadDataToBitmap() called.

Does ImageBufferData::compatibleBitmap() get called before we call beginDraw()?

> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:410
> +#if !ASSERT_DISABLED
>      context->SetTags(1, __LINE__);
> +#endif

How about creating a function like this:

void PlatformContextDirect2D::SetTags(D2D1_TAG tag1, D2D1_TAG tag2)
{
#if !ASSERT_DISABLED
    renderTarget()->SetTags(tag1, tag2);
#else
    UNUSED_PARAM(tag1);
    UNUSED_PARAM(tag2);
#endif
}

And it can be called anywhere without #if !ASSERT_DISABLED.

> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:1034
> +    platformContext.notifyObserver();

With what we need to notifyObserver()? Can the name be more specific, notifyRenderTargetFlushed() or something similar?

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:82
> +void ImageBufferData::ensureBackingStore(const IntSize& size) const

I think it is better if this function returns a value, like bool for example, to indicate it succeeds or fails.

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:111
> +    auto bytesPerRowInData = rect.width() * 4;
> +    Checked<int> height = rect.height();
> +    Checked<int> width = rect.width();

Why do we check the overflow for width and height and we do not check the overflow for rect.width() * 4?

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:140
> +            IntSize bitmapSize = bitmap->GetPixelSize();
> +            ensureBackingStore(bitmapSize);

No need for the local variable and I think we should check for the return value and return if it fails:

if (!ensureBackingStore(bitmap->GetPixelSize()))
    return nullptr;

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:245
> +    ensureBackingStore(size);
> +
> +    ASSERT(data);

I think we should return if we fail to allocate data.

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:43
> +class ImageBufferData : public PlatformContextDirect2DObserver {

This now looks a weird class. All its members and methods are public but the destructor of the base is protected while the constructor of the base is private. Can't we fix the encapsulation of this class?

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:46
> +    virtual ~ImageBufferData() = default;

I think you do not need to define this destructor as long as you defined in the base class,

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:52
> -    Vector<char> data;
> +    mutable RefPtr<JSC::Uint8ClampedArray> data;

What is the reason for switching from Vector to Uint8ClampedArray?

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:59
> +    enum class ImageTruth { InSync, BitmapOutOfDate, BufferOutOfDate };
> +    mutable ImageTruth imageTruth { ImageTruth::BufferOutOfDate };

I think these can be made private.

Also can't we use symmetric names, for example:

    enum class BitmapBufferSync { InSync, BitmapOutOfSync, BufferOutOfSync };

> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:229
> -    if (copyBehavior == CopyBackingStore && m_data.data.isEmpty())
> +    if (copyBehavior == CopyBackingStore && !m_data.data->length())
>          copyBehavior = DontCopyBackingStore;

Why do we have DontCopyBackingStore here? It looks like we always return a null image for this copy behavior.

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.h:40
> +class ImageBufferData;

This forward declaration is not needed.

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.h:55
> +// Interface for notification about changes to a Bitmap rendering context (drawing)
> +class PlatformContextDirect2DObserver : public CanMakeWeakPtr<PlatformContextDirect2DObserver> {
> +protected:
> +    virtual ~PlatformContextDirect2DObserver() = default;
> +public:
> +    virtual void backingContextDidChange() = 0;
> +};

This class and the changes in ImageBufferData seem too much for just passing a callback. Do we plan to extend this interface for other uses? Otherwise I would suggest passing just a callback to this class instead:

class PlatformContextDirect2D {
public:
    PlatformContextDirect2D(ID2D1RenderTarget*, const WTF::Function<void()>& observer = []() { })
        : m_observer(observer)
    {
    }

private:
    const WTF::Function<void()>& observer  m_observer;
};

The observer callback can look like this:

ImageBuffer::ImageBuffer()
{
    m_data.platformContext = makeUnique<PlatformContextDirect2D>(bitmapContext.get(), ()[] {
        m_data.imageTruth = ImageTruth::BufferOutOfDate;
    });
}

[Brent Fulgham]

Comment on attachment 378572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378572&action=review

Thanks for those great review comments. I'll work on a revised version.

>> Source/WebCore/ChangeLog:25
>> +           they are costly, and don't help in release builds.
> 
> I think I am missing something here. The only place that copies the image data from the CPU memory to the GPU memory is ImageBufferData::compatibleBitmap() when imageTruth == ImageTruth::BitmapOutOfDate. The scenario I could not see clearly from this patch is if putData() is called and then any drawing function is called, I would expect to see loadDataToBitmap() called.
> 
> Does ImageBufferData::compatibleBitmap() get called before we call beginDraw()?

That's a good point, and probably a flaw in this patch. I may need to have PlatformGraphicsContext check its state before beginning drawing operations to trigger a load from CPU memory.

>> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:410
>> +#endif
> 
> How about creating a function like this:
> 
> void PlatformContextDirect2D::SetTags(D2D1_TAG tag1, D2D1_TAG tag2)
> {
> #if !ASSERT_DISABLED
>     renderTarget()->SetTags(tag1, tag2);
> #else
>     UNUSED_PARAM(tag1);
>     UNUSED_PARAM(tag2);
> #endif
> }
> 
> And it can be called anywhere without #if !ASSERT_DISABLED.

Good idea!

>> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:1034
>> +    platformContext.notifyObserver();
> 
> With what we need to notifyObserver()? Can the name be more specific, notifyRenderTargetFlushed() or something similar?

I think your idea about a lambda is much better. I'll do that.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:82
>> +void ImageBufferData::ensureBackingStore(const IntSize& size) const
> 
> I think it is better if this function returns a value, like bool for example, to indicate it succeeds or fails.

Okay.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:111
>> +    Checked<int> width = rect.width();
> 
> Why do we check the overflow for width and height and we do not check the overflow for rect.width() * 4?

Good point. I'll fix that.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:140
>> +            ensureBackingStore(bitmapSize);
> 
> No need for the local variable and I think we should check for the return value and return if it fails:
> 
> if (!ensureBackingStore(bitmap->GetPixelSize()))
>     return nullptr;

Will do.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:245
>> +    ASSERT(data);
> 
> I think we should return if we fail to allocate data.

Will do.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:52
>> +    mutable RefPtr<JSC::Uint8ClampedArray> data;
> 
> What is the reason for switching from Vector to Uint8ClampedArray?

I thought it might be easier to copy to/from the JS clamped array passed as an argument, but it's not clear there's any benefit. I'll try the vector again and make a new measurement.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:59
>> +    mutable ImageTruth imageTruth { ImageTruth::BufferOutOfDate };
> 
> I think these can be made private.
> 
> Also can't we use symmetric names, for example:
> 
>     enum class BitmapBufferSync { InSync, BitmapOutOfSync, BufferOutOfSync };

Good idea!

>> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:229
>>          copyBehavior = DontCopyBackingStore;
> 
> Why do we have DontCopyBackingStore here? It looks like we always return a null image for this copy behavior.

Yeah -- this is all messed up right now. I need to make sure this code is doing the right thing. I just haven't hit this in a valid case yet to see what the right behavior is.

>> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.h:40
>> +class ImageBufferData;
> 
> This forward declaration is not needed.

Will fix.

>> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.h:55
>> +};
> 
> This class and the changes in ImageBufferData seem too much for just passing a callback. Do we plan to extend this interface for other uses? Otherwise I would suggest passing just a callback to this class instead:
> 
> class PlatformContextDirect2D {
> public:
>     PlatformContextDirect2D(ID2D1RenderTarget*, const WTF::Function<void()>& observer = []() { })
>         : m_observer(observer)
>     {
>     }
> 
> private:
>     const WTF::Function<void()>& observer  m_observer;
> };
> 
> The observer callback can look like this:
> 
> ImageBuffer::ImageBuffer()
> {
>     m_data.platformContext = makeUnique<PlatformContextDirect2D>(bitmapContext.get(), ()[] {
>         m_data.imageTruth = ImageTruth::BufferOutOfDate;
>     });
> }

Yeah, that's a great idea. I'll follow that model.

[Brent Fulgham]

Comment on attachment 378572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378572&action=review

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:43
>> +class ImageBufferData : public PlatformContextDirect2DObserver {
> 
> This now looks a weird class. All its members and methods are public but the destructor of the base is protected while the constructor of the base is private. Can't we fix the encapsulation of this class?

I'll revise as you suggest.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.h:46
>> +    virtual ~ImageBufferData() = default;
> 
> I think you do not need to define this destructor as long as you defined in the base class,

Okay.

[Fujii Hironori]

Comment on attachment 378572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378572&action=review

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:118
> +        ASSERT(SUCCEEDED(hr));

You can use COMPtr's method instead of calling QueryInterface explicitly.

COMPtr<ID2D1DeviceContext> d2dDeviceContext(Query, platformContext->renderTarget());
ASSERT(!!d2dDeviceContext);

[Brent Fulgham]

Created attachment 378690 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 378690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378690&action=review

> Source/WebCore/ChangeLog:22
> +           an ID2D1Bitmap is requested for drawing.

... and before a drawing operation is performed on the associated ImageBuffer context (i.e. the other calls to notifyPreDrawObserver).

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:285
> +    if (bitmapBufferSync == BitmapBufferSync::BitmapOutOfSync && !data.isEmpty())

Should this if-statement be moved to loadDataToBitmap()?

> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:119
> +        if (m_data.bitmapBufferSync == ImageBufferData::BitmapBufferSync::BitmapOutOfSync && !m_data.data.isEmpty())

Ditto.

> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:122
> +        m_data.bitmapBufferSync = ImageBufferData::BitmapBufferSync::BufferOutOfSync;

Should this be moved to a function in ImageBufferData for better reading, markBufferOutOfSync() for example?

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:278
> +    if (m_preDrawHandler)
> +        m_preDrawHandler();

What is the meaning of this if statement? m_preDrawHandler is not a pointer and it does not return a value. I am wondering what the complier translates it to. But I think you just need to call m_preDrawHandler() without the if statement.

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:284
> +    if (m_postDrawHandler)
> +        m_postDrawHandler();

Ditto.

[Brent Fulgham]

Comment on attachment 378690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378690&action=review

Thank you for reviewing, Said!

>> Source/WebCore/ChangeLog:22
>> +           an ID2D1Bitmap is requested for drawing.
> 
> ... and before a drawing operation is performed on the associated ImageBuffer context (i.e. the other calls to notifyPreDrawObserver).

Good point!

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:285
>> +    if (bitmapBufferSync == BitmapBufferSync::BitmapOutOfSync && !data.isEmpty())
> 
> Should this if-statement be moved to loadDataToBitmap()?

Yes -- I think so, and I should change the method name to "loadDataToBitmapIfNeeded()"

>> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:122
>> +        m_data.bitmapBufferSync = ImageBufferData::BitmapBufferSync::BufferOutOfSync;
> 
> Should this be moved to a function in ImageBufferData for better reading, markBufferOutOfSync() for example?

Okay.

>> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:278
>> +        m_preDrawHandler();
> 
> What is the meaning of this if statement? m_preDrawHandler is not a pointer and it does not return a value. I am wondering what the complier translates it to. But I think you just need to call m_preDrawHandler() without the if statement.

Oh, right. We pass an empty lambda, not a nullptr. I'll fix that.

[Brent Fulgham]

Created attachment 378734 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 378734 [details]
Patch for landing

Clearing flags on attachment: 378734

Committed r249839: <https://trac.webkit.org/changeset/249839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55343175>