Fixes for decoding to A8.

Fixes for decoding to A8.

src/images/SkImageDecoder_libpng.cpp:
A8 images are not opaque, so do not set the opaque flag.
This fixes a bug where copyTo does not work as expected (when
copying an A8 decoded image to ARGB_8888), leading to
a bitmap hash that does not represent the image correctly
(in skimage).

tools/skimage_main.cpp:
In write_bitmap, which is creating the image for visual comparison,
copy A8 to 8888, since A8 cannot be encoded.

In the section that tests reencoding, do not test reencoding A8,
which is known to not work.

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

[scroggo, 2013-10-03 16:50:38]

I had assigned to Mike, but he's out of office. Derek, can you take a look?

[djsollen, 2013-10-03 16:54:28]

lgtm

https://codereview.chromium.org/25726004/diff/4001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:118: // support A8.
is that a feature that we should support?

[scroggo, 2013-10-03 16:58:52]

https://codereview.chromium.org/25726004/diff/4001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:118: // support A8.
On 2013/10/03 16:54:28, djsollen wrote:
> is that a feature that we should support?

Possibly, although I'd set it at a low priority. Not sure if anyone wants the
feature.

[commit-bot: I haz the power, 2013-10-03 17:04:24]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/scroggo@google.com/25726004/4001

[commit-bot: I haz the power, 2013-10-03 17:13:44]

Change committed as 11589

[reed1, 2013-10-04 08:56:27]

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libpng.cpp:439: reallyHasAlpha = true;
I thought this variable was set when we actually saw a non-opaque value. If that
is true, why do we need to force it to true here? What if the alpha-only PNG was
in fact 100% opaque?

https://codereview.chromium.org/25726004/diff/4001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:118: // support A8.
On 2013/10/03 16:58:52, scroggo wrote:
> On 2013/10/03 16:54:28, djsollen wrote:
> > is that a feature that we should support?
> 
> Possibly, although I'd set it at a low priority. Not sure if anyone wants the
> feature.

Perhaps the code at this level shouldn't know if its supported or not. Why don't
the encoders report (a) what they do support, or (b) failure if they didn't
support a given config. That way this tool could "try" A8, and if it failed,
fallback to 32bit.

[scroggo, 2013-10-04 19:45:53]

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libpng.cpp:439: reallyHasAlpha = true;
On 2013/10/04 08:56:27, reed1 wrote:
> I thought this variable was set when we actually saw a non-opaque value. If
that
> is true, why do we need to force it to true here? What if the alpha-only PNG
was
> in fact 100% opaque?

Setting the image to be opaque is an optimization for drawing fast. An alpha
only PNG that is 100% opaque is not very interesting, and I suspect not used
much. Right now we don't know whether the image has alpha for certain, and we'd
have to look at each pixel to determine the answer (we could update the sampler
to check, but in the fast case of skipping the sampler, we don't look at the
pixels directly). I don't think it makes sense to add the opaque optimization
for the rare case while penalizing the common case.

https://codereview.chromium.org/25726004/diff/4001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:118: // support A8.
On 2013/10/04 08:56:27, reed1 wrote:
> On 2013/10/03 16:58:52, scroggo wrote:
> > On 2013/10/03 16:54:28, djsollen wrote:
> > > is that a feature that we should support?
> > 
> > Possibly, although I'd set it at a low priority. Not sure if anyone wants
the
> > feature.
> 
> Perhaps the code at this level shouldn't know if its supported or not. Why
don't
> the encoders report (a) what they do support, or (b) failure if they didn't
> support a given config. That way this tool could "try" A8, and if it failed,
> fallback to 32bit.

They do report failure. I have a CL at https://codereview.chromium.org/25964003
that uses a fallback.

[reed1, 2013-10-07 13:31:39]

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libpng.cpp:439: reallyHasAlpha = true;
On 2013/10/04 19:45:53, scroggo wrote:
> On 2013/10/04 08:56:27, reed1 wrote:
> > I thought this variable was set when we actually saw a non-opaque value. If
> that
> > is true, why do we need to force it to true here? What if the alpha-only PNG
> was
> > in fact 100% opaque?
> 
> Setting the image to be opaque is an optimization for drawing fast. An alpha
> only PNG that is 100% opaque is not very interesting, and I suspect not used
> much. Right now we don't know whether the image has alpha for certain, and
we'd
> have to look at each pixel to determine the answer (we could update the
sampler
> to check, but in the fast case of skipping the sampler, we don't look at the
> pixels directly). I don't think it makes sense to add the opaque optimization
> for the rare case while penalizing the common case.

Ah, I think I see. You're saying we don't always perform the check for
reallyHasAlpha (I thought we did). But now I see around line 350 we seem to not
inspect the values. If that is the case, I suggest we a the line

reallyHasAlpha = true;

... in that spot, with a comment to the effect of // likely we do have non-0xFF,
but faster not to check...
rather than down here.

[scroggo, 2013-10-07 22:10:29]

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libpng.cpp:439: reallyHasAlpha = true;
On 2013/10/07 13:31:39, reed1 wrote:
> On 2013/10/04 19:45:53, scroggo wrote:
> > On 2013/10/04 08:56:27, reed1 wrote:
> > > I thought this variable was set when we actually saw a non-opaque value.
If
> > that
> > > is true, why do we need to force it to true here? What if the alpha-only
PNG
> > was
> > > in fact 100% opaque?
> > 
> > Setting the image to be opaque is an optimization for drawing fast. An alpha
> > only PNG that is 100% opaque is not very interesting, and I suspect not used
> > much. Right now we don't know whether the image has alpha for certain, and
> we'd
> > have to look at each pixel to determine the answer (we could update the
> sampler
> > to check, but in the fast case of skipping the sampler, we don't look at the
> > pixels directly). I don't think it makes sense to add the opaque
optimization
> > for the rare case while penalizing the common case.
> 
> Ah, I think I see. You're saying we don't always perform the check for
> reallyHasAlpha (I thought we did). But now I see around line 350 we seem to
not
> inspect the values. If that is the case, I suggest we a the line
> 
> reallyHasAlpha = true;
> 
> ... in that spot, with a comment to the effect of // likely we do have
non-0xFF,
> but faster not to check...
> rather than down here.

Sounds good to me. Uploaded https://codereview.chromium.org/26210007/, which
includes fixes for other bugs I found while making these changes.

[reed1, 2013-10-08 16:25:57]

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25726004/diff/4001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libpng.cpp:438: if (SkBitmap::kA8_Config ==
decodedBitmap->config()) {
Shouldn't this block be done before line 431?