Fix GIF transparency.

Fix GIF transparency.

The GIF decoder, in the onDecode() function, tries to obtain the
transparency info from the temporary SavedImage object, temp_save,
but the decoder had been changed to save extension block data to
the GifFileType object instead. As a result, find_transpIndex() can't
find the correct transaprency index and returns -1.

This had been reported in Android 4.4. AOSP pulled skia changes
related to giflib 5.x changes after Android 4.3, which is why
the problem is not reproducible in versions before KitKat.

AOSP Issue 62016:
https://code.google.com/p/android/issues/detail?id=62016

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/63af1441ceaf60a52c0b3ea11549700a0205c85d

[jei.mayol, 2014-07-30 07:34:00]

I had built and tested the change in the external/skia fork on an AOSP
android-4.4.2_r2 branch. The involved portion of the code in
SkImageDecoder_libgif.cpp between the skia upstream and Android 4.4 fork are
largely the same.

[jei.mayol, 2014-07-31 08:20:56]

Do I need to log a separate issue against Skia or is it enough that I referred
to the AOSP issue in the commit message?

Thanks,
Mayol

[djsollen, 2014-07-31 12:33:51]

On 2014/07/31 08:20:56, jei.mayol wrote:
> Do I need to log a separate issue against Skia or is it enough that I referred
> to the AOSP issue in the commit message?
> 
> Thanks,
> Mayol

It is sufficient to reference the AOSP issue in this case. I am already looking
at an identical fix from HTC in
https://android-review.googlesource.com/#/c/100010/.

[hal.canary, 2014-07-31 13:10:14]

On 2014/07/30 07:34:00, jei.mayol wrote:
> I had built and tested the change in the external/skia fork on an AOSP
> android-4.4.2_r2 branch.

What does your test look like?  Does it make sense to check a small unit test
into Skia with this patch?

[jei.mayol, 2014-08-01 06:44:25]

On 2014/07/31 12:33:51, djsollen wrote:
> On 2014/07/31 08:20:56, jei.mayol wrote:
> > Do I need to log a separate issue against Skia or is it enough that I
referred
> > to the AOSP issue in the commit message?
> > 
> > Thanks,
> > Mayol
> 
> It is sufficient to reference the AOSP issue in this case. I am already
looking
> at an identical fix from HTC in
> https://android-review.googlesource.com/#/c/100010/.

Derek,

Thanks for letting me know about that fix from HTC. I see that it's the exact
same patch. The Android docs suggest to submit patches/fixes for external
projects to their upstream, which is why I submitted this here instead. I guess
this review is redundant and could be discarded?

[jei.mayol, 2014-08-01 07:20:12]

On 2014/07/31 13:10:14, Hal Canary wrote:
> On 2014/07/30 07:34:00, jei.mayol wrote:
> > I had built and tested the change in the external/skia fork on an AOSP
> > android-4.4.2_r2 branch.
> 
> What does your test look like?  Does it make sense to check a small unit test
> into Skia with this patch?

Hal,

I only did manual testing with this on a local aosp build. In a local app I
placed a GIF image with transparent background and compared how it looked before
and after the fix.

Yes, I think a unit test could be added to GifTest.cpp to cover for this. My
skia workspace is not completely set up for the upstream project tests as I
mostly play around with aosp, and thus worked on aosp's skia fork for this, but
I think I can look into it.

Since this is redundant to the HTC fix, should I just submit a separate review
for the unit test and discard this?

[hal.canary, 2014-08-01 16:13:31]

On 2014/08/01 07:20:12, jei.mayol wrote:
> I only did manual testing with this on a local aosp build. In a
> local app I placed a GIF image with transparent background and
> compared how it looked before and after the fix.
>
> Yes, I think a unit test could be added to GifTest.cpp to cover for
> this. My skia workspace is not completely set up for the upstream
> project tests as I mostly play around with aosp, and thus worked on
> aosp's skia fork for this, but I think I can look into it.
>
> Since this is redundant to the HTC fix, should I just submit a
> separate review for the unit test and discard this?

If you can add a small unit test to GifTest.cpp, that would be great!

[djsollen, 2015-01-30 15:32:14]

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

[djsollen, 2015-01-30 15:32:14]

Looks like this CL got lost along the way.  Leon, anything preventing us from
landing this?

[scroggo, 2015-01-30 15:48:18]

On 2015/01/30 15:32:14, djsollen wrote:
> Looks like this CL got lost along the way. 

This got uploaded two days after my daughter was born, so it got lost in the
pile during paternity leave (if it ever got sent to me?).

> Leon, anything preventing us from
> landing this?

A test would be nice. I think it would be better to add an image file to test
with DM though. I'm also fine to use the image uploaded to
https://code.google.com/p/android/issues/detail?id=62016

LGTM

On 2014/08/01 06:44:25, jei.mayol wrote:
> Derek,
> 
> Thanks for letting me know about that fix from HTC. I see that it's the exact
> same patch. The Android docs suggest to submit patches/fixes for external
> projects to their upstream, which is why I submitted this here instead. I
guess
> this review is redundant and could be discarded?

No, this is the correct patch to submit. The one in AOSP can be discarded.

[scroggo, 2015-01-30 20:48:36]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-01-30 20:48:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/429053004/1

[commit-bot: I haz the power, 2015-01-30 20:48:50]

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

[commit-bot: I haz the power, 2015-01-30 20:48:51]

Presubmit check for 429053004-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
The email jei.mayol@gmail.com is not in Skia's AUTHORS file.
Issue owner, this CL must include an addition to the Skia AUTHORS file.
Googler reviewers, please check that the AUTHORS entry corresponds to an email
address in http://goto/cla-signers. If it does not then ask the issue owner to
sign the CLA at https://developers.google.com/open-source/cla/individual
(individual) or https://developers.google.com/open-source/cla/corporate
(corporate).

[scroggo, 2015-01-30 20:51:42]

On 2015/01/30 20:48:51, I haz the power (commit-bot) wrote:
> Presubmit check for 429053004-1 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> The email mailto:jei.mayol@gmail.com is not in Skia's AUTHORS file.
> Issue owner, this CL must include an addition to the Skia AUTHORS file.
> Googler reviewers, please check that the AUTHORS entry corresponds to an email
> address in http://goto/cla-signers. If it does not then ask the issue owner to
> sign the CLA at https://developers.google.com/open-source/cla/individual
> (individual) or https://developers.google.com/open-source/cla/corporate
> (corporate).

jei.mayol@gmail.com, would you mind signing the agreement so we can submit this
for you?

[jei.mayol, 2015-02-01 14:39:25]

On 2015/01/30 20:51:42, scroggo wrote:
> On 2015/01/30 20:48:51, I haz the power (commit-bot) wrote:
> > Presubmit check for 429053004-1 failed and returned exit status 1.
> > 
> > Running presubmit commit checks ...
> > 
> > ** Presubmit ERRORS **
> > The email mailto:jei.mayol@gmail.com is not in Skia's AUTHORS file.
> > Issue owner, this CL must include an addition to the Skia AUTHORS file.
> > Googler reviewers, please check that the AUTHORS entry corresponds to an
email
> > address in http://goto/cla-signers. If it does not then ask the issue owner
to
> > sign the CLA at https://developers.google.com/open-source/cla/individual
> > (individual) or https://developers.google.com/open-source/cla/corporate
> > (corporate).
> 
> mailto:jei.mayol@gmail.com, would you mind signing the agreement so we can
submit this
> for you?

Hello,

I double-checked my contributor license agreements and verified that I've
already signed a Google Individual CLA dated 07/24/2014, 20:53 PDT. Aside from
that, I also have an Android Individual CLA signed 10/07/2013, 07:40 PDT.
Anything I might have missed?

Also, my apologies on not being able to follow up on the unit test from Hal's
previous recommendations. To make the long story short, I was away for a few
months related to my regular job and was unable to follow up on the review.

[scroggo, 2015-02-02 01:16:13]

Sorry about that; I did not realize you had already signed it :-/

Please upload a new patch with your name and email in the AUTHORS file (top
level). Then we should be able to commit this.

[jei.mayol, 2015-02-05 03:08:32]

On 2015/02/02 01:16:13, scroggo wrote:
> Sorry about that; I did not realize you had already signed it :-/
> 
> Please upload a new patch with your name and email in the AUTHORS file (top
> level). Then we should be able to commit this.

I've updated the AUTHORS file. Kindly see if you can land this now. Thanks!

[djsollen, 2015-02-05 05:25:10]

The CQ bit was checked by djsollen@google.com

[commit-bot: I haz the power, 2015-02-05 05:26:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/429053004/20001

[commit-bot: I haz the power, 2015-02-05 05:31:28]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/63af1441ceaf60a52c0b3ea11549700a0205c85d