Bug 91897

Summary: [Texmap] Performance regression in texture uploads after r121223
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: New BugsAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: dongseong.hwang, hausmann, jturcotte, kenneth, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch jturcotte: review+

Description Noam Rosenthal 2012-07-20 13:05:57 PDT
[Texmap] Performance regression in texture uploads after r121223
Comment 1 Noam Rosenthal 2012-07-20 13:06:58 PDT
Since we're using GraphicsContext3D::extractImageData, we can't take advantage of desktop GL drivers that support the BGRA extension. This leads to much more time spent in texture uploads for Qt and GTK on desktop.
Comment 2 Noam Rosenthal 2012-07-20 13:12:34 PDT
Created attachment 153579 [details]
Patch
Comment 3 Kenneth Rohde Christiansen 2012-07-22 10:44:19 PDT
Comment on attachment 153579 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:155
> +const uint8_t* GraphicsContext3D::extractRawImageData(Image* image)

why not just rawImageData?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:490
>  #endif
>  
> +
> +
> +// Extracts the contents of the given Image without copying or formatting.

why so many newlines?
Comment 4 Dongseong Hwang 2012-07-22 18:17:11 PDT
I'm sorry for this regression and thank you for fixing it.
Comment 5 Noam Rosenthal 2012-07-27 07:16:32 PDT
Comment on attachment 153579 [details]
Patch

Need to rework this patch, it causes some color issues on GTK.
Comment 6 Noam Rosenthal 2012-07-27 09:56:31 PDT
Created attachment 154978 [details]
Patch
Comment 7 Noam Rosenthal 2012-07-27 11:23:17 PDT
Committed r123894: <http://trac.webkit.org/changeset/123894>
Comment 8 Jocelyn Turcotte 2012-07-27 12:42:10 PDT
This wasn't explained here, but the reason for reverting this patch is that it introduced an additional huge performance regression since tile updates from the web process are transferred through a 2000x2000 buffer and there is no way to tell extractImageData to BGRA->RGBA only the part of the buffer that we want.

So the whole 2000x2000 buffer is converted each time we need a tiny piece of a tile to be updated.

The way to fix it would be to add a partial image extraction capability to GraphicsContext3D, but this would require an implementation for every port. WebGL doesn't need it itself and this doesn't seems like it would be worth it.
Comment 9 Noam Rosenthal 2012-07-27 12:43:38 PDT
(In reply to comment #8)
> This wasn't explained here, but the reason for reverting this patch is that it introduced an additional huge performance regression since tile updates from the web process are transferred through a 2000x2000 buffer and there is no way to tell extractImageData to BGRA->RGBA only the part of the buffer that we want.
> 
> So the whole 2000x2000 buffer is converted each time we need a tiny piece of a tile to be updated.
> 
> The way to fix it would be to add a partial image extraction capability to GraphicsContext3D, but this would require an implementation for every port. WebGL doesn't need it itself and this doesn't seems like it would be worth it.

Ah, I didn't realize that was the source of the regression. Now it makes more sense!