Switching Skia to chromium's libjpeg-turbo

Switching Skia to chromium's libjpeg-turbo

Compile SkJpegCodec and SkImageDecoder_libjpeg with
chromium's libjpeg-turbo.  SkImageDecoder_libjpeg still uses
libjpeg on Android and the Android framework.  SkJpegCodec is
still not compiled on the Android framework.

BUG=skia:

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

[msarett, 2015-08-10 14:56:53]

Patchset #1 (id:1) has been deleted

[msarett, 2015-08-10 17:22:19]

Patchset #2 (id:40001) has been deleted

[msarett, 2015-08-10 17:28:20]

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

[msarett, 2015-08-10 17:28:21]

https://codereview.chromium.org/1275773004/diff/20001/gyp/libjpeg-turbo.gyp
File gyp/libjpeg-turbo.gyp (right):

https://codereview.chromium.org/1275773004/diff/20001/gyp/libjpeg-turbo.gyp#n...
gyp/libjpeg-turbo.gyp:211: [ '"mips" in skia_arch_type', {
Chromium's copy of turbo does not have the mips SIMD code.

https://codereview.chromium.org/1275773004/diff/20001/gyp/libjpeg-turbo.gyp#n...
gyp/libjpeg-turbo.gyp:261: '-I../third_party/externals/libjpeg-turbo/linux/'
It becomes important that these include paths are actually correct because
jconfigsimd.h is stored in a different directory in the older version of turbo.

https://codereview.chromium.org/1275773004/diff/20001/platform_tools/android/...
File platform_tools/android/gyp/dependencies.gypi (right):

https://codereview.chromium.org/1275773004/diff/20001/platform_tools/android/...
platform_tools/android/gyp/dependencies.gypi:186: #[ 'arm_neon == 1 and
skia_clang_build == 0',
There are namespace conflicts between simd arm/neon functions in chromium's
turbo and Android's libjpeg.  I fixed these in our Skia version of turbo by
manually modifying the assembly (name mangling doesn't work).

I think this is ok as a temporary step.

https://codereview.chromium.org/1275773004/diff/20001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1275773004/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:490: // Set the jump location for libjpeg errors
Scanline decoder setjmp bug fix that is loosely related to this CL.

[msarett, 2015-08-10 17:31:40]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-10 17:31:52]

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

[commit-bot: I haz the power, 2015-08-10 17:35:45]

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

[commit-bot: I haz the power, 2015-08-10 17:35:45]

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

[scroggo, 2015-08-10 17:43:26]

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:445: #ifndef chromium_HAS_SKIP
It seems weird to me that this got changed (I guess I should have paid more
attention to your submission to chromium's version...). It would be nice if
there was a common name so that in theory someone could use the upstream version
of libjepg-turbo with Skia.

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:446: #define chromium_jpeg_skip_scanlines(dinfo,
count)                           \
This really isn't a part of this CL, but should this be #undefed below the one
place where it's used?

On another note, it seems like it does not need to be a member function,
although You had mentioned that you'd like to split this up so the storage is
only created once, in which case it does need to be a member function. Maybe
make that a TODO?

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:472: SkScanlineDecoder*
SkJpegCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
I assume this code needs to be rebased? (onGetScanlineDecoder no longer exists)

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:491: if (setjmp(codec->fDecoderMgr->getJmpBuf())) {
Why did this move?

[msarett, 2015-08-11 12:34:24]

Patchset #3 (id:80001) has been deleted

[msarett, 2015-08-11 12:37:37]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-11 12:37:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1275773004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1275773004/100001

[msarett, 2015-08-11 12:38:06]

Sorry for doing the rebase and the comments all in one upload.

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:445: #ifndef chromium_HAS_SKIP
On 2015/08/10 17:43:26, scroggo wrote:
> It seems weird to me that this got changed (I guess I should have paid more
> attention to your submission to chromium's version...). It would be nice if
> there was a common name so that in theory someone could use the upstream
version
> of libjepg-turbo with Skia.

This didn't get changed.  Find and replace turbo_ with chromium_ seems to have
backfired.  Sorry, my mistake.

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:446: #define chromium_jpeg_skip_scanlines(dinfo,
count)                           \
On 2015/08/10 17:43:26, scroggo wrote:
> This really isn't a part of this CL, but should this be #undefed below the one
> place where it's used?
> 
> On another note, it seems like it does not need to be a member function,
> although You had mentioned that you'd like to split this up so the storage is
> only created once, in which case it does need to be a member function. Maybe
> make that a TODO?

Adding an undef and a TODO.

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:472: SkScanlineDecoder*
SkJpegCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
On 2015/08/10 17:43:26, scroggo wrote:
> I assume this code needs to be rebased? (onGetScanlineDecoder no longer
exists)

Yes, the commit bots think so too.

https://codereview.chromium.org/1275773004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:491: if (setjmp(codec->fDecoderMgr->getJmpBuf())) {
On 2015/08/10 17:43:26, scroggo wrote:
> Why did this move?

We now call:
codec->fDecoderMgr->getJmpBuf()

instead of:
fDecoderMgr->getJmpBuf()

I moved this after the creation of the codec.  The old code will seg fault on
longjmp calls.  After rebasing, I noticed that this was already fixed in
upstream anyway.

[commit-bot: I haz the power, 2015-08-11 12:39:46]

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

[commit-bot: I haz the power, 2015-08-11 12:39:46]

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

[scroggo, 2015-08-11 13:22:14]

lgtm, once it's all working

[msarett, 2015-08-11 17:00:52]

I think this should be ready to land after my one line patch to libjpeg-turbo
lands in chromium.

[msarett, 2015-08-11 20:24:22]

The CQ bit was checked by msarett@google.com

[msarett, 2015-08-11 20:24:23]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1275773004/#ps160001
(title: "Update DEPS with libjpeg-turbo fix")

[commit-bot: I haz the power, 2015-08-11 20:25:03]

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

[commit-bot: I haz the power, 2015-08-11 20:33:03]

Committed patchset #6 (id:160001) as
https://skia.googlesource.com/skia/+/fcaaadee711a93d601ccc9f0b47d744e22c35205

[msarett, 2015-08-11 21:32:09]

Patchset #7 (id:180001) has been deleted

[Noel Gordon, 2015-08-17 05:17:18]

After this change, I can no longer build a stand alone skia on win per the skia
docs [1,2].

[1]
https://sites.google.com/site/skiadocs/developer-documentation/managing-chrom...
[2]
https://sites.google.com/site/skiadocs/user-documentation/quick-start-guides/...

C:\src\chrome\src\third_party\skia>python gyp_skia
GYP_GENERATORS environment variable not set, using default
GYP_GENERATORS is "ninja,msvs-ninja"
Updating projects from gyp files...
Traceback (most recent call last):
  File "gyp_skia", line 159, in <module>
    res = gyp.main(args)
  File "C:\src\chrome\src\third_party\skia\third_party\externals\gyp\pylib\gyp\_
_init__.py", line 538, in main
    return gyp_main(args)
  File "C:\src\chrome\src\third_party\skia\third_party\externals\gyp\pylib\gyp\_
_init__.py", line 523, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File "C:\src\chrome\src\third_party\skia\third_party\externals\gyp\pylib\gyp\g
enerator\ninja.py", line 2384, in GenerateOutput
    pool.map(CallGenerateOutputForConfig, arglists)
  File "C:\python_26_amd64\files\lib\multiprocessing\pool.py", line 148, in map
    return self.map_async(func, iterable, chunksize).get()
  File "C:\python_26_amd64\files\lib\multiprocessing\pool.py", line 422, in get
    raise self._value
Exception: Missing input files:
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jccolmmx.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jccolss2.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcgrammx.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcgrass2.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcqnt3dn.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcqntmmx.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcqnts2f.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcqnts2i.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcqntsse.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcsammmx.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jcsamss2.asm
C:\src\chrome\src\third_party\skia\third_party\externals\libjpeg-turbo\simd\jdcolmmx.asm

etc, etc ...

[Noel Gordon, 2015-08-17 06:23:15]

Additional win compilation breakage even after fixing the above

c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(25) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
[232/1091] CC obj\third_party\external...p\src\utils\libwebp_utils.rescaler.obj
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(17) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(21) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(25) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
[235/1091] CC obj\third_party\external...ebp\src\utils\libwebp_utils.thread.obj
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(17) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(21) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(25) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
[237/1091] CC obj\third_party\external...src\utils\libwebp_utils.bit_writer.obj
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(17) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(21) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
c:\src\chrome\src\third_party\skia\third_party\libwebp\webp\config.h(25) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline

Seems third_party\skia\third_party\libwebp\webp\config.h is being included in
the win build, and the MSVC compiler
certainly does not not understand

#ifdef __builtin_bswap16(x)
  ...
#endif

These "(x)" characters look to be illegal.

[msarett, 2015-08-17 12:37:58]

I think that the jpeg errors are not a real problem.  The error was probably
caused by the fact that the names of all the assmebly files are different in
chromium's libjpeg-turbo than in libjpeg-turbo trunk.  I think that running
"gclient sync" and then "./gyp_skia" before compiling should fix those errors. 
Is this the fix that you used?

As far as the webp errors, I don't know the answer immediately.  Leon has been
doing some work on updating our version of webp.  He is out this week, but I'll
look into it and see if I can fix the problem.

[Noel Gordon, 2015-08-17 12:52:44]

On 2015/08/17 12:37:58, msarett wrote:
> I think that the jpeg errors are not a real problem.  The error was probably
> caused by the fact that the names of all the assmebly files are different in
> chromium's libjpeg-turbo than in libjpeg-turbo trunk.  I think that running
> "gclient sync" and then "./gyp_skia" before compiling should fix those errors.

> Is this the fix that you used?

Step 1: I do [1] first

$ cd chromium/src/third_party/skia
$ tools/git-sync-deps

Step 2: I start cmd.exe dos box, and follow [2]

$ python .\gyp_skia

which runs and produces the errors about the libjpeg_turbo simd files.  The
instructions don't mention anything running a gclient sync.

Anyho, to workaround it, I reverted the patch on this CL, and repeated Steps 1 &
2 and was able to compile skia stand-alone.

> As far as the webp errors, I don't know the answer immediately.  Leon has been
> doing some work on updating our version of webp.  He is out this week, but
I'll
> look into it and see if I can fix the problem.

I workaround it as follows: surrounded the webp config.h code with

#if !defined(_WIN32)
  // existing code ...
#endif

and that allowed me to compile at least.

[msarett, 2015-08-17 13:30:31]

Gotcha thanks for the details.

I think your jpeg issue is that the "tools/git-sync-deps" script isn't fully
supported (but I do think it is the right choice to do what you're trying to
do).  In particular, it's not smart enough to deal with the fact that we
replaced third_party/externals/libjpeg-turbo with a completely different git
repository.  I think if you delete your skia/third_party/externals directory and
start over, it should work for you.

Still looking into webp.

[Noel Gordon, 2015-08-18 12:58:25]

On 2015/08/17 13:30:31, msarett wrote:
> Gotcha thanks for the details.
> 
> I think your jpeg issue is that the "tools/git-sync-deps" script isn't fully
> supported (but I do think it is the right choice to do what you're trying to
> do).  In particular, it's not smart enough to deal with the fact that we
> replaced third_party/externals/libjpeg-turbo with a completely different git
> repository.

Ok makes sense, need a clobber build ...

> I think if you delete your skia/third_party/externals directory and
> start over, it should work for you.

... so in my chrome checkout, I did a

% rm -rf src/third_party/skia && gclient sync -f -D

which gave me a clean, shiny-new src/third_party/skia

then I followed Steps 1 & 2 again, worked fine, I can built a stand-alone skia
on WIN32 again :)

> Still looking into webp.

Synced to your patch that fixed that, and webp is working for me again also. 
All good here.

[Noel Gordon, 2015-08-19 03:14:47]

One very minor issue compiling win32, filed skia bug
https://code.google.com/p/skia/issues/detail?id=4220 about it, but I am unable
to cc you, so just noting it here.