Add heuristic for fixing the alpha channel when reading clipboard images on Windows

Add heuristic for fixing the alpha channel when reading clipboard images on Windows

The code attempts to detect an image with a garbage in the alpha channel and reset the alpha channel to a sane state in that case.

BUG=97160
TEST=Paste an image into a page using event.ClipboardData with the Windows Basic theme active.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102317

[dcheng, 2011-09-19 23:59:05]

It seems really sad that we have to do this. I cc'ed wez to see if he any
insight in another way to solve this issue or some other way of detecting bogus
alpha channels.

I tried using DIBV5, but it didn't make a difference and I couldn't figure out a
reliable way to differentiate between DIBs with a valid alpha channel and DIBs
with an invalid alpha channel.

[tony, 2011-09-20 00:52:21]

LGTM, but a couple questions:

What are the situations where we would have an alpha value?  For example, if you
were in an image editor and copied an image with alpha to the clipboard, does
that fall into this code path?  What happens in that case if we copy/paste an
image into e.g., a word processor?  E.g., does open office writer also drop the
alpha?

[dcheng, 2011-09-20 01:28:48]

On 2011/09/20 00:52:21, tony wrote:
> LGTM, but a couple questions:
> 
> What are the situations where we would have an alpha value?  For example, if
you
> were in an image editor and copied an image with alpha to the clipboard, does
> that fall into this code path?

Yes, an image that actually has a valid alpha channel would get wiped out.

> What happens in that case if we copy/paste an
> image into e.g., a word processor?  E.g., does open office writer also drop
the
> alpha?

Open Office is somehow differentiating. I'm going to investigate further I
guess.

[Wez, 2011-09-20 14:22:09]

On 2011/09/19 23:59:05, dcheng wrote:
> I tried using DIBV5, but it didn't make a difference and I couldn't figure out
a
> reliable way to differentiate between DIBs with a valid alpha channel and DIBs
> with an invalid alpha channel.

Can you elaborate on "didn't make a difference"?  My thinking on requesting a
DIBV5 is that the BITMAPV5HEADER structure includes details like the alpha
channel mask, if any.  You can use that information to pull the pixel data out
of the bitmap manually into the Skia buffer.

[dcheng, 2011-09-20 14:27:16]

On 2011/09/20 14:22:09, Wez wrote:
> On 2011/09/19 23:59:05, dcheng wrote:
> > I tried using DIBV5, but it didn't make a difference and I couldn't figure
out
> a
> > reliable way to differentiate between DIBs with a valid alpha channel and
DIBs
> > with an invalid alpha channel.
> 
> Can you elaborate on "didn't make a difference"?  My thinking on requesting a
> DIBV5 is that the BITMAPV5HEADER structure includes details like the alpha
> channel mask, if any.  You can use that information to pull the pixel data out
> of the bitmap manually into the Skia buffer.

The alpha mask is always 0, so there's no way of differentiating between the
two.

[dcheng, 2011-09-20 23:19:39]

Updated using the heuristic. It /seems/ to work.

[tony, 2011-09-20 23:26:54]

http://codereview.chromium.org/7967007/diff/6001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7967007/diff/6001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:496: break;
This only breaks out of the inner for loop.  You probably want to break out of
the outer loop too (or just make a helper function and early return).

[dcheng, 2011-09-20 23:56:33]

http://codereview.chromium.org/7967007/diff/6001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7967007/diff/6001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:496: break;
On 2011/09/20 23:26:54, tony wrote:
> This only breaks out of the inner for loop.  You probably want to break out of
> the outer loop too (or just make a helper function and early return).

Done. Thanks for the catch.

[tony, 2011-09-21 01:20:49]

LGTM

[dcheng, 2011-09-22 00:01:02]

Mind taking another look? The primary change is to always force <32 bpp images
to be opaque as the previous code did. I'm OOO today, but I'll test it before
landing.

[tony, 2011-09-22 01:26:58]

LGTM, you might also want to update the change description to be explicit about
when we make the image opaque.

http://codereview.chromium.org/7967007/diff/9001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7967007/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:433: static bool
BitmapHasInvalidPremultipliedColors(const SkBitmap& bitmap) {
Nit: Chromium style normally puts static functions in the anonymous namespace at
the top of the file.

[Wez, 2011-09-22 10:55:41]

http://codereview.chromium.org/7967007/diff/9001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7967007/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:500: // Windows doesn't really handle alpha
channels well in many situations. When
nit: Technically it's applications which handle DIBV5 image data on the
clipboard poorly.

http://codereview.chromium.org/7967007/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:502: // source image is 32 bpp, the alpha
channel might still contain garbage data.
Isn't it rather that some applications put images with alpha channels on the
clipboard but fail to set the alpha mask correctly in the bitmap header, so we
have to check for valid alpha channel if the mask is zero?