Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1687863003:
Make SkJpegCodec compatible with libjpeg (Closed)
DescriptionMake SkJpegCodec compatible with libjpeg
BUG=skia:4470
BUG=skia:4520
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1687863003
Committed: https://skia.googlesource.com/skia/+/70e418b468b1656feae3e84851562b22e5d25660
Patch Set 1 : Test setup #Patch Set 2 : #Patch Set 3 : Reenable Jpeg for Google3 #
Total comments: 7
Patch Set 4 : Response to comments #
Total comments: 7
Patch Set 5 : Remove unnecessary ifdefs #Patch Set 6 : Rebase #
Total comments: 2
Patch Set 7 : Rebase properly #Patch Set 8 : Fix gyp #
Messages
Total messages: 32 (13 generated)
Description was changed from ========== Make SkJpegCodec compatible with libjpeg BUG=skia:4520 ========== to ========== Make SkJpegCodec compatible with libjpeg BUG=skia:4520 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + dogben@google.com, mtklein@google.com, scroggo@google.com
This should work for the CMake build and Google3. Will this be tested on CMake? I've tested locally, but it'd be cool to have one of the bots running this. It's a little more complicated than I'd hoped, but I think not too bad. https://codereview.chromium.org/1687863003/diff/40001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1687863003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:194: #if defined(SK_PMCOLOR_IS_RGBA) && defined(LIBJPEG_TURBO_VERSION) The LIBJPEG_TURBO_VERSION macro is *almost* useless. It was added in version 1.2.0 (where it is defined as 1.2.0). BGRA/RGBA were also added in 1.2.0, so we can the existence of this macro to test for BGRA/RGBA. However, we can't do anything interesting with it (for example test if version > 1.4) because 1.x.x is not a number and can't be compared to numbers. https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... File tools/BUILD.public.expected (right): https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... tools/BUILD.public.expected:325: 'src/codec/SkJpegCodec.cpp', I couldn't get the autogeneration to work so I just added this manually. Maybe that's not a good idea? This file seems out of date anyway? SkCodec_wbmp etc. have been renamed.
benjaminwagner@google.com changed reviewers: + benjaminwagner@google.com
https://codereview.chromium.org/1687863003/diff/40001/public.bzl File public.bzl (right): https://codereview.chromium.org/1687863003/diff/40001/public.bzl#newcode482 public.bzl:482: "SK_BUILD_FOR_UNIX", I patched this CL in and found out that you need to add PNG_SKIP_SETJMP_CHECK here. diff --git a/public.bzl b/public.bzl index 33e5f15..98f6ef6 100644 --- a/public.bzl +++ b/public.bzl @@ -479,6 +479,7 @@ COPTS_ALL = [] ################################################################################ DEFINES_UNIX = [ + "PNG_SKIP_SETJMP_CHECK", "SK_BUILD_FOR_UNIX", "SK_SAMPLES_FOR_X", "SK_SFNTLY_SUBSETTER", https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... File tools/BUILD.public.expected (right): https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... tools/BUILD.public.expected:325: 'src/codec/SkJpegCodec.cpp', On 2016/02/11 at 00:08:48, msarett wrote: > I couldn't get the autogeneration to work so I just added this manually. Maybe that's not a good idea? > > This file seems out of date anyway? SkCodec_wbmp etc. have been renamed. Yeah, don't worry about it. If you want the full story, let me know.
Let's follow up with CMake. It's gonna take some work for each of the src/codec backends to build I think. Here's the sort of thing we'll need to do: https://codereview.chromium.org/1689053002/patch/1/10001 https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... File tools/BUILD.public.expected (right): https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... tools/BUILD.public.expected:325: 'src/codec/SkJpegCodec.cpp', On 2016/02/11 01:59:58, Ben Wagner wrote: > On 2016/02/11 at 00:08:48, msarett wrote: > > I couldn't get the autogeneration to work so I just added this manually. > Maybe that's not a good idea? > > > > This file seems out of date anyway? SkCodec_wbmp etc. have been renamed. > > Yeah, don't worry about it. If you want the full story, let me know. I think we can just kill this file off for now, eh?
On 2016/02/11 02:06:59, mtklein wrote: > Let's follow up with CMake. It's gonna take some work for each of the src/codec > backends to build I think. Here's the sort of thing we'll need to do: > > https://codereview.chromium.org/1689053002/patch/1/10001 > > https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... > File tools/BUILD.public.expected (right): > > https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... > tools/BUILD.public.expected:325: 'src/codec/SkJpegCodec.cpp', > On 2016/02/11 01:59:58, Ben Wagner wrote: > > On 2016/02/11 at 00:08:48, msarett wrote: > > > I couldn't get the autogeneration to work so I just added this manually. > > Maybe that's not a good idea? > > > > > > This file seems out of date anyway? SkCodec_wbmp etc. have been renamed. > > > > Yeah, don't worry about it. If you want the full story, let me know. > > I think we can just kill this file off for now, eh? I think I've got something building here: https://codereview.chromium.org/1689053002 I can't build SkGifCodec, and I don't have pngstruct.h publically, so I had to short-circuit the SSE filter procs.
Great! Let's move things to: https://codereview.chromium.org/1689053002
Or did you want to land this first and then follow-up with the CMake changes? https://codereview.chromium.org/1687863003/diff/40001/public.bzl File public.bzl (right): https://codereview.chromium.org/1687863003/diff/40001/public.bzl#newcode482 public.bzl:482: "SK_BUILD_FOR_UNIX", On 2016/02/11 01:59:58, Ben Wagner wrote: > I patched this CL in and found out that you need to add PNG_SKIP_SETJMP_CHECK > here. > > diff --git a/public.bzl b/public.bzl > index 33e5f15..98f6ef6 100644 > --- a/public.bzl > +++ b/public.bzl > @@ -479,6 +479,7 @@ COPTS_ALL = [] > > ################################################################################ > > DEFINES_UNIX = [ > + "PNG_SKIP_SETJMP_CHECK", > "SK_BUILD_FOR_UNIX", > "SK_SAMPLES_FOR_X", > "SK_SFNTLY_SUBSETTER", Done. https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... File tools/BUILD.public.expected (right): https://codereview.chromium.org/1687863003/diff/40001/tools/BUILD.public.expe... tools/BUILD.public.expected:325: 'src/codec/SkJpegCodec.cpp', On 2016/02/11 02:06:59, mtklein wrote: > On 2016/02/11 01:59:58, Ben Wagner wrote: > > On 2016/02/11 at 00:08:48, msarett wrote: > > > I couldn't get the autogeneration to work so I just added this manually. > > Maybe that's not a good idea? > > > > > > This file seems out of date anyway? SkCodec_wbmp etc. have been renamed. > > > > Yeah, don't worry about it. If you want the full story, let me know. > > I think we can just kill this file off for now, eh? Done.
Description was changed from ========== Make SkJpegCodec compatible with libjpeg BUG=skia:4520 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make SkJpegCodec compatible with libjpeg BUG=skia:4470 BUG=skia:4520 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
PTAL I'd like to land this first to fix Google3. Then I'll look to incrementally add CMake support.
SkCodec code lgtm https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:194: #if defined(SK_PMCOLOR_IS_RGBA) && defined(LIBJPEG_TURBO_VERSION) Why not: #ifdef LIBJPEG_TURBO_VERSION #ifdef SK_PMCOLOR_IS_RGBA fDecoderMgr->dinfo()->out_color_space = JCS_EXT_RGBA; #else fDecoderMgr->dinfo()->out_color_space = JCS_EXT_BGRA; #endif #else fDecoderMgr->dinfo()->out_color_space = JCS_RGB; #endif https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:372: #ifndef TURBO_HAS_565 For simplicity, I think we can drop the #ifndef and just always check for kRGB? https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:420: #ifdef TURBO_HAS_565 Same here
Deferring to Ben for Google3 approval https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:194: #if defined(SK_PMCOLOR_IS_RGBA) && defined(LIBJPEG_TURBO_VERSION) On 2016/02/12 17:25:29, scroggo wrote: > Why not: > > #ifdef LIBJPEG_TURBO_VERSION > #ifdef SK_PMCOLOR_IS_RGBA > fDecoderMgr->dinfo()->out_color_space = JCS_EXT_RGBA; > #else > fDecoderMgr->dinfo()->out_color_space = JCS_EXT_BGRA; > #endif > #else > fDecoderMgr->dinfo()->out_color_space = JCS_RGB; > #endif Done. https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:305: #ifdef TURBO_HAS_565 Also removing this ifdef https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:372: #ifndef TURBO_HAS_565 On 2016/02/12 17:25:29, scroggo wrote: > For simplicity, I think we can drop the #ifndef and just always check for kRGB? Yeah you're right. I know we like to get rid of #ifdefs unless they are absolutely necessary, so I'll remove this. It bothers me though to not have this "extra" code marked somehow. But it does make things more readable. https://codereview.chromium.org/1687863003/diff/60001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:420: #ifdef TURBO_HAS_565 On 2016/02/12 17:25:29, scroggo wrote: > Same here Done.
lgtm I assume it's still good for google3.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1687863003/#ps80001 (title: "Remove unnecessary ifdefs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687863003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687863003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from benjaminwagner@google.com, scroggo@google.com Link to the patchset: https://codereview.chromium.org/1687863003/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687863003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687863003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
On 2016/02/12 at 18:14:14, Ben Wagner wrote: > lgtm > > I assume it's still good for google3. I caused a merge conflict by enabling the RAW codec in https://codereview.chromium.org/1692513003. Sorry about that.
https://codereview.chromium.org/1687863003/diff/100001/public.bzl File public.bzl (right): https://codereview.chromium.org/1687863003/diff/100001/public.bzl#newcode132 public.bzl:132: # TODO(benjaminwagner): Can this be enabled? Please don't re-add. See https://codereview.chromium.org/1692513003
https://codereview.chromium.org/1687863003/diff/100001/public.bzl File public.bzl (right): https://codereview.chromium.org/1687863003/diff/100001/public.bzl#newcode132 public.bzl:132: # TODO(benjaminwagner): Can this be enabled? On 2016/02/12 20:00:13, Ben Wagner wrote: > Please don't re-add. See https://codereview.chromium.org/1692513003 Uggh. Sorry my mistake.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from benjaminwagner@google.com, scroggo@google.com Link to the patchset: https://codereview.chromium.org/1687863003/#ps140001 (title: "Fix gyp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687863003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687863003/140001
Message was sent while issue was closed.
Description was changed from ========== Make SkJpegCodec compatible with libjpeg BUG=skia:4470 BUG=skia:4520 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make SkJpegCodec compatible with libjpeg BUG=skia:4470 BUG=skia:4520 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/70e418b468b1656feae3e84851562b22e5d25660 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/70e418b468b1656feae3e84851562b22e5d25660 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
