Add reallyHasAlpha() to SkAndroidCodec

Add reallyHasAlpha() to SkAndroidCodec

This also modified the default implementation
of onReallyHasAlpha() in SkCodec.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/90c4d5fec8a9455729b401fd7818bea221dcada2

[msarett, 2015-12-10 16:48:38]

Description was changed from

==========
Add reallyHasAlpha() to SkAndroidCodec

BUG=skia:
==========

to

==========
Add reallyHasAlpha() to SkAndroidCodec

This also modified the default implementation
of onReallyHasAlpha() in SkCodec.

BUG=skia:
==========

[msarett, 2015-12-10 16:50:14]

msarett@google.com changed reviewers:
+ djsollen@google.com, scroggo@google.com

[msarett, 2015-12-10 16:50:14]

We need this to pass NinePatchTest::testHasAlpha() in CTS.

[scroggo, 2015-12-10 17:02:32]

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkAndroidCodec.h
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkAndroidCode...
include/codec/SkAndroidCodec.h:219: bool reallyHasAlpha() const {
With some changes, this doesn't even need to be virtual.

It seems that both subclasses (SkSampledCodec and SkWebpAdapterCodec) have an
fCodec member. We could move that into this class (like we did with fStream) and
then call that directly.

Combining with an idea we discussed in person: SkAndroidCodec needs a lot less
duplicated virtuals if we instead provide an accessor for the codec:

public class SkAndroidCodec{
public:
  ...
  // I'm not tied to this name...
  const SkCodec* actualCodec() const { return fCodec.get(); }
protected:
  SkCodec* codec() { return fCodec.get(); }
private:
  SkAutoTDelete<SkCodec> fCodec;
}

Then the calling class can call these const methods directly:

SkCodec* codec = androidCodec.actualCodec();
codec->getEncodedFormat()
codec->reallyHasAlpha()
...

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:494: return kOpaque_SkAlphaType !=
this->getInfo().alphaType();
SkWebpCodec's implementation looks like this (and maybe others? I have not
checked). Should we stop them from overriding to do the same thing?

I wonder if it could be written like the following:

bool reallyHasAlpha() const {
  return this->getInfo().alphaType() != kOpaque && this->onReallyHasAlpha();
}

(sorry for the switched operands for !=; I don't have a preference there...)

[msarett, 2015-12-10 17:38:31]

Patchset #2 (id:20001) has been deleted

[msarett, 2015-12-10 17:41:18]

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkAndroidCodec.h
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkAndroidCode...
include/codec/SkAndroidCodec.h:219: bool reallyHasAlpha() const {
On 2015/12/10 17:02:32, scroggo wrote:
> With some changes, this doesn't even need to be virtual.
> 
> It seems that both subclasses (SkSampledCodec and SkWebpAdapterCodec) have an
> fCodec member. We could move that into this class (like we did with fStream)
and
> then call that directly.
> 
> Combining with an idea we discussed in person: SkAndroidCodec needs a lot less
> duplicated virtuals if we instead provide an accessor for the codec:
> 
> public class SkAndroidCodec{
> public:
>   ...
>   // I'm not tied to this name...
>   const SkCodec* actualCodec() const { return fCodec.get(); }
> protected:
>   SkCodec* codec() { return fCodec.get(); }
> private:
>   SkAutoTDelete<SkCodec> fCodec;
> }
> 
> Then the calling class can call these const methods directly:
> 
> SkCodec* codec = androidCodec.actualCodec();
> codec->getEncodedFormat()
> codec->reallyHasAlpha()
> ...

I went halfway there.  Can we save the actualCodec() part for later?

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:494: return kOpaque_SkAlphaType !=
this->getInfo().alphaType();
On 2015/12/10 17:02:32, scroggo wrote:
> SkWebpCodec's implementation looks like this (and maybe others? I have not
> checked). Should we stop them from overriding to do the same thing?
> 

Yes I think so.  Or we should make this pure virtual.  Only png and webp
remember to override this right now.

So on gif for example, we would have always (sometimes incorrectly) reported
that the image doesn't have alpha.

> I wonder if it could be written like the following:
> 
> bool reallyHasAlpha() const {
>   return this->getInfo().alphaType() != kOpaque && this->onReallyHasAlpha();
> }
> 
> (sorry for the switched operands for !=; I don't have a preference there...)

sgtm

[scroggo, 2015-12-10 18:29:42]

(Did you upload another patch?)

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkAndroidCodec.h
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkAndroidCode...
include/codec/SkAndroidCodec.h:219: bool reallyHasAlpha() const {
On 2015/12/10 17:41:18, msarett wrote:
> On 2015/12/10 17:02:32, scroggo wrote:
> > With some changes, this doesn't even need to be virtual.
> > 
> > It seems that both subclasses (SkSampledCodec and SkWebpAdapterCodec) have
an
> > fCodec member. We could move that into this class (like we did with fStream)
> and
> > then call that directly.
> > 
> > Combining with an idea we discussed in person: SkAndroidCodec needs a lot
less
> > duplicated virtuals if we instead provide an accessor for the codec:
> > 
> > public class SkAndroidCodec{
> > public:
> >   ...
> >   // I'm not tied to this name...
> >   const SkCodec* actualCodec() const { return fCodec.get(); }
> > protected:
> >   SkCodec* codec() { return fCodec.get(); }
> > private:
> >   SkAutoTDelete<SkCodec> fCodec;
> > }
> > 
> > Then the calling class can call these const methods directly:
> > 
> > SkCodec* codec = androidCodec.actualCodec();
> > codec->getEncodedFormat()
> > codec->reallyHasAlpha()
> > ...
> 
> I went halfway there.  Can we save the actualCodec() part for later?

fine with me

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1518743002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:494: return kOpaque_SkAlphaType !=
this->getInfo().alphaType();
On 2015/12/10 17:41:18, msarett wrote:
> On 2015/12/10 17:02:32, scroggo wrote:
> > SkWebpCodec's implementation looks like this (and maybe others? I have not
> > checked). Should we stop them from overriding to do the same thing?
> > 
> 
> Yes I think so.  Or we should make this pure virtual.  Only png and webp
> remember to override this right now.
> 
> So on gif for example, we would have always (sometimes incorrectly) reported
> that the image doesn't have alpha.

Yeah, I think we have a note about that in skbug.com/3582

> 
> > I wonder if it could be written like the following:
> > 
> > bool reallyHasAlpha() const {
> >   return this->getInfo().alphaType() != kOpaque && this->onReallyHasAlpha();
> > }
> > 
> > (sorry for the switched operands for !=; I don't have a preference there...)
> 
> sgtm

[msarett, 2015-12-10 18:33:04]

Now I did :)

[scroggo, 2015-12-10 18:45:44]

lgtm

https://codereview.chromium.org/1518743002/diff/40001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1518743002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:490: // If the image indicates that it does have alpha,
by default, we
Maybe add a comment that this is only called if the alpha type is not opaque?
Also, should this comment go outside the method body?

[msarett, 2015-12-10 18:53:30]

https://codereview.chromium.org/1518743002/diff/40001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1518743002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:490: // If the image indicates that it does have alpha,
by default, we
On 2015/12/10 18:45:44, scroggo wrote:
> Maybe add a comment that this is only called if the alpha type is not opaque?
> Also, should this comment go outside the method body?

Done and yes.

[scroggo, 2015-12-10 18:56:28]

On 2015/12/10 18:53:30, msarett wrote:
> https://codereview.chromium.org/1518743002/diff/40001/include/codec/SkCodec.h
> File include/codec/SkCodec.h (right):
> 
>
https://codereview.chromium.org/1518743002/diff/40001/include/codec/SkCodec.h...
> include/codec/SkCodec.h:490: // If the image indicates that it does have
alpha,
> by default, we
> On 2015/12/10 18:45:44, scroggo wrote:
> > Maybe add a comment that this is only called if the alpha type is not
opaque?
> > Also, should this comment go outside the method body?
> 
> Done and yes.

lgtm

[djsollen, 2015-12-10 20:50:55]

api lgtm

[msarett, 2015-12-10 20:55:48]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-12-10 20:55:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1518743002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1518743002/60001

[commit-bot: I haz the power, 2015-12-10 21:00:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 21:00:09]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[msarett, 2015-12-10 21:04:17]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-12-10 21:04:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1518743002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1518743002/60001

[commit-bot: I haz the power, 2015-12-10 21:09:26]

Description was changed from

==========
Add reallyHasAlpha() to SkAndroidCodec

This also modified the default implementation
of onReallyHasAlpha() in SkCodec.

BUG=skia:
==========

to

==========
Add reallyHasAlpha() to SkAndroidCodec

This also modified the default implementation
of onReallyHasAlpha() in SkCodec.

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/90c4d5fec8a9455729b401fd7818bea221dcada2
==========

[commit-bot: I haz the power, 2015-12-10 21:09:27]

Committed patchset #3 (id:60001) as
https://skia.googlesource.com/skia/+/90c4d5fec8a9455729b401fd7818bea221dcada2