Bug 93430 – incorrect transparent WebP image rendering

• Component: New Bugs
• Status: RESOLVED FIXED

[Pascal Massimino]

Hi,

this is mostly a heads up for now. The best way to fix is still open and debated.

Summary:

* libwebp tree was updated in chromium to 0.2.0 with alpha support:
     http://codereview.chromium.org/10832153/

* WEBPImageDecoder.cpp now needs an update to correct handle compressed image with alpha
  we're setting "buffer.setHasAlpha(false);" always, at line 122
  Rendering of webp-with-alpha is 'weird', as if values rgb values were not pre-multiplied.

* But even when setting "setHasAlpha(true)" (when there's alpha channel only), we need to
address the pre-multiplication of rgb values (cf m_premultiplyAlpha and line 79).

* libwebp can now emit premultiplied values, using MODE_bgrA instead of MODE_BGRA as outputMode()

i'll poke around the code further and try to get a patch, but any insight from Noel or others is welcome!

[Pascal Massimino]

Created attachment 157103 [details]
example webp file

Lossless-coded original WebP file.

[Pascal Massimino]

Created attachment 157106 [details]
expected decoded result

expected decoded result (decoded with 'dwebp' tool)

[Pascal Massimino]

Created attachment 157108 [details]
badly decoded result

screen capture from chrome 22.0.1229.0 dev
Looks like rgb values are not alpha-multiplied

[Pascal Massimino]

Created attachment 157474 [details]
Patch

[Adam Barth]

Comment on attachment 157474 [details]
Patch

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

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:59
> +    , m_decoder(0), m_hasAlpha(false)

This should be split on two lines.

[Pascal Massimino]

Created attachment 157491 [details]
Patch

[Pascal Massimino]

Created attachment 157678 [details]
Patch

[Pascal Massimino]

New patch with Layout test update and new files for decoding tests

[WebKit Review Bot]

Comment on attachment 157678 [details]
Patch

Attachment 157678 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13462770

New failing tests:
fast/images/webp-image-decoding.html

[WebKit Review Bot]

Created attachment 157692 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Pascal Massimino]

chromium-ews was unable to decode the new images and displayed the broken-image icon instead (with size 20x20 instead of expected 64x64). I'm not sure why the decoding is broken...

(In reply to comment #11)
> chromium-ews was unable to decode the new images and displayed the broken-image icon instead (with size 20x20 instead of expected 64x64). I'm not sure why the decoding is broken...

This same behavior will be seen if the resources (test2.webp, test3.webp) are missing. This works in a local build for me, not sure if it has something to do with how the chromium build applies the patch and whether those files are missing there, though. Is there a more detailed build log that shows the patch being applied?

[noel gordon]

(In reply to comment #12)
Needed a chromium DEPS roll.

[noel gordon]

(In reply to comment #0)

> i'll poke around the code further and try to get a patch, but any insight from Noel or others is welcome!

Noted this part ...

> * But even when setting "setHasAlpha(true)" (when there's alpha channel only), we need to
> address the pre-multiplication of rgb values (cf m_premultiplyAlpha and line 79).

and wondered if you intend to handle m_premultiplyAlpha for the alpha supporting formats?  If so, file a bug about it and move on with this patch which otherwise looks fine to me.

[Pascal Massimino]

Created attachment 158901 [details]
Patch

[Pascal Massimino]

(In reply to comment #15)
> Created an attachment (id=158901) [details]
> Patch

re-uploaded the patch. It's likely that the updated DEPS will make the chrome-ews test pass this time.

[noel gordon]

(In reply to comment #14)
> (In reply to comment #0)
> 
> > i'll poke around the code further and try to get a patch, but any insight from Noel or others is welcome!
> 
> Noted this part ...
> 
> > * But even when setting "setHasAlpha(true)" (when there's alpha channel only), we need to
> > address the pre-multiplication of rgb values (cf m_premultiplyAlpha and line 79).
> 
> and wondered if you intend to handle m_premultiplyAlpha for the alpha supporting formats?  If so, file a bug about it and move on with this patch which otherwise looks fine to me.

I should mention that browsers (moz, webkit, chrome) in general want a decoded row callback.  This to control the decoded frame buffer color correction, alpha pre-multiplication (m_premultiplyAlpha = true|false), and the numerical stability when the frame buffer is used with other web content (the <canvas> element in particular).  Perhaps a row callback is planned for a future version of libwebp.

[WebKit Review Bot]

Comment on attachment 158901 [details]
Patch

Clearing flags on attachment: 158901

Committed r125869: <http://trac.webkit.org/changeset/125869>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.