Fixes for partial images.

Fixes for partial images.

Fix two cases where we had undesired behavior when attempting to decode
a stream that is incomplete:

src/images/SkImageDecoder_libgif.cpp:
src/images/SkImageDecoder_libjpeg.cpp:
By default, if an image stream is incomplete, do not return a partially
decoded image. Leave in a build flag that allows decoding partial
images.

include/core/SkUserConfig.h:
Add the build flag for reference.

Note that this will slightly change the behavior of our SKP tests. Now
they will not produce partially decoded images (though they will
continue to decode the data available and then return false).

BUG=b/17419670

[scroggo, 2014-10-17 14:18:57]

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

[djsollen, 2014-10-17 14:45:29]

lgtm

[hal.canary, 2014-10-17 14:47:55]

lgtm

[scroggo, 2014-10-17 14:55:31]

scroggo@google.com changed reviewers:
+ reed@google.com

[scroggo, 2014-10-17 14:55:31]

Mike suggested that instead of using a build flag (which means we aren't testing
one version of the code or the other), we return an enum with 3 (or more?)
values:

enum DecodeSuccess {
  kFailed,
  kPartial,
  kComplete
}

We'll need to change the signatures of:

    bool decode(SkStream*, SkBitmap* bitmap, SkColorType pref, Mode);
    bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode);

as well as both versions of the following static functions:

    bool DecodeFile
    bool DecodeMemory
    bool DecodeStream

(unless we want to have different return values for different versions of
decode, which I think would be confusing).

We also need to fix the call sites. I haven't looked around for these, but I
think we just need to fix our tests and a few spots in the Android framework. We
could be a little sneaky, and define kFailed to be 0 and the others to be
non-zero, in which case the callers don't need to change (although come to think
of it, implicitly converting an enum to a bool will give us a warning that we
may consider an error).

At one point, I thought we had discussed a new API for image decoding that
returns an SkImage (or null on failure) instead of a bool, which would conflict
with this idea. Looking at
https://docs.google.com/a/google.com/document/d/1G27JOfEqGXm62pFnU8QtfUgBP50H...
though (is that our latest thinking on this?), we have a signature which returns
a bool (this signature was designed to leave memory allocation up to the
caller). This version is compatible with returning an enum.

Thoughts?

[scroggo, 2014-10-17 15:23:30]

On 2014/10/17 14:55:31, scroggo wrote:
> Mike suggested that instead of using a build flag (which means we aren't
testing
> one version of the code or the other), we return an enum with 3 (or more?)
> values:
> 
> enum DecodeSuccess {
>   kFailed,
>   kPartial,
>   kComplete
> }
> 
> We'll need to change the signatures of:
> 
>     bool decode(SkStream*, SkBitmap* bitmap, SkColorType pref, Mode);
>     bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode);
> 
> as well as both versions of the following static functions:
> 
>     bool DecodeFile
>     bool DecodeMemory
>     bool DecodeStream
> 
> (unless we want to have different return values for different versions of
> decode, which I think would be confusing).
> 
> We also need to fix the call sites. I haven't looked around for these, but I
> think we just need to fix our tests and a few spots in the Android framework.
We
> could be a little sneaky, and define kFailed to be 0 and the others to be
> non-zero, in which case the callers don't need to change (although come to
think
> of it, implicitly converting an enum to a bool will give us a warning that we
> may consider an error).
> 
> At one point, I thought we had discussed a new API for image decoding that
> returns an SkImage (or null on failure) instead of a bool, which would
conflict
> with this idea. Looking at
>
https://docs.google.com/a/google.com/document/d/1G27JOfEqGXm62pFnU8QtfUgBP50H...
> though (is that our latest thinking on this?), we have a signature which
returns
> a bool (this signature was designed to leave memory allocation up to the
> caller). This version is compatible with returning an enum.
> 
> Thoughts?

If we go this route, should we have a separate enum for successfully decoding
the bounds?

[djsollen, 2014-10-17 15:29:17]

On 2014/10/17 15:23:30, scroggo wrote:
> On 2014/10/17 14:55:31, scroggo wrote:
> > Mike suggested that instead of using a build flag (which means we aren't
> testing
> > one version of the code or the other), we return an enum with 3 (or more?)
> > values:
> > 
> > enum DecodeSuccess {
> >   kFailed,
> >   kPartial,
> >   kComplete
> > }
> > 
> > We'll need to change the signatures of:
> > 
> >     bool decode(SkStream*, SkBitmap* bitmap, SkColorType pref, Mode);
> >     bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode);
> > 
> > as well as both versions of the following static functions:
> > 
> >     bool DecodeFile
> >     bool DecodeMemory
> >     bool DecodeStream
> > 
> > (unless we want to have different return values for different versions of
> > decode, which I think would be confusing).
> > 
> > We also need to fix the call sites. I haven't looked around for these, but I
> > think we just need to fix our tests and a few spots in the Android
framework.
> We
> > could be a little sneaky, and define kFailed to be 0 and the others to be
> > non-zero, in which case the callers don't need to change (although come to
> think
> > of it, implicitly converting an enum to a bool will give us a warning that
we
> > may consider an error).
> > 
> > At one point, I thought we had discussed a new API for image decoding that
> > returns an SkImage (or null on failure) instead of a bool, which would
> conflict
> > with this idea. Looking at
> >
>
https://docs.google.com/a/google.com/document/d/1G27JOfEqGXm62pFnU8QtfUgBP50H...
> > though (is that our latest thinking on this?), we have a signature which
> returns
> > a bool (this signature was designed to leave memory allocation up to the
> > caller). This version is compatible with returning an enum.
> > 
> > Thoughts?
> 
> If we go this route, should we have a separate enum for successfully decoding
> the bounds?

I like the enum approach for return values for both decoding content and bounds.

[reed1, 2014-10-17 15:35:06]

On 2014/10/17 15:29:17, djsollen wrote:
> On 2014/10/17 15:23:30, scroggo wrote:
> > On 2014/10/17 14:55:31, scroggo wrote:
> > > Mike suggested that instead of using a build flag (which means we aren't
> > testing
> > > one version of the code or the other), we return an enum with 3 (or more?)
> > > values:
> > > 
> > > enum DecodeSuccess {
> > >   kFailed,
> > >   kPartial,
> > >   kComplete
> > > }
> > > 
> > > We'll need to change the signatures of:
> > > 
> > >     bool decode(SkStream*, SkBitmap* bitmap, SkColorType pref, Mode);
> > >     bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode);
> > > 
> > > as well as both versions of the following static functions:
> > > 
> > >     bool DecodeFile
> > >     bool DecodeMemory
> > >     bool DecodeStream
> > > 
> > > (unless we want to have different return values for different versions of
> > > decode, which I think would be confusing).
> > > 
> > > We also need to fix the call sites. I haven't looked around for these, but
I
> > > think we just need to fix our tests and a few spots in the Android
> framework.
> > We
> > > could be a little sneaky, and define kFailed to be 0 and the others to be
> > > non-zero, in which case the callers don't need to change (although come to
> > think
> > > of it, implicitly converting an enum to a bool will give us a warning that
> we
> > > may consider an error).
> > > 
> > > At one point, I thought we had discussed a new API for image decoding that
> > > returns an SkImage (or null on failure) instead of a bool, which would
> > conflict
> > > with this idea. Looking at
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1G27JOfEqGXm62pFnU8QtfUgBP50H...
> > > though (is that our latest thinking on this?), we have a signature which
> > returns
> > > a bool (this signature was designed to leave memory allocation up to the
> > > caller). This version is compatible with returning an enum.
> > > 
> > > Thoughts?
> > 
> > If we go this route, should we have a separate enum for successfully
decoding
> > the bounds?
> 
> I like the enum approach for return values for both decoding content and
bounds.

What is the sequence that might want a different return code for bounds? I
thought bounds -vs- pixels was determined by the caller, not our internal
success/failure.

[scroggo, 2014-10-17 15:40:00]

On 2014/10/17 15:35:06, reed1 wrote:
> On 2014/10/17 15:29:17, djsollen wrote:
> > On 2014/10/17 15:23:30, scroggo wrote:
> > > On 2014/10/17 14:55:31, scroggo wrote:
> > > > Mike suggested that instead of using a build flag (which means we aren't
> > > testing
> > > > one version of the code or the other), we return an enum with 3 (or
more?)
> > > > values:
> > > > 
> > > > enum DecodeSuccess {
> > > >   kFailed,
> > > >   kPartial,
> > > >   kComplete
> > > > }
> > > > 
> > > > We'll need to change the signatures of:
> > > > 
> > > >     bool decode(SkStream*, SkBitmap* bitmap, SkColorType pref, Mode);
> > > >     bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode);
> > > > 
> > > > as well as both versions of the following static functions:
> > > > 
> > > >     bool DecodeFile
> > > >     bool DecodeMemory
> > > >     bool DecodeStream
> > > > 
> > > > (unless we want to have different return values for different versions
of
> > > > decode, which I think would be confusing).
> > > > 
> > > > We also need to fix the call sites. I haven't looked around for these,
but
> I
> > > > think we just need to fix our tests and a few spots in the Android
> > framework.
> > > We
> > > > could be a little sneaky, and define kFailed to be 0 and the others to
be
> > > > non-zero, in which case the callers don't need to change (although come
to
> > > think
> > > > of it, implicitly converting an enum to a bool will give us a warning
that
> > we
> > > > may consider an error).
> > > > 
> > > > At one point, I thought we had discussed a new API for image decoding
that
> > > > returns an SkImage (or null on failure) instead of a bool, which would
> > > conflict
> > > > with this idea. Looking at
> > > >
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1G27JOfEqGXm62pFnU8QtfUgBP50H...
> > > > though (is that our latest thinking on this?), we have a signature which
> > > returns
> > > > a bool (this signature was designed to leave memory allocation up to the
> > > > caller). This version is compatible with returning an enum.
> > > > 
> > > > Thoughts?
> > > 
> > > If we go this route, should we have a separate enum for successfully
> decoding
> > > the bounds?
> > 
> > I like the enum approach for return values for both decoding content and
> bounds.
> 
> What is the sequence that might want a different return code for bounds? I
> thought bounds -vs- pixels was determined by the caller, not our internal
> success/failure.

My concern is that DecodeSuccess seems like a weird answer to "did I
decode the bounds?", since it has two possibilities that make sense and one
that does not. That said, adding kBoundsSuccess doesn't really solve that
problem (it just adds one that makes the normal success value ambiguous).
I think I'll rename kComplete to kSuccess, and just use that for the bounds
decode. How does that sound?

[reed1, 2014-10-17 15:42:04]

On 2014/10/17 15:40:00, scroggo wrote:
> On 2014/10/17 15:35:06, reed1 wrote:
> > On 2014/10/17 15:29:17, djsollen wrote:
> > > On 2014/10/17 15:23:30, scroggo wrote:
> > > > On 2014/10/17 14:55:31, scroggo wrote:
> > > > > Mike suggested that instead of using a build flag (which means we
aren't
> > > > testing
> > > > > one version of the code or the other), we return an enum with 3 (or
> more?)
> > > > > values:
> > > > > 
> > > > > enum DecodeSuccess {
> > > > >   kFailed,
> > > > >   kPartial,
> > > > >   kComplete
> > > > > }
> > > > > 
> > > > > We'll need to change the signatures of:
> > > > > 
> > > > >     bool decode(SkStream*, SkBitmap* bitmap, SkColorType pref, Mode);
> > > > >     bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode);
> > > > > 
> > > > > as well as both versions of the following static functions:
> > > > > 
> > > > >     bool DecodeFile
> > > > >     bool DecodeMemory
> > > > >     bool DecodeStream
> > > > > 
> > > > > (unless we want to have different return values for different versions
> of
> > > > > decode, which I think would be confusing).
> > > > > 
> > > > > We also need to fix the call sites. I haven't looked around for these,
> but
> > I
> > > > > think we just need to fix our tests and a few spots in the Android
> > > framework.
> > > > We
> > > > > could be a little sneaky, and define kFailed to be 0 and the others to
> be
> > > > > non-zero, in which case the callers don't need to change (although
come
> to
> > > > think
> > > > > of it, implicitly converting an enum to a bool will give us a warning
> that
> > > we
> > > > > may consider an error).
> > > > > 
> > > > > At one point, I thought we had discussed a new API for image decoding
> that
> > > > > returns an SkImage (or null on failure) instead of a bool, which would
> > > > conflict
> > > > > with this idea. Looking at
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1G27JOfEqGXm62pFnU8QtfUgBP50H...
> > > > > though (is that our latest thinking on this?), we have a signature
which
> > > > returns
> > > > > a bool (this signature was designed to leave memory allocation up to
the
> > > > > caller). This version is compatible with returning an enum.
> > > > > 
> > > > > Thoughts?
> > > > 
> > > > If we go this route, should we have a separate enum for successfully
> > decoding
> > > > the bounds?
> > > 
> > > I like the enum approach for return values for both decoding content and
> > bounds.
> > 
> > What is the sequence that might want a different return code for bounds? I
> > thought bounds -vs- pixels was determined by the caller, not our internal
> > success/failure.
> 
> My concern is that DecodeSuccess seems like a weird answer to "did I
> decode the bounds?", since it has two possibilities that make sense and one
> that does not. That said, adding kBoundsSuccess doesn't really solve that
> problem (it just adds one that makes the normal success value ambiguous).
> I think I'll rename kComplete to kSuccess, and just use that for the bounds
> decode. How does that sound?

sgtm

[djsollen, 2014-10-17 15:57:35]

On 2014/10/17 15:42:04, reed1 wrote:
> On 2014/10/17 15:40:00, scroggo wrote:
> > On 2014/10/17 15:35:06, reed1 wrote:
> > > On 2014/10/17 15:29:17, djsollen wrote:
> > > > On 2014/10/17 15:23:30, scroggo wrote:
> > > > > On 2014/10/17 14:55:31, scroggo wrote:
> > > > > > Mike suggested that instead of using a build flag (which means we
> aren't
> > > > > testing
> > > > > > one version of the code or the other), we return an enum with 3 (or
> > more?)
> > > > > > values:
> > > > > > 
> > > > > > enum DecodeSuccess {
> > > > > >   kFailed,
> > > > > >   kPartial,
> > > > > >   kComplete
> > > > > > }
> > > > > > 
> > > > > > We'll need to change the signatures of:
> > > > > > 
> > > > > >     bool decode(SkStream*, SkBitmap* bitmap, SkColorType pref,
Mode);
> > > > > >     bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode);
> > > > > > 
> > > > > > as well as both versions of the following static functions:
> > > > > > 
> > > > > >     bool DecodeFile
> > > > > >     bool DecodeMemory
> > > > > >     bool DecodeStream
> > > > > > 
> > > > > > (unless we want to have different return values for different
versions
> > of
> > > > > > decode, which I think would be confusing).
> > > > > > 
> > > > > > We also need to fix the call sites. I haven't looked around for
these,
> > but
> > > I
> > > > > > think we just need to fix our tests and a few spots in the Android
> > > > framework.
> > > > > We
> > > > > > could be a little sneaky, and define kFailed to be 0 and the others
to
> > be
> > > > > > non-zero, in which case the callers don't need to change (although
> come
> > to
> > > > > think
> > > > > > of it, implicitly converting an enum to a bool will give us a
warning
> > that
> > > > we
> > > > > > may consider an error).
> > > > > > 
> > > > > > At one point, I thought we had discussed a new API for image
decoding
> > that
> > > > > > returns an SkImage (or null on failure) instead of a bool, which
would
> > > > > conflict
> > > > > > with this idea. Looking at
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1G27JOfEqGXm62pFnU8QtfUgBP50H...
> > > > > > though (is that our latest thinking on this?), we have a signature
> which
> > > > > returns
> > > > > > a bool (this signature was designed to leave memory allocation up to
> the
> > > > > > caller). This version is compatible with returning an enum.
> > > > > > 
> > > > > > Thoughts?
> > > > > 
> > > > > If we go this route, should we have a separate enum for successfully
> > > decoding
> > > > > the bounds?
> > > > 
> > > > I like the enum approach for return values for both decoding content and
> > > bounds.
> > > 
> > > What is the sequence that might want a different return code for bounds? I
> > > thought bounds -vs- pixels was determined by the caller, not our internal
> > > success/failure.
> > 
> > My concern is that DecodeSuccess seems like a weird answer to "did I
> > decode the bounds?", since it has two possibilities that make sense and one
> > that does not. That said, adding kBoundsSuccess doesn't really solve that
> > problem (it just adds one that makes the normal success value ambiguous).
> > I think I'll rename kComplete to kSuccess, and just use that for the bounds
> > decode. How does that sound?
> 
> sgtm

ditto

[scroggo, 2014-10-17 18:29:33]

On 2014/10/17 15:57:35, djsollen wrote:
> On 2014/10/17 15:42:04, reed1 wrote:
> > On 2014/10/17 15:40:00, scroggo wrote:
> > > On 2014/10/17 15:35:06, reed1 wrote:
> > > > On 2014/10/17 15:29:17, djsollen wrote:
> > > > > On 2014/10/17 15:23:30, scroggo wrote:
> > > > > > On 2014/10/17 14:55:31, scroggo wrote:
> > > > > > > Mike suggested that instead of using a build flag (which means we
> > aren't
> > > > > > testing
> > > > > > > one version of the code or the other), we return an enum with 3
(or
> > > more?)
> > > > > > > values:
> > > > > > > 
> > > > > > > enum DecodeSuccess {
> > > > > > >   kFailed,
> > > > > > >   kPartial,
> > > > > > >   kComplete
> > > > > > > }
> > > > > > > 
> > > > > > > We'll need to change the signatures of:
> > > > > > > 
> > > > > > >     bool decode(SkStream*, SkBitmap* bitmap, SkColorType pref,
> Mode);
> > > > > > >     bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode);
> > > > > > > 
> > > > > > > as well as both versions of the following static functions:
> > > > > > > 
> > > > > > >     bool DecodeFile
> > > > > > >     bool DecodeMemory
> > > > > > >     bool DecodeStream
> > > > > > > 
> > > > > > > (unless we want to have different return values for different
> versions
> > > of
> > > > > > > decode, which I think would be confusing).
> > > > > > > 
> > > > > > > We also need to fix the call sites. I haven't looked around for
> these,
> > > but
> > > > I
> > > > > > > think we just need to fix our tests and a few spots in the Android
> > > > > framework.
> > > > > > We
> > > > > > > could be a little sneaky, and define kFailed to be 0 and the
others
> to
> > > be
> > > > > > > non-zero, in which case the callers don't need to change (although
> > come
> > > to
> > > > > > think
> > > > > > > of it, implicitly converting an enum to a bool will give us a
> warning
> > > that
> > > > > we
> > > > > > > may consider an error).
> > > > > > > 
> > > > > > > At one point, I thought we had discussed a new API for image
> decoding
> > > that
> > > > > > > returns an SkImage (or null on failure) instead of a bool, which
> would
> > > > > > conflict
> > > > > > > with this idea. Looking at
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1G27JOfEqGXm62pFnU8QtfUgBP50H...
> > > > > > > though (is that our latest thinking on this?), we have a signature
> > which
> > > > > > returns
> > > > > > > a bool (this signature was designed to leave memory allocation up
to
> > the
> > > > > > > caller). This version is compatible with returning an enum.
> > > > > > > 
> > > > > > > Thoughts?
> > > > > > 
> > > > > > If we go this route, should we have a separate enum for successfully
> > > > decoding
> > > > > > the bounds?
> > > > > 
> > > > > I like the enum approach for return values for both decoding content
and
> > > > bounds.
> > > > 
> > > > What is the sequence that might want a different return code for bounds?
I
> > > > thought bounds -vs- pixels was determined by the caller, not our
internal
> > > > success/failure.
> > > 
> > > My concern is that DecodeSuccess seems like a weird answer to "did I
> > > decode the bounds?", since it has two possibilities that make sense and
one
> > > that does not. That said, adding kBoundsSuccess doesn't really solve that
> > > problem (it just adds one that makes the normal success value ambiguous).
> > > I think I'll rename kComplete to kSuccess, and just use that for the
bounds
> > > decode. How does that sound?
> > 
> > sgtm
> 
> ditto

I used an enum instead in https://codereview.chromium.org/647023006/.

I left DecodeMemory etc as is (return bool, not enum) for the convenience of not
editing a lot more code (esp SkPicture::CreateFromStream)