Call standard libjpeg/libjpeg-turbo APIs from SkJpegCodec

Call standard libjpeg/libjpeg-turbo APIs

BUG=skia:

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

[msarett, 2015-08-28 18:56:04]

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

[msarett, 2015-08-28 18:56:05]

I originally thought it was "safer" to call the chromium_ APIs, so we could be
sure that we were in fact calling into the copy of libjpeg-turbo that I
intended.

Mike is working on an effort to compile SkCodec with standard, system copies of
libjpeg etc.  Removing SkJpegCodec's dependency on chromium's libjpeg-turbo API
should help with that, and IMO is a good thing to do in general.  Just to be
clear, when we compile with Skia's build system, we are still using the chromium
copy of turbo.

[msarett, 2015-08-28 19:37:14]

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

https://codereview.chromium.org/1322623004/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:190: dinfo.global_state = DSTATE_READY;
Applications are not "supposed" to include jpegint.h

DSTATE_READY is the reason that we need to include jpegint.h

I forgot about this hack...

We want to use jpeg_calc_output_dimensions() to see what output dimensions are
supported.  But we don't want to use fDinfo because this function is const.  So
instead we create a fake decompress struct and use it to calculate output
dimensions.

Three workarounds I can think of:
(1) Allow onGetScaledDimensions() to not be const.
(2) Replace DSTATE_READY with 202 (yuck).
(3) Replace calls to jpeg_calc_output_dimensions() to calls to our own
implementation of ceil(originalDim * num / denom), which is all
jpeg_calc_output_dimensions() does anyway.

[msarett, 2015-08-28 20:16:15]

Mike had a clever idea that allows us to remove jpegint.h

[scroggo, 2015-08-28 20:17:30]

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

https://codereview.chromium.org/1322623004/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:190: dinfo.global_state = DSTATE_READY;
On 2015/08/28 19:37:14, msarett wrote:
> Applications are not "supposed" to include jpegint.h
> 
> DSTATE_READY is the reason that we need to include jpegint.h
> 
> I forgot about this hack...
> 
> We want to use jpeg_calc_output_dimensions() to see what output dimensions are
> supported.  But we don't want to use fDinfo because this function is const. 
So
> instead we create a fake decompress struct and use it to calculate output
> dimensions.
> 
> Three workarounds I can think of:
> (1) Allow onGetScaledDimensions() to not be const.

That seems odd. I forgot, what does jpeg_calc_output_dimensions do that stops
this function from being const? Could we make the real dinfo mutable? Is it safe
to call jpeg_calc_output_dimensions multiple times?

> (2) Replace DSTATE_READY with 202 (yuck).
> (3) Replace calls to jpeg_calc_output_dimensions() to calls to our own
> implementation of ceil(originalDim * num / denom), which is all
> jpeg_calc_output_dimensions() does anyway.

The downside I see to including jpegint.h is that we are relying on their
internal implementation. It seems like either of these options still relies on
their internal implementation, so I do not see what they buy us.

[mtklein, 2015-08-28 20:26:41]

ps4 lgtm

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

https://codereview.chromium.org/1322623004/diff/40001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:545: #define jpeg_skip_scanlines(dinfo, count)        
                  \
Might want to reformat these \'s/.

[scroggo, 2015-08-28 20:31:00]

lgtm

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

https://codereview.chromium.org/1322623004/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:189: dinfo.global_state =
fDecoderMgr->dinfo()->global_state;
Haha, agreed, this is much better!

[msarett, 2015-08-28 20:37:59]

The CQ bit was checked by msarett@google.com

[msarett, 2015-08-28 20:37:59]

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

[commit-bot: I haz the power, 2015-08-28 20:38:08]

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

[commit-bot: I haz the power, 2015-08-28 20:40:18]

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

[commit-bot: I haz the power, 2015-08-28 20:40:19]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[msarett, 2015-08-28 20:42:58]

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

https://codereview.chromium.org/1322623004/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:190: dinfo.global_state = DSTATE_READY;
On 2015/08/28 20:17:30, scroggo wrote:
> On 2015/08/28 19:37:14, msarett wrote:
> > Applications are not "supposed" to include jpegint.h
> > 
> > DSTATE_READY is the reason that we need to include jpegint.h
> > 
> > I forgot about this hack...
> > 
> > We want to use jpeg_calc_output_dimensions() to see what output dimensions
are
> > supported.  But we don't want to use fDinfo because this function is const. 
> So
> > instead we create a fake decompress struct and use it to calculate output
> > dimensions.
> > 
> > Three workarounds I can think of:
> > (1) Allow onGetScaledDimensions() to not be const.
> 
> That seems odd. I forgot, what does jpeg_calc_output_dimensions do that stops
> this function from being const? Could we make the real dinfo mutable? Is it
safe
> to call jpeg_calc_output_dimensions multiple times?
> 

It is safe to call jpeg_calc_output_dimensions() multiple times.  It uses
dinfo->num and dinfo->denom to calculate dinfo->out_width and dinfo->out_height.

When we finally call jpeg_start_decompress(), it will actually call
jpeg_calc_output_dimensions() to set out_width and out_height for a final time,
before beginning to decode.  After we call jpeg_start_decompress(), we shouldn't
call jpeg_calc_output_dimensions() anymore.

> > (2) Replace DSTATE_READY with 202 (yuck).
> > (3) Replace calls to jpeg_calc_output_dimensions() to calls to our own
> > implementation of ceil(originalDim * num / denom), which is all
> > jpeg_calc_output_dimensions() does anyway.
> 
> The downside I see to including jpegint.h is that we are relying on their
> internal implementation. It seems like either of these options still relies on
> their internal implementation, so I do not see what they buy us.

Agreed.  I think we came up with a good solution.

[msarett, 2015-08-28 21:11:55]

Crashes on windows were caused by the TURBO_HAS_SKIP flag not being defined
properly.  This is fixed by using "defines" instead of "cflags".

Other crashes were caused by multiple calls to getScaledDimensions() then
getPixels().  The second time we call getScaledDimensions (before we have
rewound), the state of fDInfo will be invalid and libjpeg-turbo will error exit.
 We fix this by saving the state after the first time we read the header.

[msarett, 2015-08-28 21:12:03]

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

[commit-bot: I haz the power, 2015-08-28 21:12:17]

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

[mtklein, 2015-08-28 21:16:40]

lgtm

[commit-bot: I haz the power, 2015-08-28 21:17:28]

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

[commit-bot: I haz the power, 2015-08-28 21:17:29]

Dry run: This issue passed the CQ dry run.

[msarett, 2015-09-01 12:00:55]

Leon can you give this another look?

The version you approved didn't pass the commit queue.  We need to store the
"ready state" in a class variable so that rewinding works properly.

[scroggo, 2015-09-01 13:12:44]

On 2015/09/01 12:00:55, msarett wrote:
> Leon can you give this another look?
> 
> The version you approved didn't pass the commit queue.  We need to store the
> "ready state" in a class variable so that rewinding works properly.

LGTM. Glad the CQ caught the bug!

[msarett, 2015-09-01 13:37:40]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-09-01 13:37:47]

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

[commit-bot: I haz the power, 2015-09-01 13:43:45]

Committed patchset #7 (id:110001) as
https://skia.googlesource.com/skia/+/fbccb5995dcda645a3a8af51805639cb0c0d2300