Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1401883005:
Add dm target to BUILD and refactor BUILD file. Fix blaze compilation errors. (Closed)
DescriptionAdd dm target to BUILD and refactor BUILD file. The corresponding google3 BUILD file changes are in google3 cl/105413829.
Fix blaze compilation errors. The default compilation options for blaze require that the initialization list must list fields in initialization order. Also deal with differing versions of libjpeg.
The BUILD changes were started by melanielc in google3 cl/102860957.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/6f6bef84a3be8ecbd0fd6452a8f257d95fbecef4
Patch Set 1 : #
Total comments: 24
Patch Set 2 : Respond to code review comments; fix BUILD_simulator.py. #Patch Set 3 : Rebase and fix new issues in SkJpegCodec.cpp. #
Total comments: 9
Messages
Total messages: 33 (12 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401883005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401883005/20001
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401883005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401883005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
benjaminwagner@google.com changed reviewers: + mtklein@google.com
mtklein@google.com changed reviewers: + msarett@google.com
+Matt for changes to src/codec/SkJpegCodec.cpp https://codereview.chromium.org/1401883005/diff/40001/BUILD.public File BUILD.public (right): https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode22 BUILD.public:22: exclude = [ This list might be easier to follow with some sorting? https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode44 BUILD.public:44: # TODO(mtklein): Should these be fixed upstream? These are mostly working as intended I think. A lot of these are specific to particular platforms. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode45 BUILD.public:45: "src/device/xps/*", # Missing header ObjBase.h. (Windows-only.) https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode131 BUILD.public:131: "include/animator", I'm a little skeptical about some of these additions, but I'm willing to start wide and pull back. I don't think we use anything in include or src related to animator, nacl, svg, views, or xml. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode202 BUILD.public:202: "gm/gmmain.cpp", # Old main Doesn't exist anymore. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode206 BUILD.public:206: "tests/FontConfigParser.cpp", # Android-only. Ditto. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode208 BUILD.public:208: "gm/techtalk1.cpp", # Not in our .gyp. Ditto. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode209 BUILD.public:209: "dm/DMSrcSinkAndroid.cpp", # Does not compile Might want to say "# Android-only.", but this is fine. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode281 BUILD.public:281: deps = DEPS, It might be clearer if we move the parts of DEPS that can be public here: deps = DEPS + [ ":opts_ssse3", ":opts_sse4", ] same with DM_DEPS.
lgtm https://codereview.chromium.org/1401883005/diff/40001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1401883005/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:157: int colorBytes = (dinfo->out_color_space == JCS_RGB565) ? 2 : dinfo->out_color_components; RGB565 is relatively new to turbo. I had to add this to chromium's copy from upstream not too long ago. https://codereview.chromium.org/1401883005/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:258: fDecoderMgr->dinfo()->out_color_space = JCS_EXT_RGBA; RGBA and BGRA aren't a part of libjpeg. We will have to change our implementation to work with regular libjpeg. Or is this really old turbo? https://codereview.chromium.org/1401883005/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:523: jpeg_skip_scanlines(fDecoderMgr->dinfo(), count); This works for now. I need to clean this up.
PTAL https://codereview.chromium.org/1401883005/diff/40001/BUILD.public File BUILD.public (right): https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode22 BUILD.public:22: exclude = [ On 2015/10/14 18:45:17, mtklein wrote: > This list might be easier to follow with some sorting? Done. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode44 BUILD.public:44: # TODO(mtklein): Should these be fixed upstream? On 2015/10/14 18:45:17, mtklein wrote: > These are mostly working as intended I think. A lot of these are specific to > particular platforms. Done. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode45 BUILD.public:45: "src/device/xps/*", # Missing header ObjBase.h. On 2015/10/14 18:45:16, mtklein wrote: > (Windows-only.) Done. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode131 BUILD.public:131: "include/animator", On 2015/10/14 18:45:17, mtklein wrote: > I'm a little skeptical about some of these additions, but I'm willing to start > wide and pull back. I don't think we use anything in include or src related to > animator, nacl, svg, views, or xml. svg is used in dm/DMSrcSink.cpp, and xml is used by svg. src/effects/SkMorphologyImageFilter.cpp uses src/effects. I moved src/codec to DM_INCLUDES. I excluded the others. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode202 BUILD.public:202: "gm/gmmain.cpp", # Old main On 2015/10/14 18:45:16, mtklein wrote: > Doesn't exist anymore. Done. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode206 BUILD.public:206: "tests/FontConfigParser.cpp", # Android-only. On 2015/10/14 18:45:16, mtklein wrote: > Ditto. Done. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode208 BUILD.public:208: "gm/techtalk1.cpp", # Not in our .gyp. On 2015/10/14 18:45:17, mtklein wrote: > Ditto. Done. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode209 BUILD.public:209: "dm/DMSrcSinkAndroid.cpp", # Does not compile On 2015/10/14 18:45:16, mtklein wrote: > Might want to say "# Android-only.", but this is fine. Done. https://codereview.chromium.org/1401883005/diff/40001/BUILD.public#newcode281 BUILD.public:281: deps = DEPS, On 2015/10/14 18:45:17, mtklein wrote: > It might be clearer if we move the parts of DEPS that can be public here: > > deps = DEPS + [ > ":opts_ssse3", > ":opts_sse4", > ] > > same with DM_DEPS. Done. https://codereview.chromium.org/1401883005/diff/40001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1401883005/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:157: int colorBytes = (dinfo->out_color_space == JCS_RGB565) ? 2 : dinfo->out_color_components; On 2015/10/14 19:58:25, msarett wrote: > RGB565 is relatively new to turbo. > > I had to add this to chromium's copy from upstream not too long ago. Acknowledged. https://codereview.chromium.org/1401883005/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:258: fDecoderMgr->dinfo()->out_color_space = JCS_EXT_RGBA; On 2015/10/14 19:58:25, msarett wrote: > RGBA and BGRA aren't a part of libjpeg. > > We will have to change our implementation to work with regular libjpeg. Or is > this really old turbo? It's libjpeg. I'll follow up with you about this. https://codereview.chromium.org/1401883005/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:523: jpeg_skip_scanlines(fDecoderMgr->dinfo(), count); On 2015/10/14 19:58:25, msarett wrote: > This works for now. I need to clean this up. Cool, thanks!
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401883005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401883005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Matt reenabled warnings under src/codec (accidentally disabled) and fixed them yesterday. Mind rebasing? Some of them were fixed in slightly different ways than you've done here, so I want to make sure we don't stomp on that.
On 2015/10/15 14:22:17, mtklein wrote: > Matt reenabled warnings under src/codec (accidentally disabled) and fixed them > yesterday. Mind rebasing? Some of them were fixed in slightly different ways > than you've done here, so I want to make sure we don't stomp on that. Done. Reverted cpps except SkJpegCodec.cpp, and fixed a compile error in SkJpegCodec.cpp.
The CQ bit was checked by mtklein@google.com
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com Link to the patchset: https://codereview.chromium.org/1401883005/#ps80001 (title: "Rebase and fix new issues in SkJpegCodec.cpp.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401883005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401883005/80001
msarett@google.com changed reviewers: + scroggo@google.com
Adding Leon for comments on Google3 specific code. https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:507: static uint32_t jpeg_skip_scanlines(jpeg_decompress_struct* dinfo, int count) { +1
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://skia.googlesource.com/skia/+/6f6bef84a3be8ecbd0fd6452a8f257d95fbecef4
Message was sent while issue was closed.
https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:154: #if defined (GOOGLE3) Is this because GOOGLE3 is expected to build against a particular version of libjpeg? Or perhaps just not libjpeg-turbo? Or it should build against a variety of libjpegs? This define just seems too vague (and yet too specific - maybe other builders outside of GOOGLE3 need these special cases). I think we should take this one of two ways: 1. Require we build with a particular version of libjpeg-turbo. This means we will need to update them in sync with us, which may be more work in some ways. 2. Use flags which correspond to specific features. We already do this with TURBO_HAS_SKIP. It seems like libpng has every individual feature behind some kind of flag; I do not remember whether libjpeg does as well, but I recall SkImageDecoder_libjpeg having something like ANDROID_RGB_565 for some features that were not in libjpeg. Even if the library does not, we could have our own flags, which would be a lot clearer than checking for GOOGLE3. This is potentially more work in its own way - bugs can arise for flags that we're not actively testing (or we just test more combinations...). I know that Blink decided to remove many of its ifdefs and just assume particular versions of libraries are being used. (They're in a different situation, since Blink is not a library that is expected to be used outside of Chromium.) https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:255: return false; It appears that in GOOGLE3, SkJpegCodec will not work for most images. Why bother building it there at all? https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:413: SK_CRASH(); Why this difference? I assume that SkASSERT works in GOOGLE3, or our code would be littered with this conditional (or we'd just fix it in SkASSERT/SkPreConfig, so we wouldn't need this here).
Message was sent while issue was closed.
https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:154: #if defined (GOOGLE3) Why don't we just use libjpeg's own defines as the guards, e.g. #if defined(JCS_RGB565) ... #endif #if defined(JCS_EXT_RGBA) && defined(JSC_EXT_BGRA) ... #endif.
Message was sent while issue was closed.
https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:154: #if defined (GOOGLE3) On 2015/10/27 14:27:59, mtklein wrote: > Why don't we just use libjpeg's own defines as the guards, e.g. > #if defined(JCS_RGB565) > ... > #endif > > #if defined(JCS_EXT_RGBA) && defined(JSC_EXT_BGRA) > ... > #endif. sgtm They are actually enums. Will that still work?
Message was sent while issue was closed.
On 2015/10/27 14:31:36, msarett wrote: > https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.cpp > File src/codec/SkJpegCodec.cpp (right): > > https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... > src/codec/SkJpegCodec.cpp:154: #if defined (GOOGLE3) > On 2015/10/27 14:27:59, mtklein wrote: > > Why don't we just use libjpeg's own defines as the guards, e.g. > > #if defined(JCS_RGB565) > > ... > > #endif > > > > #if defined(JCS_EXT_RGBA) && defined(JSC_EXT_BGRA) > > ... > > #endif. > > sgtm > > They are actually enums. Will that still work? No, I don't think so.
Message was sent while issue was closed.
https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:154: #if defined (GOOGLE3) On 2015/10/27 at 13:51:23, scroggo wrote: > Is this because GOOGLE3 is expected to build against a particular version of libjpeg? Or perhaps just not libjpeg-turbo? Or it should build against a variety of libjpegs? > > This define just seems too vague (and yet too specific - maybe other builders outside of GOOGLE3 need these special cases). I think we should take this one of two ways: > 1. Require we build with a particular version of libjpeg-turbo. > This means we will need to update them in sync with us, which may be more work in some ways. > 2. Use flags which correspond to specific features. > We already do this with TURBO_HAS_SKIP. It seems like libpng has every individual feature behind some kind of flag; I do not remember whether libjpeg does as well, but I recall SkImageDecoder_libjpeg having something like ANDROID_RGB_565 for some features that were not in libjpeg. Even if the library does not, we could have our own flags, which would be a lot clearer than checking for GOOGLE3. This is potentially more work in its own way - bugs can arise for flags that we're not actively testing (or we just test more combinations...). I know that Blink decided to remove many of its ifdefs and just assume particular versions of libraries are being used. (They're in a different situation, since Blink is not a library that is expected to be used outside of Chromium.) Sorry, I should have left a comment or TODO. I forwarded you a thread about updating the version of libjpeg_turbo in Google3. https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:255: return false; On 2015/10/27 at 13:51:23, scroggo wrote: > It appears that in GOOGLE3, SkJpegCodec will not work for most images. Why bother building it there at all? I didn't realize that not supporting JCS_EXT_* would mean it wouldn't work for most images. I can send a CL to conditionally remove Jpeg from SkCodec.cpp. https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:413: SK_CRASH(); On 2015/10/27 at 13:51:23, scroggo wrote: > Why this difference? I assume that SkASSERT works in GOOGLE3, or our code would be littered with this conditional (or we'd just fix it in SkASSERT/SkPreConfig, so we wouldn't need this here). Sorry, I added this before https://codereview.chromium.org/1400343005 and didn't realize it wasn't needed anymore.
Message was sent while issue was closed.
On 2015/10/27 15:07:22, Ben Wagner wrote: > https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.cpp > File src/codec/SkJpegCodec.cpp (right): > > https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... > src/codec/SkJpegCodec.cpp:154: #if defined (GOOGLE3) > On 2015/10/27 at 13:51:23, scroggo wrote: > > Is this because GOOGLE3 is expected to build against a particular version of > libjpeg? Or perhaps just not libjpeg-turbo? Or it should build against a variety > of libjpegs? > > > > This define just seems too vague (and yet too specific - maybe other builders > outside of GOOGLE3 need these special cases). I think we should take this one of > two ways: > > 1. Require we build with a particular version of libjpeg-turbo. > > This means we will need to update them in sync with us, which may be more work > in some ways. > > 2. Use flags which correspond to specific features. > > We already do this with TURBO_HAS_SKIP. It seems like libpng has every > individual feature behind some kind of flag; I do not remember whether libjpeg > does as well, but I recall SkImageDecoder_libjpeg having something like > ANDROID_RGB_565 for some features that were not in libjpeg. Even if the library > does not, we could have our own flags, which would be a lot clearer than > checking for GOOGLE3. This is potentially more work in its own way - bugs can > arise for flags that we're not actively testing (or we just test more > combinations...). I know that Blink decided to remove many of its ifdefs and > just assume particular versions of libraries are being used. (They're in a > different situation, since Blink is not a library that is expected to be used > outside of Chromium.) > > Sorry, I should have left a comment or TODO. I forwarded you a thread about > updating the version of libjpeg_turbo in Google3. > > https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... > src/codec/SkJpegCodec.cpp:255: return false; > On 2015/10/27 at 13:51:23, scroggo wrote: > > It appears that in GOOGLE3, SkJpegCodec will not work for most images. Why > bother building it there at all? > > I didn't realize that not supporting JCS_EXT_* would mean it wouldn't work for > most images. I can send a CL to conditionally remove Jpeg from SkCodec.cpp. > > https://codereview.chromium.org/1401883005/diff/80001/src/codec/SkJpegCodec.c... > src/codec/SkJpegCodec.cpp:413: SK_CRASH(); > On 2015/10/27 at 13:51:23, scroggo wrote: > > Why this difference? I assume that SkASSERT works in GOOGLE3, or our code > would be littered with this conditional (or we'd just fix it in > SkASSERT/SkPreConfig, so we wouldn't need this here). > > Sorry, I added this before https://codereview.chromium.org/1400343005 and didn't > realize it wasn't needed anymore. I agree there's no pressing need to support SkJpegCodec, or SkCodec in general, in Google3. I can see it simplifies things to ignore clients where we cannot dictate the libjpeg flavor and version, perhaps at least until SkCodec's launched in Blink + Android. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
