Image decoder fixes (mostly) around A8.

Image decoder fixes (mostly) around A8.

Opaquness:
When decoding Gray to A8 in libpng, set reallyHasAlpha to true
and add a comment why we do not check for the real answer.

Add comments in jpeg decoder explaining why A8 is not opaque.

Fix a bug where an A8 subset is considered to be opaque.

Other fixes:
In SkJPEGImageDecoder, only allocate as much memory as
needed for each source row, based on the input config.
Also pull out common code into a static function.

When performing the check for requiring unpremultiplied colors,
allow A8 to succeed, since that setting should have no effect on
A8.

Add the check for requiring unpremultiplied colors to subset
decoding.

Fix a bug where attempting to sample gray to A8 does not sample.

R=reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=11897

[reed1, 2013-10-08 16:24:17]

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libjpeg.cpp:565: // Assume an A8 bitmap is opaque to
avoid the check of each
"is not" ?

same question for all copies of this comment block

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:949: // The exception is A8, where there is
nothing to premultiply.
What is the config that 
- has alpha
- has premul color components
- is not 8888
- cannot support unpremul
?

[scroggo, 2013-10-10 21:46:59]

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libjpeg.cpp:565: // Assume an A8 bitmap is opaque to
avoid the check of each
On 2013/10/08 16:24:17, reed1 wrote:
> "is not" ?
> 
> same question for all copies of this comment block

Done.

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:949: // The exception is A8, where there is
nothing to premultiply.
On 2013/10/08 16:24:17, reed1 wrote:
> What is the config that 
> - has alpha
> - has premul color components
> - is not 8888
> - cannot support unpremul
> ?

4444 and Index8. Both of them COULD support unpremul (and in fact for Index8 we
have actually skipped premultiplication), but we had decided to return false and
limit unpremul to 8888.

[reed1, 2013-10-14 13:26:05]

requesting some reorg to make the fact that this block of checks involves both
capabilities and current-policy decisions.

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:949: // The exception is A8, where there is
nothing to premultiply.
On 2013/10/10 21:46:59, scroggo wrote:
> On 2013/10/08 16:24:17, reed1 wrote:
> > What is the config that 
> > - has alpha
> > - has premul color components
> > - is not 8888
> > - cannot support unpremul
> > ?
> 
> 4444 and Index8. Both of them COULD support unpremul (and in fact for Index8
we
> have actually skipped premultiplication), but we had decided to return false
and
> limit unpremul to 8888.

Thanks for the dox... I think the code is not as clear. Is it correct to write
the code as:

if (hasAlpha && this->requireUnpremul()) {
    switch (config) {
        ....
        return false;   // we have chosen to not support premul for these.
    }
}

[scroggo, 2013-10-14 17:15:19]

New patch.

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/26210007/diff/17001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:949: // The exception is A8, where there is
nothing to premultiply.
On 2013/10/14 13:26:05, reed1 wrote:
> On 2013/10/10 21:46:59, scroggo wrote:
> > On 2013/10/08 16:24:17, reed1 wrote:
> > > What is the config that 
> > > - has alpha
> > > - has premul color components
> > > - is not 8888
> > > - cannot support unpremul
> > > ?
> > 
> > 4444 and Index8. Both of them COULD support unpremul (and in fact for Index8
> we
> > have actually skipped premultiplication), but we had decided to return false
> and
> > limit unpremul to 8888.
> 
> Thanks for the dox... I think the code is not as clear. Is it correct to write
> the code as:
> 
> if (hasAlpha && this->requireUnpremul()) {
>     switch (config) {
>         ....
>         return false;   // we have chosen to not support premul for these.
>     }
> }
> 

Yes. Latest code follows this pattern.

[reed1, 2013-10-14 17:37:12]

THANK YOU -- very clear to my old eyes

lgtm

[scroggo, 2013-10-21 20:47:39]

Committed patchset #8 manually as r11897 (presubmit successful).