Remove dependencies on Android's forked decoder libraries

Remove dependencies on Android's forked decoder libraries

Disable SkImageDecoder's code which relies on Android's customized
libpng and libjpeg. Build standard versions of libpng and libjpeg-turbo
everywhere. The SkImageDecoder code has been replaced with SkCodec, which
can decode subsets using standard library APIs

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/b747b90501823cc160848957dfa552877d83b11d

[msarett, 2015-11-04 18:58:47]

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

[scroggo, 2015-11-04 19:28:24]

This seems to sort of go halfway to removing the old forked jpeg code. The full
way would be:

- remove the special libjpeg from DEPS and
platform_tools/android/gyp/dependencies.gypi
- remove SK_JPEG_INDEX_SUPPORTED code
- maybe others?

This raises another question - are we ready to make the full switch? We already
have images in Gold that were generated by the old code, which we can continue
to compare to - though they'll eventually fall off. May not be a problem since
we don't seem to be far different. We don't a lot of performance data, though,
since we cannot run on the bots due to memory leaks. Is it worth keeping around
the old code (to be built not as part of the Android framework, but with special
flags) so that we can benchmark? Or do we think we've learned enough from
running the benchmarks locally?

https://codereview.chromium.org/1406153015/diff/1/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1406153015/diff/1/gyp/codec.gyp#newcode21
gyp/codec.gyp:21: 'libjpeg-turbo.gyp:libjpeg-turbo',
I think you need to update libjpeg-turbo.gyp to handle skia_android_framework.

IIUC, libjpeg-turbo.gyp tells us to build a bunch of files in third_party, but
instead you just want to include libjpeg-turbo as a shared library.

I think gyp/android_deps.gyp is the right example. For png, instead of building
anything, we just add a library and an include_dir:

            'direct_dependent_settings': {
              'libraries' : [
                '-lpng',
              ],
              'include_dirs': [
                'external/libpng',
              ],

We do the same thing for jpeg. So I think this needs to be updated to use
libjpeg-turbo.

https://codereview.chromium.org/1406153015/diff/1/gyp/libjpeg.gyp
File gyp/libjpeg.gyp (left):

https://codereview.chromium.org/1406153015/diff/1/gyp/libjpeg.gyp#oldcode20
gyp/libjpeg.gyp:20: 'target_name': 'libjpeg',
libjpeg goes through android_deps if we're building on Android. Maybe we don't
need so much misdirection now that we'll be building the same version of
libjpeg_turbo everywhere - we'll still need to do something different for the
Android framework, where we just link the shared library, but maybe we don't
need to go through android_deps.

https://codereview.chromium.org/1406153015/diff/1/src/images/SkImageDecoder_l...
File src/images/SkImageDecoder_libjpeg.cpp (left):

https://codereview.chromium.org/1406153015/diff/1/src/images/SkImageDecoder_l...
src/images/SkImageDecoder_libjpeg.cpp:58: #define SK_JPEG_INDEX_SUPPORTED
Should we go ahead and remove the code inside

#ifdef SK_JPEG_INDEX_SUPPORTED

?

[msarett, 2015-11-05 15:00:56]

"This raises another question - are we ready to make the full switch? We already
have images in Gold that were generated by the old code, which we can continue
to compare to - though they'll eventually fall off. May not be a problem since
we don't seem to be far different. We don't a lot of performance data, though,
since we cannot run on the bots due to memory leaks. Is it worth keeping around
the old code (to be built not as part of the Android framework, but with special
flags) so that we can benchmark? Or do we think we've learned enough from
running the benchmarks locally?"

Gold/Correctness Testing:

I don't see any benefit to keep testing the old code.  I think the correctness
issues are well-documented at this point.

Performance Testing:

I don't feel like we're done here yet, but I don't want this to hold up deleting
everything.  The gyp changes that we need to make would be tricky to undo
locally to keep testing, and it means Skia would need to keep the extra
libraries around.

I think we can accomplish this testing by checking out an old copy of Skia.

Code Deletion:

I have actually just gone ahead and made the gyp and DEPS changes.  IMO, we can
start here and then move ahead to code deletion in SkImageDecoder, dm,
nanobench, and tools after this has landed and is stable.

https://codereview.chromium.org/1406153015/diff/1/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1406153015/diff/1/gyp/codec.gyp#newcode21
gyp/codec.gyp:21: 'libjpeg-turbo.gyp:libjpeg-turbo',
On 2015/11/04 19:28:23, scroggo wrote:
> I think you need to update libjpeg-turbo.gyp to handle skia_android_framework.
> 
> IIUC, libjpeg-turbo.gyp tells us to build a bunch of files in third_party, but
> instead you just want to include libjpeg-turbo as a shared library.
> 
> I think gyp/android_deps.gyp is the right example. For png, instead of
building
> anything, we just add a library and an include_dir:
> 
>             'direct_dependent_settings': {
>               'libraries' : [
>                 '-lpng',
>               ],
>               'include_dirs': [
>                 'external/libpng',
>               ],
> 
> We do the same thing for jpeg. So I think this needs to be updated to use
> libjpeg-turbo.

You're right, there's quite a bit more gyp work to do.

https://codereview.chromium.org/1406153015/diff/1/gyp/libjpeg.gyp
File gyp/libjpeg.gyp (left):

https://codereview.chromium.org/1406153015/diff/1/gyp/libjpeg.gyp#oldcode20
gyp/libjpeg.gyp:20: 'target_name': 'libjpeg',
On 2015/11/04 19:28:23, scroggo wrote:
> libjpeg goes through android_deps if we're building on Android. Maybe we don't
> need so much misdirection now that we'll be building the same version of
> libjpeg_turbo everywhere - we'll still need to do something different for the
> Android framework, where we just link the shared library, but maybe we don't
> need to go through android_deps.

Acknowledged.

libjpeg-turbo.gyp is messy with all the yasm stuff.  I think it makes sense to
have codec.gyp choose either libjpeg-turbo.gyp:libjpeg-turbo or
android_deps.gyp:libjpeg-turbo.

https://codereview.chromium.org/1406153015/diff/1/src/images/SkImageDecoder_l...
File src/images/SkImageDecoder_libjpeg.cpp (left):

https://codereview.chromium.org/1406153015/diff/1/src/images/SkImageDecoder_l...
src/images/SkImageDecoder_libjpeg.cpp:58: #define SK_JPEG_INDEX_SUPPORTED
On 2015/11/04 19:28:23, scroggo wrote:
> Should we go ahead and remove the code inside
> 
> #ifdef SK_JPEG_INDEX_SUPPORTED
> 
> ?

Yes, I'd like to do it later in the interest of keeping this CL smaller.

[msarett, 2015-11-05 15:05:43]

https://codereview.chromium.org/1406153015/diff/20001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (left):

https://codereview.chromium.org/1406153015/diff/20001/gyp/common_conditions.g...
gyp/common_conditions.gypi:375: 'SK_PRINT_CODEC_MESSAGES',
Maybe this doesn't belong with this CL.

But this outputs a tremendous amount of jpeg garbage (so much that I wonder if
it affects performance).  Maybe a better fix is to turn off error printing from
libjpeg-turbo.  I'm not sure there has ever been a useful output error message.

[msarett, 2015-11-05 15:39:59]

https://codereview.chromium.org/1406153015/diff/20001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1406153015/diff/20001/gyp/images.gyp#newcode17
gyp/images.gyp:17: 'libjpeg-turbo.gyp:libjpeg-turbo',
This file still needs to be fixed.

[msarett, 2015-11-05 16:14:04]

I verified that the Makefile generated by gyp_to_android.py builds properly with
the changes to BRD in master_skia.

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp#newcode85
gyp/images.gyp:85: [ 'skia_android_framework == 0', {
It would be cleaner to include this above and then use dependencies! down below.

Unfortunately, they gyp generators for the android framework will try to parse
this file before hitting dependencies! and ignoring it.  It fails to parse
because host_os is not defined (we define this in android_setup.sh).

[scroggo, 2015-11-05 18:35:53]

> Remove dependencies on forked libjpeg

Will you please update the commit message? The current patch set appears to
remove dependencies on both forked libjpeg and forked libpng. I think it would
be useful to also state which fork you're talking about, and why - I don't know
that it needs a ton of detail for why, but something like:

"""
Remove dependencies on Android's forked decoder libraries

Disable SkImageDecoder's code which relies on Android's customized
libpng and libjpeg. Build standard versions of libpng and libjpeg-turbo
everywhere. The SkImageDecoder code has been replaced with SkCodec, which
can decode subsets using standard library APIs
"""

On the other hand, maybe this is change is big and complicated enough without
bringing png into it?

> I think we can accomplish this testing by checking out an old copy of Skia.

I think this will be tricky (especially if we find any bugs in our benchmarks).
It also doesn't allow us to automate running the benchmarks, but we're unable to
do that anyway, thanks to memory leaks. That doesn't mean we shouldn't do it
necessarily, but it is something to consider.

https://codereview.chromium.org/1406153015/diff/20001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (left):

https://codereview.chromium.org/1406153015/diff/20001/gyp/common_conditions.g...
gyp/common_conditions.gypi:375: 'SK_PRINT_CODEC_MESSAGES',
On 2015/11/05 15:05:43, msarett wrote:
> Maybe this doesn't belong with this CL.
> 
> But this outputs a tremendous amount of jpeg garbage (so much that I wonder if
> it affects performance).  Maybe a better fix is to turn off error printing
from
> libjpeg-turbo.  I'm not sure there has ever been a useful output error
message.

FWIW, turning off error printing in libjpeg-turbo should be easy. In
SkJpegDecoderMgr.cpp, we set emit_message and output_message to functions which
call SkCodecPrintf. We could instead set them (one or both) to do nothing.
SkImageDecoder_libjpeg does this.

Chrome's JPEGImageDecoder is worth looking at as well. It uses the default for
emit_message (found here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe...),
which uses the int parameter to decide whether to print anything. For
progressive images, it has its own emit_message function (here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
It uses the default version of output_message (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe...)

https://codereview.chromium.org/1406153015/diff/40001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1406153015/diff/40001/gyp/codec.gyp#newcode71
gyp/codec.gyp:71: 'android_deps.gyp:libjpeg-turbo',
It's weird to me that we directly use libpng.gyp here, which in turn decides
between libpng_static and android_deps:libpng, but we make the decision about
libjpeg-turbo vs android_deps:libjpeg_turbo.

https://codereview.chromium.org/1406153015/diff/40001/gyp/codec.gyp#newcode79
gyp/codec.gyp:79: 'libpng.gyp:libpng_static',
Do we still need libpng_static? I thought it was only around so we could build
two versions of libpng - one for codec, and one for imagedecoder.

(I guess it may still be handy within the file to label one as static, but I
don't see why images.gyp or codec.gyp need to make the distinction.)

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp#newcode85
gyp/images.gyp:85: [ 'skia_android_framework == 0', {
On 2015/11/05 16:14:04, msarett wrote:
> It would be cleaner to include this above and then use dependencies! down
below.
> 
> Unfortunately, they gyp generators for the android framework will try to parse
> this file before hitting dependencies! and ignoring it.  It fails to parse
> because host_os is not defined (we define this in android_setup.sh).

Yeah, this is an annoying feature of gyp itself - it will parse files it doesn't
need to. It might be worth adding a comment in the code about this.

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp#newcode148
gyp/images.gyp:148: 'libpng.gyp:libpng',
I'm confused: images.gyp seems a lot more complicated than codec.gyp. It used to
be more complicated because we had to worry about modified versions of
libjpeg/libpng. But now, they should be using the same versions as each other
everywhere, correct?

codec.gyp has one condition:
if skia_android_framework:
  libpng
  android_deps:libjpeg-turbo
else:
  libpng_static
  libjpeg-turbo

Can this one be simplified?

https://codereview.chromium.org/1406153015/diff/40001/gyp/libjpeg.gyp
File gyp/libjpeg.gyp (left):

https://codereview.chromium.org/1406153015/diff/40001/gyp/libjpeg.gyp#oldcode7
gyp/libjpeg.gyp:7: # See http://code.google.com/p/skia/issues/detail?id=543
('wrap libjpeg.gyp
Is this bug still relevant? I think we'll be using chrome's version of
libjpeg-turbo, but we still have our own gyp file? At the very least it seems
worthwhile to update that bug with how things have changed.

[msarett, 2015-11-05 22:11:50]

Description was changed from

==========
Remove dependencies on forked libjpeg

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

to

==========
Remove dependencies on Android's forked decoder libraries

Disable SkImageDecoder's code which relies on Android's customized
libpng and libjpeg. Build standard versions of libpng and libjpeg-turbo
everywhere. The SkImageDecoder code has been replaced with SkCodec, which
can decode subsets using standard library APIs

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

[msarett, 2015-11-05 22:13:25]

"I think this will be tricky (especially if we find any bugs in our benchmarks).
It also doesn't allow us to automate running the benchmarks, but we're unable to
do that anyway, thanks to memory leaks. That doesn't mean we shouldn't do it
necessarily, but it is something to consider."

I agree that benchmarking will become tricky, however, I still think we ought to
get rid of these libraries.

https://codereview.chromium.org/1406153015/diff/20001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (left):

https://codereview.chromium.org/1406153015/diff/20001/gyp/common_conditions.g...
gyp/common_conditions.gypi:375: 'SK_PRINT_CODEC_MESSAGES',
On 2015/11/05 18:35:52, scroggo wrote:
> On 2015/11/05 15:05:43, msarett wrote:
> > Maybe this doesn't belong with this CL.
> > 
> > But this outputs a tremendous amount of jpeg garbage (so much that I wonder
if
> > it affects performance).  Maybe a better fix is to turn off error printing
> from
> > libjpeg-turbo.  I'm not sure there has ever been a useful output error
> message.
> 
> FWIW, turning off error printing in libjpeg-turbo should be easy. In
> SkJpegDecoderMgr.cpp, we set emit_message and output_message to functions
which
> call SkCodecPrintf. We could instead set them (one or both) to do nothing.
> SkImageDecoder_libjpeg does this.
> 
> Chrome's JPEGImageDecoder is worth looking at as well. It uses the default for
> emit_message (found here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe...),
> which uses the int parameter to decide whether to print anything. For
> progressive images, it has its own emit_message function (here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> It uses the default version of output_message (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe...)
> 

I'll make the change in SkJpegCodec in a follow-up CL.  Turning this back on.

https://codereview.chromium.org/1406153015/diff/40001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1406153015/diff/40001/gyp/codec.gyp#newcode71
gyp/codec.gyp:71: 'android_deps.gyp:libjpeg-turbo',
On 2015/11/05 18:35:53, scroggo wrote:
> It's weird to me that we directly use libpng.gyp here, which in turn decides
> between libpng_static and android_deps:libpng, but we make the decision about
> libjpeg-turbo vs android_deps:libjpeg_turbo.

Moving the decision out of this file.

https://codereview.chromium.org/1406153015/diff/40001/gyp/codec.gyp#newcode79
gyp/codec.gyp:79: 'libpng.gyp:libpng_static',
On 2015/11/05 18:35:53, scroggo wrote:
> Do we still need libpng_static? I thought it was only around so we could build
> two versions of libpng - one for codec, and one for imagedecoder.
> 
> (I guess it may still be handy within the file to label one as static, but I
> don't see why images.gyp or codec.gyp need to make the distinction.)

I think we're fine to include libpng now.  I think it will end up choosing
libpng_static unless we are on the framework.  Punting on whether or not a
static library is this is the right decision...

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp#newcode148
gyp/images.gyp:148: 'libpng.gyp:libpng',
On 2015/11/05 18:35:53, scroggo wrote:
> I'm confused: images.gyp seems a lot more complicated than codec.gyp. It used
to
> be more complicated because we had to worry about modified versions of
> libjpeg/libpng. But now, they should be using the same versions as each other
> everywhere, correct?
> 
> codec.gyp has one condition:
> if skia_android_framework:
>   libpng
>   android_deps:libjpeg-turbo
> else:
>   libpng_static
>   libjpeg-turbo
> 
> Can this one be simplified?

You're right we can simplify this.

https://codereview.chromium.org/1406153015/diff/40001/gyp/libjpeg.gyp
File gyp/libjpeg.gyp (left):

https://codereview.chromium.org/1406153015/diff/40001/gyp/libjpeg.gyp#oldcode7
gyp/libjpeg.gyp:7: # See http://code.google.com/p/skia/issues/detail?id=543
('wrap libjpeg.gyp
On 2015/11/05 18:35:52, scroggo wrote:
> Is this bug still relevant? I think we'll be using chrome's version of
> libjpeg-turbo, but we still have our own gyp file? At the very least it seems
> worthwhile to update that bug with how things have changed.

Added some thoughts to the bug.

[scroggo, 2015-11-05 22:24:56]

lgtm

https://codereview.chromium.org/1406153015/diff/40001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1406153015/diff/40001/gyp/codec.gyp#newcode79
gyp/codec.gyp:79: 'libpng.gyp:libpng_static',
On 2015/11/05 22:13:25, msarett wrote:
> On 2015/11/05 18:35:53, scroggo wrote:
> > Do we still need libpng_static? I thought it was only around so we could
build
> > two versions of libpng - one for codec, and one for imagedecoder.
> > 
> > (I guess it may still be handy within the file to label one as static, but I
> > don't see why images.gyp or codec.gyp need to make the distinction.)
> 
> I think we're fine to include libpng now.  I think it will end up choosing
> libpng_static unless we are on the framework.  Punting on whether or not a
> static library is this is the right decision...

Oh, sorry, I didn't mean we should not build a static library; I just wondered
whether we needed to distinguish that in the name.

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp#newcode126
gyp/images.gyp:126: 'export_dependent_settings': [
Why did this get removed? Is this a separate cleanup?

https://codereview.chromium.org/1406153015/diff/60001/gyp/libjpeg-turbo-selec...
File gyp/libjpeg-turbo-selector.gyp (right):

https://codereview.chromium.org/1406153015/diff/60001/gyp/libjpeg-turbo-selec...
gyp/libjpeg-turbo-selector.gyp:1: # Copyright 2014 Google Inc.
2015

[msarett, 2015-11-05 22:39:53]

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1406153015/diff/40001/gyp/images.gyp#newcode126
gyp/images.gyp:126: 'export_dependent_settings': [
On 2015/11/05 22:24:55, scroggo wrote:
> Why did this get removed? Is this a separate cleanup?

This is a clean-up.  libpng.gyp will choose either libpng_static or
android_deps:libpng and also add that to export_dependent_settings.  So I don't
think it is necessary to set export_dependent_settings here.

https://codereview.chromium.org/1406153015/diff/60001/gyp/libjpeg-turbo-selec...
File gyp/libjpeg-turbo-selector.gyp (right):

https://codereview.chromium.org/1406153015/diff/60001/gyp/libjpeg-turbo-selec...
gyp/libjpeg-turbo-selector.gyp:1: # Copyright 2014 Google Inc.
On 2015/11/05 22:24:56, scroggo wrote:
> 2015

:)

[scroggo, 2015-11-05 22:42:16]

LGTM

[msarett, 2015-11-06 19:04:09]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-11-06 19:04:23]

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

[commit-bot: I haz the power, 2015-11-06 19:15:53]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/b747b90501823cc160848957dfa552877d83b11d