Switch SkJpegCode to libjpeg-turbo

Add libjpeg-turbo library (depends on yasm)
Mangle external function names to avoid conflict with libjpeg
Take advantage of direct color conversion (RGBA, BGRA, 565)
Prepare to use jpeg_skip_scanlines (when it is upstreamed)

BUG=skia:

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

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

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

Committed: https://skia.googlesource.com/skia/+/40141b57f061fbfcc2fa38da942d9efe25aca4d0

Committed: https://skia.googlesource.com/skia/+/1c8a587120fd33854974457cf2a795ead98183a9

[msarett, 2015-06-15 23:56:07]

Patchset #4 (id:60001) has been deleted

[msarett, 2015-06-15 23:56:21]

Patchset #3 (id:40001) has been deleted

[msarett, 2015-06-15 23:56:28]

Patchset #2 (id:20001) has been deleted

[msarett, 2015-06-15 23:56:36]

Patchset #1 (id:1) has been deleted

[msarett, 2015-06-15 23:58:28]

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

[djsollen, 2015-06-16 19:33:30]

https://codereview.chromium.org/1180983002/diff/80001/platform_tools/android/...
File platform_tools/android/bin/android_perf (right):

https://codereview.chromium.org/1180983002/diff/80001/platform_tools/android/...
platform_tools/android/bin/android_perf:44: adb_pull_if_needed
/system/lib/libstdc++.so $TMP_SYS_LIB
can you upload this as a separate patch?

[scroggo, 2015-06-16 20:39:23]

I know you mentioned this was not for landing, but I think with some cleanups it
is fine to land this. (We can always have a fallback path for
jpeg_skip_scanlines with a FIXME if this lands before we actually have that
method.)

https://codereview.chromium.org/1180983002/diff/80001/gyp/libjpeg.gyp
File gyp/libjpeg.gyp (right):

https://codereview.chromium.org/1180983002/diff/80001/gyp/libjpeg.gyp#newcode23
gyp/libjpeg.gyp:23:
'../third_party/libjpeg-turbo/libjpeg-turbo-code/libjpeg-turbo.gyp:libjpeg-turbo',
I don't see any reason not to use turbo on all platforms. (That said, for
SkImageDecoder, we'll need to continue to use the forked version of libjpeg.)

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

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:33: static void convert_CMYK_to_RGBA(uint8_t* row,
uint32_t width) {
The comment says RGBX. I think X implies A isn't there or it's ignored, whereas
here it's there but always 0xFF. I'm fine with either one, but prefer
consistency.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:146: // libjpeg supports scaling by 1/1, 1/2, 1/4, and
1/8, so we will support these as well
Update this comment?

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:149: if (desiredScale > 1.875f) {
Wait, libjpeg scales up? What is the advantage to that?

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:241: bool isCMYK = JCS_CMYK ==
fDecoderMgr->dinfo()->jpeg_color_space ||
nit: I find this more readable if you use a temporary variable:

auto jpeg_color_space = fDecoderMgr->dinfo()->jpeg_color_space;
bool isCMYK = JCS_CMYK == jpeg_color_space || JCS_YCCK == jpeg_color_space;

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:258: // TODO (msarett):
What is the TODO here?

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:266: // We will enable decodes to gray even if the
image is color because this is
Does libjpeg(-turbo) support converting full color to grayscale? Oh, I guess due
to the way jpeg is stored this is easy for them to provide? I don't know that
anyone wants this, but if it's easy to support I'm not opposed to it.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:330: SkDebugf("POOPY\n");
lol

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:331: fDecoderMgr->returnFailure("cannot scale to
requested dims", kInvalidScale);
Should this say *return* fDecoderMgr->returnFailure(...)?

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:340: SkDEBUGCODE(const uint32_t rowsPerDecode =
dinfo->rec_outbuf_height;)
This variable is never used after the assert. How about:

SkASSERT(1 == dinfo->rec_outbuf_height);

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:401: uint32_t rowsDecoded =
jpeg_read_scanlines(fCodec->fDecoderMgr->dinfo(), &dstRow, 1);
Wait, we don't need to do any conversion? (besides from CMYK) Does JCS_EXT_RGBA
output to SkPMColor? Do we need to make sure libjpeg-turbo is compiled with the
same color ordering as Skia?

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:404: fCodec->fDecoderMgr->dinfo()->output_scanline =
this->dstInfo().height();
Do we need to call jpeg_finish_decompress? (whether we got the error case or
not)

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegDecoder...
File src/codec/SkJpegDecoderMgr.cpp (right):

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegDecoder...
src/codec/SkJpegDecoderMgr.cpp:41: switch (fDInfo.jpeg_color_space) {
Isn't there an option for 565?

https://codereview.chromium.org/1180983002/diff/80001/src/images/SkImageDecod...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/1180983002/diff/80001/src/images/SkImageDecod...
src/images/SkImageDecoder_libjpeg.cpp:88: #if 0
Derek knows the magic to make SkCodec using libjpeg-turbo while SkImageDecoder
uses libjpeg. With that magic, you don't need to disable this. (The CL to do
something similar for libpng is here:
https://codereview.chromium.org/1058823002/)

[msarett, 2015-06-18 20:47:27]

Patchset #4 (id:140001) has been deleted

[msarett, 2015-06-18 20:47:35]

Patchset #3 (id:120001) has been deleted

[msarett, 2015-06-18 20:50:26]

Patchset #4 (id:180001) has been deleted

[msarett, 2015-06-18 20:56:37]

Patchset #4 (id:200001) has been deleted

[msarett, 2015-06-18 21:06:11]

I made the suggested fixes!

libjpeg-turbo.gyp is a work in progress.  I don't think it will work on all
platforms and it depends on the client having yasm installed.  Also, I had to
comment out a few files - I haven't figured out yet why they won't compile (and
chromium doesn't compile them either).

This CL depends on the following changes:

1) I defined NEED_SHORT_EXTERNAL_NAMES in skia-libjpeg and android-skia-libjpeg.

2) I also needed to prefix some function names in android-skia-libjpeg because
they weren't covered by NEED_SHORT_EXTERNAL_NAMES.

3) My current version of libjpeg-turbo.  This is tip of tree +
jpeg_skip_scanlines() + changes to jconfig.h to make it compile.

https://codereview.chromium.org/1180983002/diff/80001/gyp/libjpeg.gyp
File gyp/libjpeg.gyp (right):

https://codereview.chromium.org/1180983002/diff/80001/gyp/libjpeg.gyp#newcode23
gyp/libjpeg.gyp:23:
'../third_party/libjpeg-turbo/libjpeg-turbo-code/libjpeg-turbo.gyp:libjpeg-turbo',
On 2015/06/16 20:39:22, scroggo wrote:
> I don't see any reason not to use turbo on all platforms. (That said, for
> SkImageDecoder, we'll need to continue to use the forked version of libjpeg.)

Yes agreed.

https://codereview.chromium.org/1180983002/diff/80001/platform_tools/android/...
File platform_tools/android/bin/android_perf (right):

https://codereview.chromium.org/1180983002/diff/80001/platform_tools/android/...
platform_tools/android/bin/android_perf:44: adb_pull_if_needed
/system/lib/libstdc++.so $TMP_SYS_LIB
On 2015/06/16 19:33:29, djsollen wrote:
> can you upload this as a separate patch?

Done.

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

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:33: static void convert_CMYK_to_RGBA(uint8_t* row,
uint32_t width) {
On 2015/06/16 20:39:22, scroggo wrote:
> The comment says RGBX. I think X implies A isn't there or it's ignored,
whereas
> here it's there but always 0xFF. I'm fine with either one, but prefer
> consistency.

Done.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:146: // libjpeg supports scaling by 1/1, 1/2, 1/4, and
1/8, so we will support these as well
On 2015/06/16 20:39:22, scroggo wrote:
> Update this comment?

Done.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:149: if (desiredScale > 1.875f) {
On 2015/06/16 20:39:22, scroggo wrote:
> Wait, libjpeg scales up? What is the advantage to that?

Oops, I added this for testing jpeg_skip_scanlines().  As we've discussed, we
don't intend to support upscaling.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:241: bool isCMYK = JCS_CMYK ==
fDecoderMgr->dinfo()->jpeg_color_space ||
On 2015/06/16 20:39:22, scroggo wrote:
> nit: I find this more readable if you use a temporary variable:
> 
> auto jpeg_color_space = fDecoderMgr->dinfo()->jpeg_color_space;
> bool isCMYK = JCS_CMYK == jpeg_color_space || JCS_YCCK == jpeg_color_space;

Done.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:258: // TODO (msarett):
On 2015/06/16 20:39:22, scroggo wrote:
> What is the TODO here?

This has been done!  The original issue was caused by the fact that turbo and
patched libjpeg have different names for RGB565.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:266: // We will enable decodes to gray even if the
image is color because this is
On 2015/06/16 20:39:22, scroggo wrote:
> Does libjpeg(-turbo) support converting full color to grayscale? Oh, I guess
due
> to the way jpeg is stored this is easy for them to provide? I don't know that
> anyone wants this, but if it's easy to support I'm not opposed to it.

I have no recollection of writing this comment, and I'm not sure what made me
put this code here haha.  Having said that, I just tested and this works.  As
you said, I guess I don't see a reason to unsupport this.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:330: SkDebugf("POOPY\n");
On 2015/06/16 20:39:22, scroggo wrote:
> lol

Probably not necessary...

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:331: fDecoderMgr->returnFailure("cannot scale to
requested dims", kInvalidScale);
On 2015/06/16 20:39:22, scroggo wrote:
> Should this say *return* fDecoderMgr->returnFailure(...)?

Yes!

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:340: SkDEBUGCODE(const uint32_t rowsPerDecode =
dinfo->rec_outbuf_height;)
On 2015/06/16 20:39:22, scroggo wrote:
> This variable is never used after the assert. How about:
> 
> SkASSERT(1 == dinfo->rec_outbuf_height);

Done.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:401: uint32_t rowsDecoded =
jpeg_read_scanlines(fCodec->fDecoderMgr->dinfo(), &dstRow, 1);
On 2015/06/16 20:39:23, scroggo wrote:
> Wait, we don't need to do any conversion? (besides from CMYK) Does
JCS_EXT_RGBA
> output to SkPMColor? Do we need to make sure libjpeg-turbo is compiled with
the
> same color ordering as Skia?

Yes we do.  As it happens, this works on Android but was broken for my desktop. 
I have fixed this in setOutputColorSpace.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:404: fCodec->fDecoderMgr->dinfo()->output_scanline =
this->dstInfo().height();
On 2015/06/16 20:39:22, scroggo wrote:
> Do we need to call jpeg_finish_decompress? (whether we got the error case or
> not)

Yes we should call this!  I think I wrongly assumed onFinish() would be called,
but it won't if we never reach the end of the image.

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegDecoder...
File src/codec/SkJpegDecoderMgr.cpp (right):

https://codereview.chromium.org/1180983002/diff/80001/src/codec/SkJpegDecoder...
src/codec/SkJpegDecoderMgr.cpp:41: switch (fDInfo.jpeg_color_space) {
On 2015/06/16 20:39:23, scroggo wrote:
> Isn't there an option for 565?

This function returns the color space that we will recommend.  We never
recommend 565, but will support it if the client asks for it.

https://codereview.chromium.org/1180983002/diff/80001/src/images/SkImageDecod...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/1180983002/diff/80001/src/images/SkImageDecod...
src/images/SkImageDecoder_libjpeg.cpp:88: #if 0
On 2015/06/16 20:39:23, scroggo wrote:
> Derek knows the magic to make SkCodec using libjpeg-turbo while SkImageDecoder
> uses libjpeg. With that magic, you don't need to disable this. (The CL to do
> something similar for libpng is here:
> https://codereview.chromium.org/1058823002/)

Done.

[msarett, 2015-06-18 21:07:58]

Also I forgot to ask what the procedure is for committing changes to the
libraries that we use internally - looks like I'll need to do this.

[msarett, 2015-06-19 16:03:38]

This version is worth testing on all of our platforms because it might actually
work.

Also, I wanted to mention that it's definitely possible to prefix libjpeg-turbo
function names.  So if that is more desirable that modifying our existing
libjpegs, I can switch to that approach.

Two remaining issues with compiling turbo:
1) Android does not define __WORDSIZE so I make it default to 32.  If there are
actually Android devices that are 64-bit, performance may improve by defining
this to 64.  I am working on figuring out how to check this at compile time for
Android (no luck so far).

2) On x86 and x86_64 architectures, we need to be able to compile .asm files. 
libjpeg-turbo recommends using NASM.  Chromium uses YASM.  Currently we depend
on the system having already installed YASM (works on my desktop).  Chromium
actually has a patched version of YASM that they use in a lot of situations
depending on the OS etc.  Maybe we should use this as well?  Maybe we need to
add YASM to skia dependencies?  I probably need to track somebody down to ask
about this.

[scroggo, 2015-06-19 16:11:25]

On 2015/06/18 21:06:11, msarett wrote:
> I made the suggested fixes!
> 
> libjpeg-turbo.gyp is a work in progress.  I don't think it will work on all
> platforms

Why not?

> and it depends on the client having yasm installed.

Even better - add yasm to DEPS, and we'll build it on the client. (Separately,
we'll need to make sure the Android repo includes yasm.)

> Also, I had to
> comment out a few files - I haven't figured out yet why they won't compile
(and
> chromium doesn't compile them either).

If chromium doesn't compile them, and we can build and get the functionality we
need without them, I wouldn't worry too much about them. (Do they look like
optimizations for specific platforms?)

On 2015/06/18 21:07:58, msarett wrote:
> Also I forgot to ask what the procedure is for committing changes to the
> libraries that we use internally - looks like I'll need to do this.

Do you need to modify the libraries themselves? There are a couple of ways to do
it:

- mirror in GoogleSource
- check the code directly into Skia's third_party directory

Either way, I think you need to add a README.XXX (Chromium uses README.chromium,
perhaps ours should be README.skia) which lists the differences/license info and
references a patch file with our differences. For an example, look at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe...

Shouldn't there be a change to the DEPS file?

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

https://codereview.chromium.org/1180983002/diff/220001/gyp/libjpeg-turbo.gyp#...
gyp/libjpeg-turbo.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
I started to correct this with a Skia copyright (we typically copyright as
Google Inc), but if this file was pulled from Chromium, this may be the right
way to go.

https://codereview.chromium.org/1180983002/diff/220001/gyp/libjpeg-turbo.gyp#...
gyp/libjpeg-turbo.gyp:6: # This file is not used when use_system_libjpeg==1.
Settings for building with
When do we ever want to build with the system libjpeg?  (I guess we do that for
building SkImageDecoder, which we'll do for comparison as long as it's still
around?)

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

https://codereview.chromium.org/1180983002/diff/220001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:76: row[0] = SkMulDiv255Round(row[0], row[3]);
I just realized this *also* assumes a particular ordering (I think this was
previously okay, because we would swizzle afterwards, but if we are not
swizzling, this will need to output in the right order).

[scroggo, 2015-06-19 17:35:35]

On 2015/06/19 16:03:38, msarett wrote:
> This version is worth testing on all of our platforms because it might
actually
> work.
> 
> Also, I wanted to mention that it's definitely possible to prefix
libjpeg-turbo
> function names.  So if that is more desirable that modifying our existing
> libjpegs, I can switch to that approach.

We already do the prefix trick in https://codereview.chromium.org/1058823002/
(https://skia.googlesource.com/skia/+/master/third_party/libpng/)

I'm not sure which is more desirable.

> 
> Two remaining issues with compiling turbo:
> 1) Android does not define __WORDSIZE so I make it default to 32.  If there
are
> actually Android devices that are 64-bit, performance may improve by defining
> this to 64.  I am working on figuring out how to check this at compile time
for
> Android (no luck so far).

I am pretty sure there are 64 bit Android devices. Not sure the flag to
determine this.

> 
> 2) On x86 and x86_64 architectures, we need to be able to compile .asm files. 
> libjpeg-turbo recommends using NASM.  Chromium uses YASM.  Currently we depend
> on the system having already installed YASM (works on my desktop).  Chromium
> actually has a patched version of YASM that they use in a lot of situations
> depending on the OS etc.  Maybe we should use this as well? 

Yes.

> Maybe we need to
> add YASM to skia dependencies? 

Yes.

> I probably need to track somebody down to ask
> about this.

Does Chromium pull it in via DEPS? I would think that would be the right way to
do it. Alternatively, we can check it directly into Skia's third_party.

[msarett, 2015-06-22 22:21:53]

Patchset #6 (id:260001) has been deleted

[msarett, 2015-06-22 22:36:04]

This CL has a few small changes to the source code and a lot of changes to the
dependencies.

Source Changes:

1) I fixed the CMYK-to-RGBA conversion to work for multiple RGBA orderings.

2) I changed the implementation of skipScanlines() to rely on tip of tree turbo
WITHOUT my patch.  I plan to change it to my implementation after it is
upstreamed (hopefully).

Dependency Changes:

1) I have pushed tip of tree libjpeg-turbo to third_party/libjpeg-turbo, so the
remaining code in SkJpegCodec that depends on turbo should be fine.

2) turbo depends on yasm.  I added yasm binaries and source code to
third_party/externals via the DEPS file.  These are exactly the same as chromium
DEPS.

3) yasm also requires config files for different architectures.  I added these
(copied from chromium) to third_party/yasm.

4) I managed to make changes that mangle the libjpeg function names, so they do
not conflict with turbo, without modifying anything in externals directories. 
The change is straight-forward for non-Android and a bit hacky for Android.

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

https://codereview.chromium.org/1180983002/diff/220001/gyp/libjpeg-turbo.gyp#...
gyp/libjpeg-turbo.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2015/06/19 16:11:25, scroggo wrote:
> I started to correct this with a Skia copyright (we typically copyright as
> Google Inc), but if this file was pulled from Chromium, this may be the right
> way to go.

Yeah I'm certainly adding modifications, but I think its accurate to credit
Chromium with this file.

https://codereview.chromium.org/1180983002/diff/220001/gyp/libjpeg-turbo.gyp#...
gyp/libjpeg-turbo.gyp:6: # This file is not used when use_system_libjpeg==1.
Settings for building with
On 2015/06/19 16:11:25, scroggo wrote:
> When do we ever want to build with the system libjpeg?  (I guess we do that
for
> building SkImageDecoder, which we'll do for comparison as long as it's still
> around?)

I don't think we ever do.  I need to remove this comment - this is a copy paste
error.

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

https://codereview.chromium.org/1180983002/diff/220001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:76: row[0] = SkMulDiv255Round(row[0], row[3]);
On 2015/06/19 16:11:25, scroggo wrote:
> I just realized this *also* assumes a particular ordering (I think this was
> previously okay, because we would swizzle afterwards, but if we are not
> swizzling, this will need to output in the right order).

Nice catch, you're right!

[scroggo, 2015-06-23 15:05:52]

On 2015/06/22 22:36:04, msarett wrote:
> This CL has a few small changes to the source code and a lot of changes to the
> dependencies.
> 
> Source Changes:
> 
> 1) I fixed the CMYK-to-RGBA conversion to work for multiple RGBA orderings.
> 
> 2) I changed the implementation of skipScanlines() to rely on tip of tr1948
ee turbo
> WITHOUT my patch.  I plan to change it to my implementation after it is
> upstreamed (hopefully).
> 
> Dependency Changes:
> 
> 1) I have pushed tip of tree libjpeg-turbo to third_party/libjpeg-turbo, so
the
> remaining code in SkJpegCodec that depends on turbo should be fine.

When you say tip of tree turbo, do you mean the version that Chromium is using?
Or true tip of tree?

> 
> 2) turbo depends on yasm.  I added yasm binaries and source code to
> third_party/externals via the DEPS file.  These are exactly the same as
chromium
> DEPS.
> 
> 3) yasm also requires config files for different architectures.  I added these
> (copied from chromium) to third_party/yasm.

This seems like a lot. Is there a way to autogenerate or have some kind of
mirror?

> 
> 4) I managed to make changes that mangle the libjpeg function names, so they
do
> not conflict with turbo, without modifying anything in externals directories. 
> The change is straight-forward for non-Android and a bit hacky for Android.

Does this work for the Android framework?

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

https://codereview.chromium.org/1180983002/diff/290001/gyp/libjpeg-turbo.gyp#...
gyp/libjpeg-turbo.gyp:190: # Build rules for an asm file.
This is messy... Is this the way Chromium does it?

https://codereview.chromium.org/1180983002/diff/290001/platform_tools/android...
File platform_tools/android/third_party/jpeg/README.skia (right):

https://codereview.chromium.org/1180983002/diff/290001/platform_tools/android...
platform_tools/android/third_party/jpeg/README.skia:1: On Android, we pull in an
outdated copy of libjpeg to platform_tools/third_party/externals/jpeg.
Maybe instead of "an outdated copy of libjpeg", this should say that we pull in
a copy with customizations that SkImageDecoder depends on?

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

https://codereview.chromium.org/1180983002/diff/290001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:424: // TODO (msarett): Upstream jpeg_skip_scanlines()
so we can this faster approach
nit: we can use*

Maybe file a bug to track this, and reference the bug here?

I'm wondering if we should write a local version of jpeg_skip_scanlines, which
"skips" by writing to garbage memory. That way, when you do get
jpeg_skip_scanlines upstreamed, you'll have a smaller change to make here (just
delete the local function). You could even take that a step further and use a
macro:

#ifndef jpeg_skip_scanlines
  #define jpeg_skip_scanlines <this implementation>
#endif

That way you don't even need to change your code at all, and it will work
whether it's used with a version of libjpeg-turbo with jpeg_skip_scanlines, or
even the original libjpeg. (The downside to using the macro is that we could
accidentally use the wrong version, but maybe that's worth it to be able to work
regardless of what libjpeg is being used.)

One (potential) downside is that if we want to keep the same signature, we would
need to move fStorage from a member variable to a local variable inside
jpeg_skip_scanlines. If skipScanlines is called more than once with the same
scanlineDecoder, this would mean allocating the memory for each call (although
perhaps that's not a common use case anyway). (On the other hand, if the
scanline decoder were long lived, it would mean we only had the memory allocated
when we need it, but I don't think that's really a concern either.)

https://codereview.chromium.org/1180983002/diff/290001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:445: SkAutoMalloc        fStorage;
If you leave these as member variables, maybe add the TODO to the other
references to them, since they are only needed until we upstream
jpeg_skip_scanlines.

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/READM...
File third_party/yasm/README.skia (right):

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/READM...
third_party/yasm/README.skia:6: the yasm is pulled into externals via the DEPS
file.  Chromium manually generates these files, so
nit: "the yasm library"? Or just "yasm"? "the yasm" sounds funny to me (maybe if
I knew what yasm stood for it would sound more natural).

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/READM...
third_party/yasm/README.skia:7: we will directly check in our own copies.
Should we generate them, too? Why do we do something different?

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/confi...
File third_party/yasm/config/android/Makefile (right):

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/confi...
third_party/yasm/config/android/Makefile:36: # $Id: Makefile.inc 1948 2007-09-13
02:53:30Z peter $
Are these all comments? Are they all necessary?

[msarett, 2015-06-23 17:47:43]

One major outstanding problem which is that this almost definitely breaks the
Android framework.  I am currently looking into options to fix this and setting
up an environment to test this on.

-----------------------

“When you say tip of tree turbo, do you mean the version that Chromium is using?
 Or true tip of tree?”

I mean true tip of tree.

“This seems like a lot. Is there a way to autogenerate or have some kind of
mirror?”

Chromium explains how these files are generated here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/yasm/R...

I will include this info in the README.  The process seems manual and seems like
it is a pain.  I simply copied these files from Chromium.  It would be better to
mirror them - but if that is possible I don’t know how.  This is not a part of
the yasm git repos that we are already mirroring.

“Does this work for the Android framework?”

We discussed in person.  This almost definitely breaks the Android framework. 
This is not fixed yet.

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

https://codereview.chromium.org/1180983002/diff/290001/gyp/libjpeg-turbo.gyp#...
gyp/libjpeg-turbo.gyp:190: # Build rules for an asm file.
On 2015/06/23 15:05:51, scroggo wrote:
> This is messy... Is this the way Chromium does it?

Yes this is messy - it's been quite frustrating.  But this is exactly how
Chromium does it :).

https://codereview.chromium.org/1180983002/diff/290001/platform_tools/android...
File platform_tools/android/third_party/jpeg/README.skia (right):

https://codereview.chromium.org/1180983002/diff/290001/platform_tools/android...
platform_tools/android/third_party/jpeg/README.skia:1: On Android, we pull in an
outdated copy of libjpeg to platform_tools/third_party/externals/jpeg.
On 2015/06/23 15:05:51, scroggo wrote:
> Maybe instead of "an outdated copy of libjpeg", this should say that we pull
in
> a copy with customizations that SkImageDecoder depends on?

Done.

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

https://codereview.chromium.org/1180983002/diff/290001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:424: // TODO (msarett): Upstream jpeg_skip_scanlines()
so we can this faster approach
On 2015/06/23 15:05:51, scroggo wrote:
> nit: we can use*
> 
> Maybe file a bug to track this, and reference the bug here?
> 
> I'm wondering if we should write a local version of jpeg_skip_scanlines, which
> "skips" by writing to garbage memory. That way, when you do get
> jpeg_skip_scanlines upstreamed, you'll have a smaller change to make here
(just
> delete the local function). You could even take that a step further and use a
> macro:
> 
> #ifndef jpeg_skip_scanlines
>   #define jpeg_skip_scanlines <this implementation>
> #endif
> 
> That way you don't even need to change your code at all, and it will work
> whether it's used with a version of libjpeg-turbo with jpeg_skip_scanlines, or
> even the original libjpeg. (The downside to using the macro is that we could
> accidentally use the wrong version, but maybe that's worth it to be able to
work
> regardless of what libjpeg is being used.)
> 
> One (potential) downside is that if we want to keep the same signature, we
would
> need to move fStorage from a member variable to a local variable inside
> jpeg_skip_scanlines. If skipScanlines is called more than once with the same
> scanlineDecoder, this would mean allocating the memory for each call (although
> perhaps that's not a common use case anyway). (On the other hand, if the
> scanline decoder were long lived, it would mean we only had the memory
allocated
> when we need it, but I don't think that's really a concern either.)

I like the suggestion of using a local version of jpeg_skip_scanlines().  I am
getting tired of adding and removing those fields and making this change.  I was
previously worried about allocating new memory in each call to skip, but, as
you've mentioned, there is not a realistic use case that does this anyway.

https://codereview.chromium.org/1180983002/diff/290001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:445: SkAutoMalloc        fStorage;
On 2015/06/23 15:05:51, scroggo wrote:
> If you leave these as member variables, maybe add the TODO to the other
> references to them, since they are only needed until we upstream
> jpeg_skip_scanlines.

Removing these fields.

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/READM...
File third_party/yasm/README.skia (right):

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/READM...
third_party/yasm/README.skia:6: the yasm is pulled into externals via the DEPS
file.  Chromium manually generates these files, so
On 2015/06/23 15:05:51, scroggo wrote:
> nit: "the yasm library"? Or just "yasm"? "the yasm" sounds funny to me (maybe
if
> I knew what yasm stood for it would sound more natural).

Done.

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/READM...
third_party/yasm/README.skia:7: we will directly check in our own copies.
On 2015/06/23 15:05:51, scroggo wrote:
> Should we generate them, too? Why do we do something different?

Sorry this is unclear.  Chromium has manually generated these for all platforms
and then checked them in.  We are simply copying these from Chromium to avoid
doing that work again.

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/confi...
File third_party/yasm/config/android/Makefile (right):

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/confi...
third_party/yasm/config/android/Makefile:36: # $Id: Makefile.inc 1948 2007-09-13
02:53:30Z peter $
On 2015/06/23 15:05:51, scroggo wrote:
> Are these all comments? Are they all necessary?

I copied this file directly from chromium (I'm not sure I made this clear).  No
idea why all these comments are here, but it doesn't seem worth it to go through
and delete them all?

[msarett, 2015-06-23 17:48:13]

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

[commit-bot: I haz the power, 2015-06-23 17:48:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/300001

[commit-bot: I haz the power, 2015-06-23 17:50:02]

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

[commit-bot: I haz the power, 2015-06-23 17:50:04]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[msarett, 2015-06-23 17:58:29]

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

[commit-bot: I haz the power, 2015-06-23 17:58:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/310001

[commit-bot: I haz the power, 2015-06-23 18:01:17]

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

[commit-bot: I haz the power, 2015-06-23 18:01:19]

Dry run: Exceeded global retry quota

[msarett, 2015-06-23 18:20:02]

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

[commit-bot: I haz the power, 2015-06-23 18:20:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/320001

[commit-bot: I haz the power, 2015-06-23 18:23:06]

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

[commit-bot: I haz the power, 2015-06-23 18:23:07]

Dry run: Exceeded global retry quota

[msarett, 2015-06-23 18:53:41]

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

[commit-bot: I haz the power, 2015-06-23 18:54:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/330001

[commit-bot: I haz the power, 2015-06-23 18:56:21]

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

[commit-bot: I haz the power, 2015-06-23 18:56:23]

Dry run: Exceeded global retry quota

[msarett, 2015-06-23 19:01:24]

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

[commit-bot: I haz the power, 2015-06-23 19:01:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/340001

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

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

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

Dry run: Exceeded global retry quota

[scroggo, 2015-06-23 19:08:35]

On Tue, Jun 23, 2015 at 1:47 PM, <msarett@google.com> wrote:
> I will include this info in the README.  The process seems manual and seems
like
> it is a pain.  I simply copied these files from Chromium.  It would be better
to
> mirror them - but if that is possible I don’t know how.  This is not a part of
> the yasm git repos that we are already mirroring.

I believe it is possible, but I'll feel more confident in saying that once I do
a similar thing for https://codereview.chromium.org/1178013008/. It just means
creating a new repository which is a copy of these files. One advantage is that
it separates this code from Skia proper.

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/confi...
File third_party/yasm/config/android/Makefile (right):

https://codereview.chromium.org/1180983002/diff/290001/third_party/yasm/confi...
third_party/yasm/config/android/Makefile:36: # $Id: Makefile.inc 1948 2007-09-13
02:53:30Z peter $
On 2015/06/23 17:47:43, msarett wrote:
> On 2015/06/23 15:05:51, scroggo wrote:
> > Are these all comments? Are they all necessary?
> 
> I copied this file directly from chromium (I'm not sure I made this clear). 
No
> idea why all these comments are here, but it doesn't seem worth it to go
through
> and delete them all?

Haha, fair enough.

https://codereview.chromium.org/1180983002/diff/300001/platform_tools/android...
File platform_tools/android/third_party/jpeg/README.skia (right):

https://codereview.chromium.org/1180983002/diff/300001/platform_tools/android...
platform_tools/android/third_party/jpeg/README.skia:2: This is located at
platform_tools/third_party/externals/jpeg ans is pulled in via the DEPS file.
nit: and*

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

https://codereview.chromium.org/1180983002/diff/300001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:418: // TODO (msarett): Remove this function when it
is no longer necessary.
Alternatively, what do you think about making this a macro that is only defined
if jpeg_skip_scanlines is not already defined?

https://codereview.chromium.org/1180983002/diff/300001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:419: JDIMENSION
jpeg_skip_scanlines(jpeg_decompress_struct* dinfo, JDIMENSION count) {
Shouldn't this return something?

Also, it seems like it does not need to be a member function?

[msarett, 2015-06-24 15:32:25]

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

[commit-bot: I haz the power, 2015-06-24 15:32:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/350001

[commit-bot: I haz the power, 2015-06-24 15:34:07]

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

[commit-bot: I haz the power, 2015-06-24 15:34:09]

Dry run: Exceeded global retry quota

[msarett, 2015-06-24 15:44:29]

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

[commit-bot: I haz the power, 2015-06-24 15:44:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/360001

[commit-bot: I haz the power, 2015-06-24 15:49:24]

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

[commit-bot: I haz the power, 2015-06-24 15:49:25]

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...)
  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-06-24 16:54:17]

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

[commit-bot: I haz the power, 2015-06-24 16:54:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/370001

[commit-bot: I haz the power, 2015-06-24 16:59:11]

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

[commit-bot: I haz the power, 2015-06-24 16:59:13]

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...)
  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-06-24 17:14:47]

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

[commit-bot: I haz the power, 2015-06-24 17:15:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/390001

[commit-bot: I haz the power, 2015-06-24 17:23:29]

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

[commit-bot: I haz the power, 2015-06-24 17:23:30]

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...)

[msarett, 2015-06-24 17:28:42]

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

[commit-bot: I haz the power, 2015-06-24 17:29:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/400001

[msarett, 2015-06-24 17:36:15]

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

[commit-bot: I haz the power, 2015-06-24 17:36:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/410001

[commit-bot: I haz the power, 2015-06-24 17:42:32]

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

[commit-bot: I haz the power, 2015-06-24 17:42:35]

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...)
  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-06-24 18:28:28]

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

[commit-bot: I haz the power, 2015-06-24 18:28:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/460001

[commit-bot: I haz the power, 2015-06-24 18:33:18]

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

[commit-bot: I haz the power, 2015-06-24 18:33:19]

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...)

[msarett, 2015-06-24 18:53:38]

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

[commit-bot: I haz the power, 2015-06-24 18:53:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/490001

[commit-bot: I haz the power, 2015-06-24 18:58:36]

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

[commit-bot: I haz the power, 2015-06-24 18:58:38]

Dry run: 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-06-24 19:43:43]

Patchset #30 (id:510001) has been deleted

[msarett, 2015-06-24 19:44:08]

Patchset #10 (id:310001) has been deleted

[msarett, 2015-06-24 19:44:41]

Patchset #29 (id:500001) has been deleted

[msarett, 2015-06-24 19:44:49]

Patchset #28 (id:490001) has been deleted

[msarett, 2015-06-24 19:44:56]

Patchset #27 (id:480001) has been deleted

[msarett, 2015-06-24 19:45:03]

Patchset #26 (id:470001) has been deleted

[msarett, 2015-06-24 19:49:14]

Patchset #25 (id:460001) has been deleted

[msarett, 2015-06-24 19:49:20]

Patchset #24 (id:450001) has been deleted

[msarett, 2015-06-24 19:49:25]

Patchset #23 (id:440001) has been deleted

[msarett, 2015-06-24 19:49:31]

Patchset #22 (id:430001) has been deleted

[msarett, 2015-06-24 19:49:44]

Patchset #21 (id:420001) has been deleted

[msarett, 2015-06-24 19:49:58]

Patchset #20 (id:410001) has been deleted

[msarett, 2015-06-24 19:50:04]

Patchset #19 (id:400001) has been deleted

[msarett, 2015-06-24 19:50:16]

Patchset #18 (id:390001) has been deleted

[msarett, 2015-06-24 19:50:22]

Patchset #17 (id:380001) has been deleted

[msarett, 2015-06-24 19:50:36]

Patchset #16 (id:370001) has been deleted

[msarett, 2015-06-24 19:50:47]

Patchset #15 (id:360001) has been deleted

[msarett, 2015-06-24 19:50:58]

Patchset #14 (id:350001) has been deleted

[msarett, 2015-06-24 19:51:05]

Patchset #13 (id:340001) has been deleted

[msarett, 2015-06-24 19:51:12]

Patchset #12 (id:330001) has been deleted

[msarett, 2015-06-24 19:51:18]

Patchset #11 (id:320001) has been deleted

[msarett, 2015-06-24 19:51:25]

Patchset #10 (id:310001) has been deleted

[msarett, 2015-06-24 19:53:57]

Patchset #10 (id:520001) has been deleted

[msarett, 2015-06-24 19:55:05]

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

[commit-bot: I haz the power, 2015-06-24 19:55:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/530001

[commit-bot: I haz the power, 2015-06-24 20:01:28]

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

[commit-bot: I haz the power, 2015-06-24 20:01:30]

Dry run: This issue passed the CQ dry run.

[msarett, 2015-06-24 20:07:09]

Sorry for the spam emails all day.

I switched to mangling turbo function names to avoid issues with the Android
framework.

I also (finally) got this code to work on all platforms.

There is a still a question of if we want to create a separate git repository
for the third_party/yasm/config files that are copied directly from Chromium. 
Otherwise, I think this is ready to go.

https://codereview.chromium.org/1180983002/diff/300001/platform_tools/android...
File platform_tools/android/third_party/jpeg/README.skia (right):

https://codereview.chromium.org/1180983002/diff/300001/platform_tools/android...
platform_tools/android/third_party/jpeg/README.skia:2: This is located at
platform_tools/third_party/externals/jpeg ans is pulled in via the DEPS file.
On 2015/06/23 19:08:35, scroggo wrote:
> nit: and*

I removed this file.

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

https://codereview.chromium.org/1180983002/diff/300001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:418: // TODO (msarett): Remove this function when it
is no longer necessary.
On 2015/06/23 19:08:35, scroggo wrote:
> Alternatively, what do you think about making this a macro that is only
defined
> if jpeg_skip_scanlines is not already defined?

Works for me.

https://codereview.chromium.org/1180983002/diff/300001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:419: JDIMENSION
jpeg_skip_scanlines(jpeg_decompress_struct* dinfo, JDIMENSION count) {
On 2015/06/23 19:08:35, scroggo wrote:
> Shouldn't this return something?
> 
> Also, it seems like it does not need to be a member function?

Done.

[scroggo, 2015-06-24 21:08:49]

> WIP: Use libjpeg-turbo and jpeg_skip_scanlines() from SkJpegCodec
> A look at how SkJpegCodec would use libjpeg-turbo

Please update the description before submission.

I'd like to get Derek's input on submitting the yasm code directly, but
otherwise this lgtm.

https://codereview.chromium.org/1180983002/diff/540001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1180983002/diff/540001/gyp/codec.gyp#newcode25
gyp/codec.gyp:25: 'libjpeg-turbo.gyp:libjpeg-turbo',
Why does this need to be exported?

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

https://codereview.chromium.org/1180983002/diff/540001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:77: #if defined(SK_PMCOLOR_IS_RGBA)
It occurred to me that while we currently only support kN32 which maps to either
kRGBA_8888_SkColorType or kBGRA_8888_SkColorType, it would be nice to support
both: i.e. we should still suggest kN32, but it the client requests the other it
should be simple to give it to them (the question is how to test - not sure
whether we can draw the non-native colortype).

https://codereview.chromium.org/1180983002/diff/540001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:177: jpeg_decompress_struct dinfo;
Does this need to be prefixed, too?

https://codereview.chromium.org/1180983002/diff/540001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:238: #if defined(SK_PMCOLOR_IS_RGBA)
Can you move this code into the else statement of case kN32 below?

https://codereview.chromium.org/1180983002/diff/540001/src/codec/SkJpegDecode...
File src/codec/SkJpegDecoderMgr.cpp (right):

https://codereview.chromium.org/1180983002/diff/540001/src/codec/SkJpegDecode...
src/codec/SkJpegDecoderMgr.cpp:40: SkColorType JpegDecoderMgr::getColorType() {
nit: Maybe this should be getRecommendedColorType?

[djsollen, 2015-06-24 21:22:33]

lgtm

I'm fine landing the yasm configs here for now until we can find a better
solution, but I don't see one at the moment.

[msarett, 2015-06-24 21:56:54]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-06-24 21:57:19]

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

[commit-bot: I haz the power, 2015-06-24 22:10:30]

Committed patchset #11 (id:540001) as
https://skia.googlesource.com/skia/+/b60c3f8291529303299262dba19b1a896060bd2d

[mtklein, 2015-06-24 22:14:08]

A revert of this CL (patchset #11 id:540001) has been created in
https://codereview.chromium.org/1199253006/ by mtklein@google.com.

The reason for reverting is:
https://uberchromegw.corp.google.com/i/client.skia.compile/builders/Build-Ubu....

[mtklein, 2015-06-24 22:14:44]

On 2015/06/24 22:14:08, mtklein wrote:
> A revert of this CL (patchset #11 id:540001) has been created in
> https://codereview.chromium.org/1199253006/ by mailto:mtklein@google.com.
> 
> The reason for reverting is:
>
https://uberchromegw.corp.google.com/i/client.skia.compile/builders/Build-Ubu....

Also,

http://build.chromium.org/p/client.skia.android/builders/Test-Android-GCC-Nex...

[mtklein, 2015-06-24 22:19:57]

On 2015/06/24 22:14:44, mtklein wrote:
> On 2015/06/24 22:14:08, mtklein wrote:
> > A revert of this CL (patchset #11 id:540001) has been created in
> > https://codereview.chromium.org/1199253006/ by mailto:mtklein@google.com.
> > 
> > The reason for reverting is:
> >
>
https://uberchromegw.corp.google.com/i/client.skia.compile/builders/Build-Ubu....
> 
> Also,
> 
>
http://build.chromium.org/p/client.skia.android/builders/Test-Android-GCC-Nex...

Tons of other various failures, a GYP, build, or runtime.  o_O

[msarett, 2015-06-25 20:15:02]

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

[msarett, 2015-06-25 20:15:03]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1180983002/#ps550001
(title: "Switch to libjpeg-turbo trunk and broken bot fixes")

[commit-bot: I haz the power, 2015-06-25 20:15:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/550001

[commit-bot: I haz the power, 2015-06-25 20:18:18]

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

[commit-bot: I haz the power, 2015-06-25 20:18:20]

Dry run: Try jobs failed on following builders:
  Build-Mac10.8-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.8-Clang-...)

[msarett, 2015-06-26 17:21:09]

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

[msarett, 2015-06-26 17:21:09]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1180983002/#ps620001
(title: "Android x86 fix")

[commit-bot: I haz the power, 2015-06-26 17:21:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/620001

[commit-bot: I haz the power, 2015-06-26 17:22:11]

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

[commit-bot: I haz the power, 2015-06-26 17:22:13]

Dry run: Exceeded global retry quota

[msarett, 2015-06-26 18:06:57]

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

[msarett, 2015-06-26 18:06:57]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1180983002/#ps630001
(title: "Removing yasm binary for testing")

[commit-bot: I haz the power, 2015-06-26 18:07:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/630001

[commit-bot: I haz the power, 2015-06-26 18:26:27]

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

[commit-bot: I haz the power, 2015-06-26 18:26:29]

Dry run: This issue passed the CQ dry run.

[msarett, 2015-06-26 19:34:14]

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

[msarett, 2015-06-26 19:34:15]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1180983002/#ps640001
(title: "Turn off codec on skia android framework")

[commit-bot: I haz the power, 2015-06-26 19:34:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180983002/640001

[msarett, 2015-06-26 20:04:15]

Major changes:

1) The DEPS files now pulls in libjpeg-turbo trunk which has my skip scanlines
function.
2) SkJpegCodec is disabled on the android framework.
3) I think this will compile on all platforms.

If this looks good to you, I will directly check in the yasm binary that we need
for x86 Android and then check this in (and cross my fingers).

[scroggo, 2015-06-29 14:05:51]

LGTM

Just so I understand, what is the plan for enabling on Android framework? Will
we have two libjpegs side by side?

https://codereview.chromium.org/1180983002/diff/670001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1180983002/diff/670001/gyp/codec.gyp#newcode66
gyp/codec.gyp:66: # TODO(msarett): update libjpeg-turbo on Android so we can
compile SkJpegCodec
Will Android then have both libjpeg and libjpeg-turbo? I just want to make sure
I understand the plan.

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

https://codereview.chromium.org/1180983002/diff/670001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:239: J_COLOR_SPACE outRGBA = JCS_EXT_RGBA;
It seems like this is only needed if the dst is kN32, and !isCMYK?

https://codereview.chromium.org/1180983002/diff/670001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:419: // Remove this macro after all platforms have an
up to date version of libjpeg-turbo
I think we should remove this comment. After thinking about it more, I think it
makes sense to always have the ifdef, so our library will work whether the
client is using turbo with skip, turbo without skip, or even regular libjpeg.

[msarett, 2015-06-29 16:52:04]

Yeah the plan is to check in libjpeg-turbo to Android (maybe AOSP).  That way we
can use both SkCodec and SkImageDecoder on Android.  Then, if we can replace the
use of SkImageDecoder with SkCodec, we can hopefully remove libjpeg.

https://codereview.chromium.org/1180983002/diff/670001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1180983002/diff/670001/gyp/codec.gyp#newcode66
gyp/codec.gyp:66: # TODO(msarett): update libjpeg-turbo on Android so we can
compile SkJpegCodec
On 2015/06/29 14:05:50, scroggo wrote:
> Will Android then have both libjpeg and libjpeg-turbo? I just want to make
sure
> I understand the plan.

Yeah the plan is to check in libjpeg-turbo to Android (maybe AOSP).  That way we
can use both SkCodec and SkImageDecoder on Android.  Then, if we can replace the
use of SkImageDecoder with SkCodec, we can hopefully remove libjpeg.

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

https://codereview.chromium.org/1180983002/diff/670001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:239: J_COLOR_SPACE outRGBA = JCS_EXT_RGBA;
On 2015/06/29 14:05:50, scroggo wrote:
> It seems like this is only needed if the dst is kN32, and !isCMYK?

You're right.  I will move this to that specific case.

https://codereview.chromium.org/1180983002/diff/670001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:419: // Remove this macro after all platforms have an
up to date version of libjpeg-turbo
On 2015/06/29 14:05:50, scroggo wrote:
> I think we should remove this comment. After thinking about it more, I think
it
> makes sense to always have the ifdef, so our library will work whether the
> client is using turbo with skip, turbo without skip, or even regular libjpeg.

Agreed. Removing.

[msarett, 2015-06-29 16:52:17]

The CQ bit was checked by msarett@google.com

[msarett, 2015-06-29 16:52:18]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com,
scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1180983002/#ps680001
(title: "Minor fixes")

[commit-bot: I haz the power, 2015-06-29 16:52:30]

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

[commit-bot: I haz the power, 2015-06-29 17:14:16]

Committed patchset #25 (id:680001) as
https://skia.googlesource.com/skia/+/f8bf9181d7b0463c8e371755cfbb9ece90b34fc5

[msarett, 2015-06-29 17:30:33]

A revert of this CL (patchset #25 id:680001) has been created in
https://codereview.chromium.org/1214023003/ by msarett@google.com.

The reason for reverting is: yasm is leaking memory :(.

https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G....

[mtklein, 2015-06-29 18:08:40]

On 2015/06/29 17:30:33, msarett wrote:
> A revert of this CL (patchset #25 id:680001) has been created in
> https://codereview.chromium.org/1214023003/ by mailto:msarett@google.com.
> 
> The reason for reverting is: yasm is leaking memory :(.
> 
>
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G....

(It's totally fine to disable ASAN for yasm.  We just want to make sure Skia,
nanobench, and DM are built with it.)

[msarett, 2015-07-01 17:42:50]

Patchset #29 (id:720001) has been deleted

[msarett, 2015-07-01 19:27:14]

The CQ bit was checked by msarett@google.com

[msarett, 2015-07-01 19:27:15]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1180983002/#ps710001
(title: "Merge")

[commit-bot: I haz the power, 2015-07-01 19:27:27]

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

[commit-bot: I haz the power, 2015-07-01 19:36:24]

Committed patchset #28 (id:710001) as
https://skia.googlesource.com/skia/+/e9e3ee33f30c14c31afd5fc3fe4dda7f15783c75

[msarett, 2015-07-01 20:10:42]

A revert of this CL (patchset #28 id:710001) has been created in
https://codereview.chromium.org/1213093003/ by msarett@google.com.

The reason for reverting is: Broke iOS build..

[msarett, 2015-07-01 20:58:45]

Patchset #29 (id:730001) has been deleted

[msarett, 2015-07-01 21:25:48]

Patchset #29 (id:740001) has been deleted

[msarett, 2015-07-01 21:33:04]

The CQ bit was checked by msarett@google.com

[msarett, 2015-07-01 21:33:05]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1180983002/#ps750001
(title: "Fix ios")

[commit-bot: I haz the power, 2015-07-01 21:33:17]

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

[commit-bot: I haz the power, 2015-07-01 21:41:09]

Committed patchset #29 (id:750001) as
https://skia.googlesource.com/skia/+/40141b57f061fbfcc2fa38da942d9efe25aca4d0

[jvanverth1, 2015-07-02 17:35:59]

A revert of this CL (patchset #29 id:750001) has been created in
https://codereview.chromium.org/1226543003/ by jvanverth@google.com.

The reason for reverting is: DEPS roll failing.

[scroggo, 2015-07-07 14:12:47]

On 2015/07/02 17:35:59, jvanverth1 wrote:
> A revert of this CL (patchset #29 id:750001) has been created in
> https://codereview.chromium.org/1226543003/ by mailto:jvanverth@google.com.
> 
> The reason for reverting is: DEPS roll failing.

Was this believed to fix the DEPS roll? (which one? It looks like the revert was
included in https://codereview.chromium.org/1214943004/, which still did not
succeed.)

Matt, is there any reason not to reland this?

(I would be surprised if this affected Chromium at all, which is not using
SkCodec.)

[msarett, 2015-07-07 15:28:46]

Patchset #31 (id:780001) has been deleted

[msarett, 2015-07-07 15:29:01]

Patchset #30 (id:760001) has been deleted

[msarett, 2015-07-07 15:36:37]

The CQ bit was checked by msarett@google.com

[msarett, 2015-07-07 15:36:38]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1180983002/#ps800001
(title: "Fix xcode builds")

[commit-bot: I haz the power, 2015-07-07 15:36:53]

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

[commit-bot: I haz the power, 2015-07-07 15:50:07]

Committed patchset #30 (id:800001) as
https://skia.googlesource.com/skia/+/1c8a587120fd33854974457cf2a795ead98183a9