Bug 89246 – [Cairo] do not multiply/demultiply colors when alpha is 255

• Component: Platform
• Status: RESOLVED FIXED

[arno.]

Hi,

currently, when computing color from premultiplied ARGB, and vice/versa (colorFromPremultipliedARGB and premultipliedARGBFromColor functions), the computation is performed if alpha is non 0. But if alpha is 255, computation is the identity computation. So we can avoid this computation in the case alpha is 255.

[arno.]

Created attachment 147901 [details]
patch proposal

[arno.]

Created attachment 148836 [details]
updated patch on top of 89138

[Simon Fraser (smfr)]

Is this a measurable perf win on a specific test?

[arno.]

(In reply to comment #3)
> Is this a measurable perf win on a specific test?

http://renevier.net/misc/webkit_89246.html

tested with GtkLauncher

getImageData goes from 275ms to 136ms when alpha is 255
             goes from 255ms to 261ms when alpha is 128
putImageData goes from 165ms to 82ms when alpha is 255
             goes from 160ms to 165ms when alpha is 128

So, we loose about 3% or 5% speed when there is transparency (may be because of the extra condition check?), but we are twice as fast when there is no transparency.

[Adam Barth]

Seems reasonable, but I don't know how to weigh 3-5% loss in the transparent case against the speed win in the opaque case.  It's likely someone other than me should review this patch.

[arno.]

CCing darin as he has reviewed patches in Color.cpp

[arno.]

Does someone known who could be a good reviewer for this patch ?

[Simon Fraser (smfr)]

(In reply to comment #5)
> Seems reasonable, but I don't know how to weigh 3-5% loss in the transparent case against the speed win in the opaque case.  It's likely someone other than me should review this patch.

I don't either. Some perf data from PLT or pages on the web might help.

[Darin Adler]

Comment on attachment 148836 [details]
updated patch on top of 89138

We should only add this code if the speedup is measurable.

[Darin Adler]

(In reply to comment #9)
> We should only add this code if the speedup is measurable.

Oh, I see, it was measured.

[Darin Adler]

Comment on attachment 148836 [details]
updated patch on top of 89138

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

> Source/WebCore/platform/graphics/Color.cpp:403
> -    if (unsigned alpha = (pixelColor & 0xFF000000) >> 24) {
> +    unsigned alpha = (pixelColor & 0xFF000000) >> 24;
> +    if (alpha && alpha != 255) {

Some compilers might make more efficient code if we write this kind of crazy code:

    unsigned char alpha = pixelColor >> 24;
    if (alpha + 1 < 2)

Might be worth testing if that’s faster.

> Source/WebCore/platform/graphics/Color.cpp:420
> -    if (unsigned alpha = color.alpha()) {
> +    unsigned alpha = color.alpha();
> +    if (alpha && alpha != 255) {

Similarly:

    unsigned char alpha = color.alpha();
    if (alpha + 1 < 2)

[Darin Adler]

Comment on attachment 148836 [details]
updated patch on top of 89138

If the code is used in such a tight loop, maybe these functions should be inlined, or done with vector operations on systems that support them.

[arno.]

(In reply to comment #11)
> (From update of attachment 148836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148836&action=review
> 
> > Source/WebCore/platform/graphics/Color.cpp:403
> > -    if (unsigned alpha = (pixelColor & 0xFF000000) >> 24) {
> > +    unsigned alpha = (pixelColor & 0xFF000000) >> 24;
> > +    if (alpha && alpha != 255) {
> 
> Some compilers might make more efficient code if we write this kind of crazy code:
> 
>     unsigned char alpha = pixelColor >> 24;
>     if (alpha + 1 < 2)
> 
> Might be worth testing if that’s faster.

Actually, it's much faster.
getImageData and putImageData take now 120ms and 85ms to execute with alpha 128. So, it's nearly twice faster.

It tried to set red(), green(), blue() and alpha() methods inline, but there was no performance change.

I didn't try yet to use vectors.

[arno.]

(In reply to comment #13)

> Actually, it's much faster.
> getImageData and putImageData take now 120ms and 85ms to execute with alpha 128. So, it's nearly twice faster.
> 

Sorry, I was totally mistaken. Actually, there is no performance win with that change.

[arno.]

Actually, I discovered that skipping the calls to premultipliedARGBFromColor/colorFromPremultipliedARGB, and performing the computation inside the loop is much faster. This optimization is already performed in webkitVideoSinkRender:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp?rev=124472#L212
This makes getImageData 10% faster, and putImageData 30% faster.

Then, if I don't perform the multiply/demultiply when alpha is 255, it's still 3%/5% slower with transparency. But with the 10%/30% previous win, overall, there is no case with performance loss.

[arno.]

Created attachment 156120 [details]
new patch proposal

[arno.]

With the new patch, on http://renevier.net/misc/webkit_89246.html

getImageData goes from 275ms to 62ms when alpha is 255
             goes from 255ms to 235ms when alpha is 128
putImageData goes from 165ms to 55ms when alpha is 255
             goes from 160ms to 115ms when alpha is 128

(tested with GtkLauncher)

[Kenneth Rohde Christiansen]

Comment on attachment 156120 [details]
new patch proposal

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

> Source/WebCore/ChangeLog:9
> +        Do not use colorFromPremultipliedARGB in getImageData nor
> +        premultipliedARGBFromColor in putByteArray. Avoiding object creation

Qt has a lot of such optimizations, worth have a look, though some might have moved into Qt itself

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:180
> +            // We don't use Color::colorFromPremultipliedARGB() here because
> +            // one function call per pixel is just too expensive:
> +            // On standard 200x400 canvas for example, 80000 calls each time.

// Avoid calling Color::colorFromPremultipliedARGB() per pixel for performance reasons.

[arno.]

Created attachment 170453 [details]
Patch

updated patch: better comment wording

[WebKit Review Bot]

Comment on attachment 170453 [details]
Patch

Rejecting attachment 170453 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ngeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [EFL][WK2] Get rid of Ewk_View private C API (Part 1)

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 154.

Full output: http://queues.webkit.org/results/14553357

[arno.]

(In reply to comment #20)
> (From update of attachment 170453 [details])
> Rejecting attachment 170453 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> ngeLog
> CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
> Failed to merge in the changes.
> Patch failed at 0001 [EFL][WK2] Get rid of Ewk_View private C API (Part 1)
> 
> When you have resolved this problem run "git rebase --continue".
> If you would prefer to skip this patch, instead run "git rebase --skip".
> To restore the original branch and stop rebasing run "git rebase --abort".
> 
> rebase refs/remotes/origin/master: command returned error: 1
> 
> Died at Tools/Scripts/update-webkit line 154.
> 
> Full output: http://queues.webkit.org/results/14553357

I don't really understand why it failed here.

[WebKit Review Bot]

Comment on attachment 170453 [details]
Patch

Clearing flags on attachment: 170453

Committed r132522: <http://trac.webkit.org/changeset/132522>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.