Bug 91897 - [Texmap] Performance regression in texture uploads after r121223
Summary: [Texmap] Performance regression in texture uploads after r121223
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-20 13:05 PDT by Noam Rosenthal
Modified: 2012-07-27 12:43 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.49 KB, patch)
2012-07-20 13:12 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (7.06 KB, patch)
2012-07-27 09:56 PDT, Noam Rosenthal
jturcotte: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!