fixed CMYK jpeg issue & got jpeg decoding working on linux

This delivery does 4 related things:

  1) it allows Skia to decode CMYK encoded jpegs addressing issue 69 (SkImageDecoder_libjpeg.cpp)

  2) it gets the jpeg code working again by partially rolling back delivery 841 (which relied on some Android-specific changes to libjpeg) (SkJpegUtility.cpp)

  3) it adds a new test case to exercise the jpeg decoding (cmykjpeg.cpp & gmslides.gypi)

  4) it enables jpeg file handling on Linux (images.gyp)

What is currently lacking is a public domain CMYK jpeg image - the image attached to Issue 69 is of uncertain provenance.

[reed1, 2012-03-08 19:09:32]

lgtm

[DerekS, 2012-03-08 19:43:54]

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp
File gm/cmykjpeg.cpp (right):

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp#newcode26
gm/cmykjpeg.cpp:26: SkFILEStream
stream("/home/robertphillips/debug/FlameBoy_Cut.eps_6BE7C7E9.jpg");
You should check-in this image or better yet include the encoded content in a
variable at the end of this file.

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp#newcode47
gm/cmykjpeg.cpp:47: canvas->save();
I don't think we need the save and restore here. I believe the caller should
make sure the canvas is restored.

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp
File gyp/images.gyp (right):

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp#newcode126
gyp/images.gyp:126: '-ljpeg',
this requires the user have libjpeg on their machine. Check with epoger about
changing this to be dependent on the code in /third_party/externals.

Further, if we are using the stuff in externals this should work on all
platforms.

http://codereview.appspot.com/5785054/diff/1/src/images/SkJpegUtility.cpp
File src/images/SkJpegUtility.cpp (right):

http://codereview.appspot.com/5785054/diff/1/src/images/SkJpegUtility.cpp#new...
src/images/SkJpegUtility.cpp:81: }
remove space

[epoger, 2012-03-08 21:32:59]

At a high level: in the future, I would recommend splitting changes such as
these into multiple CLs if possible.  (It would be nice if the addition of CMYK
decoding was separate from getting jpeg working again generally.)  Just my 2
cents.  (I certainly don't think you should bother breaking this one up now.)

I have to run right now, but I will comment on more specifics by tomorrow
morning...

[epoger, 2012-03-09 17:20:01]

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp
File gm/cmykjpeg.cpp (right):

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp#newcode47
gm/cmykjpeg.cpp:47: canvas->save();
On 2012/03/08 19:43:54, djsollen wrote:
> I don't think we need the save and restore here. I believe the caller should
> make sure the canvas is restored.

I would just follow the pattern of other gm's.  Looking at randomly-selected
http://code.google.com/p/skia/source/browse/trunk/gm/convexpaths.cpp and
http://code.google.com/p/skia/source/browse/trunk/gm/giantbitmap.cpp , it would
appear that Derek is right.  (these other gm's don't bother with the save and
restore)

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp
File gyp/images.gyp (right):

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp#newcode67
gyp/images.gyp:67: '../src/images/SkImageDecoder_libjpeg.cpp',
Like Derek wrote below, I think you can leave the jpeg support in for all
platforms since we are importing at as source.

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp#newcode91
gyp/images.gyp:91: '../src/images/SkImageDecoder_libjpeg.h',
Like Derek wrote below, I think you can leave the jpeg support in for all
platforms since we are importing at as source.

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp#newcode126
gyp/images.gyp:126: '-ljpeg',
On 2012/03/08 19:43:54, djsollen wrote:
> this requires the user have libjpeg on their machine. Check with epoger about
> changing this to be dependent on the code in /third_party/externals.

Right.  I think you should remove this stuff and instead add
third_party/externals/libjpeg/libjpeg.gyp:libjpeg to the 'dependencies' list at
the top of this file.

http://codereview.appspot.com/5785054/diff/1/src/images/SkImageDecoder_libjpe...
File src/images/SkImageDecoder_libjpeg.cpp (right):

http://codereview.appspot.com/5785054/diff/1/src/images/SkImageDecoder_libjpe...
src/images/SkImageDecoder_libjpeg.cpp:152: // At this point we've received CMYK
pixels from libjpeg. We
thanks for the explanation!

[robertphillips, 2012-03-19 18:41:48]

In this patch I have:

 altered gm & SampleApp to take a resource path argument (-i)
 added a CMYK.jpg file in gm/resources
 hopefully alter images.gyp to link the jpeg libraries on all platforms
 added a method in SkImageDecoder_Factory.cpp to force linking of the decoders

[robertphillips, 2012-03-19 18:42:38]

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp
File gm/cmykjpeg.cpp (right):

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp#newcode26
gm/cmykjpeg.cpp:26: SkFILEStream
stream("/home/robertphillips/debug/FlameBoy_Cut.eps_6BE7C7E9.jpg");
On 2012/03/08 19:43:54, djsollen wrote:
> You should check-in this image or better yet include the encoded content in a
> variable at the end of this file.

Done.

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp#newcode47
gm/cmykjpeg.cpp:47: canvas->save();
On 2012/03/08 19:43:54, djsollen wrote:
> I don't think we need the save and restore here. I believe the caller should
> make sure the canvas is restored.

Done.

http://codereview.appspot.com/5785054/diff/1/gm/cmykjpeg.cpp#newcode47
gm/cmykjpeg.cpp:47: canvas->save();
On 2012/03/09 17:20:01, epoger wrote:
> On 2012/03/08 19:43:54, djsollen wrote:
> > I don't think we need the save and restore here. I believe the caller should
> > make sure the canvas is restored.
> 
> I would just follow the pattern of other gm's.  Looking at randomly-selected
> http://code.google.com/p/skia/source/browse/trunk/gm/convexpaths.cpp and
> http://code.google.com/p/skia/source/browse/trunk/gm/giantbitmap.cpp , it
would
> appear that Derek is right.  (these other gm's don't bother with the save and
> restore)

Done.

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp
File gyp/images.gyp (right):

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp#newcode67
gyp/images.gyp:67: '../src/images/SkImageDecoder_libjpeg.cpp',
On 2012/03/09 17:20:01, epoger wrote:
> Like Derek wrote below, I think you can leave the jpeg support in for all
> platforms since we are importing at as source.

Done.

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp#newcode91
gyp/images.gyp:91: '../src/images/SkImageDecoder_libjpeg.h',
On 2012/03/09 17:20:01, epoger wrote:
> Like Derek wrote below, I think you can leave the jpeg support in for all
> platforms since we are importing at as source.

Done.

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp#newcode126
gyp/images.gyp:126: '-ljpeg',
On 2012/03/08 19:43:54, djsollen wrote:
> this requires the user have libjpeg on their machine. Check with epoger about
> changing this to be dependent on the code in /third_party/externals.
> 
> Further, if we are using the stuff in externals this should work on all
> platforms.

Done.

http://codereview.appspot.com/5785054/diff/1/gyp/images.gyp#newcode126
gyp/images.gyp:126: '-ljpeg',
On 2012/03/09 17:20:01, epoger wrote:
> On 2012/03/08 19:43:54, djsollen wrote:
> > this requires the user have libjpeg on their machine. Check with epoger
about
> > changing this to be dependent on the code in /third_party/externals.
> 
> Right.  I think you should remove this stuff and instead add
> third_party/externals/libjpeg/libjpeg.gyp:libjpeg to the 'dependencies' list
at
> the top of this file.

Done.

http://codereview.appspot.com/5785054/diff/1/src/images/SkImageDecoder_libjpe...
File src/images/SkImageDecoder_libjpeg.cpp (right):

http://codereview.appspot.com/5785054/diff/1/src/images/SkImageDecoder_libjpe...
src/images/SkImageDecoder_libjpeg.cpp:152: // At this point we've received CMYK
pixels from libjpeg. We
On 2012/03/09 17:20:01, epoger wrote:
> thanks for the explanation!

Done.

http://codereview.appspot.com/5785054/diff/1/src/images/SkJpegUtility.cpp
File src/images/SkJpegUtility.cpp (right):

http://codereview.appspot.com/5785054/diff/1/src/images/SkJpegUtility.cpp#new...
src/images/SkJpegUtility.cpp:81: }
On 2012/03/08 19:43:54, djsollen wrote:
> remove space

Done.

[reed1, 2012-03-19 18:48:14]

1. can the changes to SkJpegUtility be landed in isolation?
2. do we need to be all-or-nothing with which codecs we "require" in
ForceLinking? Is this really needed for more than JPEG?

http://codereview.appspot.com/5785054/diff/9001/gm/gm.cpp
File gm/gm.cpp (right):

http://codereview.appspot.com/5785054/diff/9001/gm/gm.cpp#newcode11
gm/gm.cpp:11: const char*  GM::fResourcePath = NULL;
No need to explicitly init to 0. Skia leaves this off (or should)

http://codereview.appspot.com/5785054/diff/9001/gm/gm.h
File gm/gm.h (left):

http://codereview.appspot.com/5785054/diff/9001/gm/gm.h#oldcode59
gm/gm.h:59: virtual SkISize onISize() = 0;
Static Methods Begin With A Capital

http://codereview.appspot.com/5785054/diff/9001/gm/gm.h
File gm/gm.h (right):

http://codereview.appspot.com/5785054/diff/9001/gm/gm.h#newcode60
gm/gm.h:60: protected:
static variables should either be prefixed by 'g' instead of 'f', or they can be
local to the .cpp

[robertphillips, 2012-03-19 19:01:47]

> 1. can the changes to SkJpegUtility be landed in isolation?

The SkJpegUtility changes are separate and could be delivered independently.

> 2. do we need to be all-or-nothing with which codecs we "require" in
> ForceLinking? Is this really needed for more than JPEG?

Actually I was a bit aggressive with that list. I need to temporarily remove the
png decoder. Once jpeg is working I hope to use the same trick of adding a
dependency to libpng for all platforms - leveraging Chrome's inclusion of the
source. Judging from images.gyp all the other formats should work on all
platforms (bmp, ico, png & wbmp) - although since they probably weren't being
linked in this could be a false assumption. I am testing this out on Windows
right now and hope to add a gm slide that tests all the decoders.

[DerekS, 2012-03-19 19:05:32]

http://codereview.appspot.com/5785054/diff/9001/gyp/images.gyp
File gyp/images.gyp (right):

http://codereview.appspot.com/5785054/diff/9001/gyp/images.gyp#newcode99
gyp/images.gyp:99: '-ljpeg', '-lpng',
do we need this now that we are compiling the source for libjpeg?

[epoger, 2012-03-19 19:26:39]

Other than Mike/Derek's existing comments, it L G T M.

http://codereview.appspot.com/5785054/diff/9001/gyp/images.gyp
File gyp/images.gyp (right):

http://codereview.appspot.com/5785054/diff/9001/gyp/images.gyp#newcode99
gyp/images.gyp:99: '-ljpeg', '-lpng',
On 2012/03/19 19:05:32, djsollen wrote:
> do we need this now that we are compiling the source for libjpeg?

Indeed, I would leave this section as it was before, just png.

[robertphillips, 2012-03-20 13:18:29]

This latest patch addresses the following issues:
  renames the resource path global to being with a 'g'
  renames the setResourcePath method to SetResourcePath
  reverts the images.gyp file to leave off the libjpeg
  adds a conditional block to libjpeg.gyp to deal with jpeg name mangling

[robertphillips, 2012-03-20 13:18:44]

http://codereview.appspot.com/5785054/diff/9001/gm/gm.cpp
File gm/gm.cpp (right):

http://codereview.appspot.com/5785054/diff/9001/gm/gm.cpp#newcode11
gm/gm.cpp:11: const char*  GM::fResourcePath = NULL;
On 2012/03/19 18:48:14, reed1 wrote:
> No need to explicitly init to 0. Skia leaves this off (or should)

Done.

http://codereview.appspot.com/5785054/diff/9001/gm/gm.h
File gm/gm.h (left):

http://codereview.appspot.com/5785054/diff/9001/gm/gm.h#oldcode59
gm/gm.h:59: virtual SkISize onISize() = 0;
On 2012/03/19 18:48:14, reed1 wrote:
> Static Methods Begin With A Capital

Done.

http://codereview.appspot.com/5785054/diff/9001/gm/gm.h
File gm/gm.h (right):

http://codereview.appspot.com/5785054/diff/9001/gm/gm.h#newcode60
gm/gm.h:60: protected:
On 2012/03/19 18:48:14, reed1 wrote:
> static variables should either be prefixed by 'g' instead of 'f', or they can
be
> local to the .cpp

Done.

[robertphillips, 2012-03-20 13:31:44]

I have altered the SkImageDecoder_Factory.cpp so that it only forces linking of
the jpeg codec.

[reed1, 2012-03-20 14:11:23]

lgtm

[robertphillips, 2012-03-22 13:04:00]

This patch should fix the SampleApp command line processing bug. In the prior
delivery I had not kept the initialization of the samples before their use in
findByTitle. I have also altered the command line syntax to require "--slide".

[reed1, 2012-03-22 13:23:30]

lgtm

[robertphillips, 2012-04-02 21:05:26]

code committed as r3438 (.gyp delivery still waiting)