Issue 5785054: fixed CMYK jpeg issue & got jpeg decoding working on linux (Closed)
|
Created:
12 years, 1 month ago by robertphillips Modified:
12 years, 1 month ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionThis 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.
Patch Set 1 #
Total comments: 18
Patch Set 2 : Next revision of CMYK jpeg fix #
Total comments: 8
Patch Set 3 : Minor changes to address comments #Patch Set 4 : Updated Decoder Factory to only force jpeg linking #Patch Set 5 : fix for gm command line arg processing bug #Patch Set 6 : Patch for SampleApp command line parsing issue #MessagesTotal messages: 19
lgtm
Sign in to reply to this message.
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
Sign in to reply to this message.
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...
Sign in to reply to this message.
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!
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
I have altered the SkImageDecoder_Factory.cpp so that it only forces linking of the jpeg codec.
Sign in to reply to this message.
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
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".
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
|
