Bug 201122 – [FTW] Go back to ID2D1Bitmap as our NativeImage type

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

I switched to IWICBitmap as the OS base type representing bitmap images in Bug 200093. However, this was an ill-advised approach, because it dramatically harmed performance due to the heavy use of software rendering.

I originally made this change because I thought this was the only way to get to the backing bits of the bitmaps, but it turns out that a more recent Direct2D data type (ID2D1Bitmap1) has the ability to map its memory to CPU-accessible memory, allowing software filter effects.

This patch switches back to the ID2D1Bitap data type, and hooks up the ID2D1Bitmap1 data type to access the underlying memory of the bitmaps when software filter effects are used.

[Sam Weinig]

What does [FTW] mean in the context of this bug? It's not a WebKit related acronym I am familiar with.

[Brent Fulgham]

(In reply to Sam Weinig from comment #1)
> What does [FTW] mean in the context of this bug? It's not a WebKit related
> acronym I am familiar with.

FTW stands for "For The Win(dows)", which is a join effort of Sony and Apple to create a single unified Windows port.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #2)
> (In reply to Sam Weinig from comment #1)
> > What does [FTW] mean in the context of this bug? It's not a WebKit related
> > acronym I am familiar with.
> 
> FTW stands for "For The Win(dows)", which is a join effort of Sony and Apple
> to create a single unified Windows port.

s/join/joint/

[Brent Fulgham]

Created attachment 377217 [details]
Patch

[Brent Fulgham]

Created attachment 377218 [details]
Patch

[Brent Fulgham]

To give a rough idea of performance, I ran MotionMark 1.1 on our three Windows variants and got the following results:

AppleWin                  4.35
WinCairo                  8.39
FTW                      28.71

I think this show poorly for FTW, since Focus and Suits are hitting software paths heavily. Fixing those two code paths should significantly improve things.

I have achieved scores of around 200 when bypassing the non-accelerated MotionMark tests, so I think we have significant room for improvement.

Still, even in its current form FTW is a huge improvement over existing options.

[Fujii Hironori]

Here is the result on my PC.

WinCairo WK2 Direct2D: 163.03
WinCairo WK2 Cairo: 136.66

https://ibb.co/XZ7kSSS
https://ibb.co/vvthmkp

AMD Ryzen 7 1700 Eight-Core Processor
NVIDIA GeForce GTX 1050

[Brent Fulgham]

(In reply to Brent Fulgham from comment #6)
> To give a rough idea of performance, I ran MotionMark 1.1 on our three
> Windows variants and got the following results:
> 
> AppleWin                  4.35
> WinCairo                  8.39
> FTW                      28.71
> 

This was measured on an old Mac Pro, running a current Windows 10 Pro (1803, patch level 17134.885)

Intel Xeon CPU (E5645 @ 2.4 GHZ)
AMD Radeon HD 5700

[Brent Fulgham]

(In reply to Fujii Hironori from comment #7)
> Here is the result on my PC.
> 
> WinCairo WK2 Direct2D: 163.03
> WinCairo WK2 Cairo: 136.66
> 
> https://ibb.co/XZ7kSSS
> https://ibb.co/vvthmkp
> 
> AMD Ryzen 7 1700 Eight-Core Processor
> NVIDIA GeForce GTX 1050

Thank you for running those metrics, Fujii. Clearly Direct2D is a larger improvement on older hardware. I think if we can tune the workflow better we will see further improvements. At least on my system I think I am starving the GPU pipeline from time to time, but I am not experienced enough in Windows tools to figure out what to fix.

[Alex Christensen]

Comment on attachment 377218 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:166
> +    ASSERT_NOT_IMPLEMENTED_YET();

This is a poorly named macro.

> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:837
> +    COMPtr<ID2D1Bitmap> tileImage = sourceImage;

What if we passed a ComPtr&& as a parameter instead of a raw pointer?

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:49
> +void swizzleAndPremultiply(const uint8_t* srcRows, unsigned rowCount, unsigned colCount, unsigned srcStride, unsigned destStride, uint8_t* destRows)

Is this a hot path?  It might benefit from some manual SIMD with intrinsics someday.

> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:140
> +void inPlaceSwizzle(uint8_t* byteData, unsigned byteCount)

ditto

> Source/WebCore/svg/graphics/SVGImage.cpp:257
> +#ifndef _NDEBUG

#if !ASSERT_DISABLED

[Brent Fulgham]

Comment on attachment 377218 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:166
>> +    ASSERT_NOT_IMPLEMENTED_YET();
> 
> This is a poorly named macro.

I should just use the 'notImplemented()' function.

>> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:837
>> +    COMPtr<ID2D1Bitmap> tileImage = sourceImage;
> 
> What if we passed a ComPtr&& as a parameter instead of a raw pointer?

Good idea! Done.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:49
>> +void swizzleAndPremultiply(const uint8_t* srcRows, unsigned rowCount, unsigned colCount, unsigned srcStride, unsigned destStride, uint8_t* destRows)
> 
> Is this a hot path?  It might benefit from some manual SIMD with intrinsics someday.

Probably -- there's a lot of tuning needed, but this would be a good place to make changes.

>> Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:140
>> +void inPlaceSwizzle(uint8_t* byteData, unsigned byteCount)
> 
> ditto

Ditto! :-)

>> Source/WebCore/svg/graphics/SVGImage.cpp:257
>> +#ifndef _NDEBUG
> 
> #if !ASSERT_DISABLED

Done!

[Brent Fulgham]

Created attachment 377255 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 377255 [details]
Patch for landing

Clearing flags on attachment: 377255

Committed r249110: <https://trac.webkit.org/changeset/249110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54716793>

[Said Abou-Hallawa]

I am confused by this change. Does this 

So I was expecting to see r247841 is rolled out first. And then another patch is (In reply to WebKit Commit Bot from comment #13)
> Comment on attachment 377255 [details]
> Patch for landing
> 
> Clearing flags on attachment: 377255
> 
> Committed r249110: <https://trac.webkit.org/changeset/249110>

Does this change roll out r247841? Or does it roll some of it, keep the rest of it and even add new code?

If the answer is the later, I think this is confusing and will make thing harder to track. My understanding is when a revision causes a regression, the first we should is to roll out the whole change.

[Brent Fulgham]

(In reply to Said Abou-Hallawa from comment #16)
> Does this change roll out r247841? Or does it roll some of it, keep the rest
> of it and even add new code?

r247841 could no longer be cleanly rolled out, because the code had been refactored.

This change keeps the refactored code (including Webkit2 support) but switches the base NativeImagePtr type from IWICBitmap back to ID2D1Bitmap.