DM: fix failures when using -r by comparing unpremultiplied.

DM: fix failures when using -r by comparing unpremultiplied.

PNGs store unpremultiplied colors, so we have to convert back and forth with
SkBitmap.  This is lossy.  GM solves this problem by stripping the alpha
channel before writing the PNG.

This flips it around, converting the GM's output to unpremultiplied as needed.  This way each pixel goes from premul to unpremul once, never back.

Tested: 
out/Release/dm -w /tmp/w --config 565 8888 gpu
out/Release/dm -r /tmp/w --config 565 8888 gpu
BUG=

Committed: http://code.google.com/p/skia/source/detail?r=12926

[bsalomon, 2014-01-06 14:22:06]

On 2014/01/04 04:27:07, mtklein wrote:

Hmm... I think this may not work when comparing PNGs generated across OS's or
skia revisions. The PNG encoder is part of the skia porting layer and so subject
to change. Maybe that's OK for DM (but wouldn't be for sk*diff). Maybe this
should be a flag? Or should we choose a different encoding that supports premul?

[mtklein, 2014-01-06 15:48:28]

On 2014/01/06 14:22:06, bsalomon wrote:
> On 2014/01/04 04:27:07, mtklein wrote:
> 
> Hmm... I think this may not work when comparing PNGs generated across OS's or
> skia revisions. The PNG encoder is part of the skia porting layer and so
subject
> to change. Maybe that's OK for DM (but wouldn't be for sk*diff). Maybe this
> should be a flag? Or should we choose a different encoding that supports
premul?

Follow up to our conversation where I was going to look at WebP:

As far as I can tell, even at the tip of tree, libwebp doesn't support
premultiplied output.  It seems to even require that its inputs are
unpremultiplied.  It will, however, do the premultiplication for you when
decoding if you ask it.

I'd like to submit this, then later if I get our WebP encoder up to parity with
PNG (not stripping alpha, supporting lossless; I'm close modulo bugs), we could
probably switch to this same new strategy but using WebP.  We have a gyp setting
use_system_libwebp but no one is using it, so if it stays that way, we should be
more able to rely on byte-identical encodings across platforms.

[mtklein, 2014-01-06 15:54:07]

On 2014/01/06 15:48:28, mtklein wrote:
> On 2014/01/06 14:22:06, bsalomon wrote:
> > On 2014/01/04 04:27:07, mtklein wrote:
> > 
> > Hmm... I think this may not work when comparing PNGs generated across OS's
or
> > skia revisions. The PNG encoder is part of the skia porting layer and so
> subject
> > to change. Maybe that's OK for DM (but wouldn't be for sk*diff). Maybe this
> > should be a flag? Or should we choose a different encoding that supports
> premul?
> 
> Follow up to our conversation where I was going to look at WebP:
> 
> As far as I can tell, even at the tip of tree, libwebp doesn't support
> premultiplied output.  It seems to even require that its inputs are
> unpremultiplied.  It will, however, do the premultiplication for you when
> decoding if you ask it.
> 
> I'd like to submit this, then later if I get our WebP encoder up to parity
with
> PNG (not stripping alpha, supporting lossless; I'm close modulo bugs), we
could
> probably switch to this same new strategy but using WebP.  We have a gyp
setting
> use_system_libwebp but no one is using it, so if it stays that way, we should
be
> more able to rely on byte-identical encodings across platforms.

Actually, better idea.

We keep PNG, but read it back into an unpremultiplied SkBitmap.  To check the
GM, we convert its output to unpremultiplied and then compare the two SkBitmaps.
 As long the two unpremultiply steps are the same (one when writing the PNG, one
when checking the GM output), this will be platform independent.  There will
just be two identical premul->unpremul steps, each only half of our current
lossy round trip.

[epoger, 2014-01-06 17:13:27]

Mike-

As you've noted, GM strips the alpha channel before writing the PNG.  So it
throws away data to sidestep a problem... not the greatest solution.

But GM also uses SkBitmapHasher to compare expected and actual bitmaps, which I
think avoids this same problem.  SkBitmapHasher performs its hash on the
premultiplied RGB values held by the SkBitmap, before GM writes out the PNG, and
this hash is written out as the actual result of the test.  The PNG file is only
used as a visual reference when accepting those actual bitmap hashes into the
expectations file.

In other words, GM's SkBitmapHash value is captured before the premul->unpremul
conversion.

Food for thought...

And, in a broader view, I certainly hope we can unite GM and DM into a single
thing.  But that's probably a longer conversation. :-)

[mtklein, 2014-01-06 17:17:45]

On 2014/01/06 17:13:27, epoger wrote:
> Mike-
> 
> As you've noted, GM strips the alpha channel before writing the PNG.  So it
> throws away data to sidestep a problem... not the greatest solution.
> 
> But GM also uses SkBitmapHasher to compare expected and actual bitmaps, which
I
> think avoids this same problem.  SkBitmapHasher performs its hash on the
> premultiplied RGB values held by the SkBitmap, before GM writes out the PNG,
and
> this hash is written out as the actual result of the test.  The PNG file is
only
> used as a visual reference when accepting those actual bitmap hashes into the
> expectations file.
> 
> In other words, GM's SkBitmapHash value is captured before the
premul->unpremul
> conversion.
> 
> Food for thought...
> 
> And, in a broader view, I certainly hope we can unite GM and DM into a single
> thing.  But that's probably a longer conversation. :-)

This problem happens in GM only when using IndividualImagesExpectataionsSource,
which reads images as expectations not hashes.  We're fixing the equivalent mode
here.  DM uses the same hashes as GM when it's looking at a JSON file.

[mtklein, 2014-01-06 17:24:03]

On 2014/01/06 17:17:45, mtklein wrote:
> On 2014/01/06 17:13:27, epoger wrote:
> > Mike-
> > 
> > As you've noted, GM strips the alpha channel before writing the PNG.  So it
> > throws away data to sidestep a problem... not the greatest solution.
> > 
> > But GM also uses SkBitmapHasher to compare expected and actual bitmaps,
which
> I
> > think avoids this same problem.  SkBitmapHasher performs its hash on the
> > premultiplied RGB values held by the SkBitmap, before GM writes out the PNG,
> and
> > this hash is written out as the actual result of the test.  The PNG file is
> only
> > used as a visual reference when accepting those actual bitmap hashes into
the
> > expectations file.
> > 
> > In other words, GM's SkBitmapHash value is captured before the
> premul->unpremul
> > conversion.
> > 
> > Food for thought...
> > 
> > And, in a broader view, I certainly hope we can unite GM and DM into a
single
> > thing.  But that's probably a longer conversation. :-)
> 
> This problem happens in GM only when using
IndividualImagesExpectataionsSource,
> which reads images as expectations not hashes.  We're fixing the equivalent
mode
> here.  DM uses the same hashes as GM when it's looking at a JSON file.

Brian, ready for another look at PS 2.

[epoger, 2014-01-06 17:24:18]

On 2014/01/06 17:17:45, mtklein wrote:
> This problem happens in GM only when using
IndividualImagesExpectataionsSource,
> which reads images as expectations not hashes.  We're fixing the equivalent
mode
> here.  DM uses the same hashes as GM when it's looking at a JSON file.

Makes sense, thanks for the reminder.

[bsalomon, 2014-01-06 18:46:43]

https://codereview.chromium.org/122923003/diff/100001/dm/DMWriteTask.cpp
File dm/DMWriteTask.cpp (right):

https://codereview.chromium.org/122923003/diff/100001/dm/DMWriteTask.cpp#newc...
dm/DMWriteTask.cpp:134: if (bitmap.config() == SkBitmap::kARGB_8888_Config) {
Can we look at the bitmap alpha type rather than config?

[mtklein, 2014-01-06 18:54:06]

On 2014/01/06 18:46:43, bsalomon wrote:
> https://codereview.chromium.org/122923003/diff/100001/dm/DMWriteTask.cpp
> File dm/DMWriteTask.cpp (right):
> 
>
https://codereview.chromium.org/122923003/diff/100001/dm/DMWriteTask.cpp#newc...
> dm/DMWriteTask.cpp:134: if (bitmap.config() == SkBitmap::kARGB_8888_Config) {
> Can we look at the bitmap alpha type rather than config?

Well, we need both.  We don't need to do this for alpha-only formats like A8. 
As written, the code only works for bitmaps containing SkPMColor, and that's in
practice all we're going to shove into there, so it seems clearest to make that
check explicit.  We could make it work for other premultiplied formats, but I
think the only one left is 4444, and why bother there?

[mtklein, 2014-01-06 19:07:02]

On 2014/01/06 18:54:06, mtklein wrote:
> On 2014/01/06 18:46:43, bsalomon wrote:
> > https://codereview.chromium.org/122923003/diff/100001/dm/DMWriteTask.cpp
> > File dm/DMWriteTask.cpp (right):
> > 
> >
>
https://codereview.chromium.org/122923003/diff/100001/dm/DMWriteTask.cpp#newc...
> > dm/DMWriteTask.cpp:134: if (bitmap.config() == SkBitmap::kARGB_8888_Config)
{
> > Can we look at the bitmap alpha type rather than config?
> 
> Well, we need both.  We don't need to do this for alpha-only formats like A8. 
> As written, the code only works for bitmaps containing SkPMColor, and that's
in
> practice all we're going to shove into there, so it seems clearest to make
that
> check explicit.  We could make it work for other premultiplied formats, but I
> think the only one left is 4444, and why bother there?

To Mike, who asked to use SkColorType off thread: like this?  Works for me.

[bsalomon, 2014-01-06 19:08:18]

On 2014/01/06 19:07:02, mtklein wrote:
> On 2014/01/06 18:54:06, mtklein wrote:
> > On 2014/01/06 18:46:43, bsalomon wrote:
> > > https://codereview.chromium.org/122923003/diff/100001/dm/DMWriteTask.cpp
> > > File dm/DMWriteTask.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/122923003/diff/100001/dm/DMWriteTask.cpp#newc...
> > > dm/DMWriteTask.cpp:134: if (bitmap.config() ==
SkBitmap::kARGB_8888_Config)
> {
> > > Can we look at the bitmap alpha type rather than config?
> > 
> > Well, we need both.  We don't need to do this for alpha-only formats like
A8. 
> > As written, the code only works for bitmaps containing SkPMColor, and that's
> in
> > practice all we're going to shove into there, so it seems clearest to make
> that
> > check explicit.  We could make it work for other premultiplied formats, but
I
> > think the only one left is 4444, and why bother there?
> 
> To Mike, who asked to use SkColorType off thread: like this?  Works for me.

lgtm

[commit-bot: I haz the power, 2014-01-06 19:39:56]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/122923003/160001

[commit-bot: I haz the power, 2014-01-06 20:24:23]

Change committed as 12926