Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1038863003:
WIP: Added support for giflib, updated jpeg and png (Closed)
DescriptionWIP: Added support for giflib, updated jpeg and png
BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/255dcd11992ebe74eb54202c48cf5394d33a8ce6
Committed: https://skia.googlesource.com/skia/+/6d0e7b2031f8eee4e88905082de0506cfbb2bfc3
Patch Set 1 #
Total comments: 18
Patch Set 2 : Added conditionals to jpeg and png sources, removed extra giflib #
Total comments: 2
Patch Set 3 : Fixed merge conflicts #
Total comments: 6
Patch Set 4 : Mips fix try again #Patch Set 5 : Arm fix trybot #Patch Set 6 : For review #
Total comments: 10
Patch Set 7 : Added dependencies! and flags #
Total comments: 5
Patch Set 8 : Style stuff and defines #Patch Set 9 : Fix for png dependencies on arm64 #Patch Set 10 : removing update to libpng #Patch Set 11 : Merge #
Messages
Total messages: 35 (14 generated)
msarett@google.com changed reviewers: + djsollen@google.com, scroggo@google.com
https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp File gyp/giflib.gyp (left): https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp#oldcode14 gyp/giflib.gyp:14: [ 'skia_giflib_static', As far as I can tell, this is always 0. So past code always used -lgif. https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp#newcode41 gyp/giflib.gyp:41: 'android_deps.gyp:gif', For android, do what images.gyp does and use android's library. https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... platform_tools/android/gyp/dependencies.gypi:130: #'../third_party/externals/jpeg/djpeg.c', Certain new files are causing android not to build: '../third_party/externals/jpeg/cjpeg.c', '../third_party/externals/jpeg/djpeg.c', '../third_party/externals/jpeg/jmem-ashmem.c', '../third_party/externals/jpeg/mips_jidctfst.c', '../third_party/externals/jpeg/mips_idct_le.S', Let me know if you have any idea why. Otherwise, I will look into this further.
https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp File gyp/giflib.gyp (left): https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp#oldcode14 gyp/giflib.gyp:14: [ 'skia_giflib_static', On 2015/03/26 18:22:46, msarett wrote: > As far as I can tell, this is always 0. So past code always used -lgif. Dead code. We should probably remove skia_giflib_static. https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... platform_tools/android/gyp/dependencies.gypi:130: #'../third_party/externals/jpeg/djpeg.c', On 2015/03/26 18:22:46, msarett wrote: > Certain new files are causing android not to build: > '../third_party/externals/jpeg/cjpeg.c', > '../third_party/externals/jpeg/djpeg.c', > '../third_party/externals/jpeg/jmem-ashmem.c', > '../third_party/externals/jpeg/mips_jidctfst.c', > '../third_party/externals/jpeg/mips_idct_le.S', > > Let me know if you have any idea why. Otherwise, I will look into this further. I *think* some of these are not part of the library, but separate programs (e.g. djpeg.c). mips will only compile on mips platforms, presumably, so these will need to be conditional.
https://codereview.chromium.org/1038863003/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/1038863003/diff/1/DEPS#newcode26 DEPS:26: "platform_tools/android/third_party/externals/gif" : "https://android.googlesource.com/platform/external/giflib.git@android-5.1.0_r3", should we drop this since we now have it in the main 3rd party code? https://codereview.chromium.org/1038863003/diff/1/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1038863003/diff/1/gyp/codec.gyp#newcode16 gyp/codec.gyp:16: 'giflib.gyp:giflib', no need to add it here until you have code in this target that uses it. https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp#newcode38 gyp/giflib.gyp:38: }, { # skia_os == "android" let's not make the distinction between android and other platforms for libgif as they all build using the same sources. https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... platform_tools/android/gyp/dependencies.gypi:84: 'target_name': 'png', I think this and jpeg are missing a conditional block where you add the platform specific sources and flags when appropriate. https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... platform_tools/android/gyp/dependencies.gypi:89: '../third_party/externals/png/arm/filter_neon_intrinsics.c', I don't think we can always depend on this. what happens if you try android_make -d xoom which is a target that doesn't support neon?
https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp#newcode38 gyp/giflib.gyp:38: }, { # skia_os == "android" On 2015/03/26 19:51:05, djsollen wrote: > let's not make the distinction between android and other platforms for libgif as > they all build using the same sources. We *do* need to make a distinction for skia_android_framework though. I think android_deps takes care of that for you, so you'll need to move that code here.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Added conditionals to jpeg and png sources, removed extra giflib https://codereview.chromium.org/1038863003/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/1038863003/diff/1/DEPS#newcode26 DEPS:26: "platform_tools/android/third_party/externals/gif" : "https://android.googlesource.com/platform/external/giflib.git@android-5.1.0_r3", On 2015/03/26 19:51:05, djsollen wrote: > should we drop this since we now have it in the main 3rd party code? Done. https://codereview.chromium.org/1038863003/diff/1/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1038863003/diff/1/gyp/codec.gyp#newcode16 gyp/codec.gyp:16: 'giflib.gyp:giflib', On 2015/03/26 19:51:05, djsollen wrote: > no need to add it here until you have code in this target that uses it. True, I will move this to my gif CL. https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp File gyp/giflib.gyp (left): https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp#oldcode14 gyp/giflib.gyp:14: [ 'skia_giflib_static', On 2015/03/26 18:29:34, scroggo wrote: > On 2015/03/26 18:22:46, msarett wrote: > > As far as I can tell, this is always 0. So past code always used -lgif. > > Dead code. We should probably remove skia_giflib_static. Done. https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1038863003/diff/1/gyp/giflib.gyp#newcode38 gyp/giflib.gyp:38: }, { # skia_os == "android" On 2015/03/26 20:10:11, scroggo wrote: > On 2015/03/26 19:51:05, djsollen wrote: > > let's not make the distinction between android and other platforms for libgif > as > > they all build using the same sources. > > We *do* need to make a distinction for skia_android_framework though. I think > android_deps takes care of that for you, so you'll need to move that code here. Gotcha. https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... platform_tools/android/gyp/dependencies.gypi:84: 'target_name': 'png', On 2015/03/26 19:51:06, djsollen wrote: > I think this and jpeg are missing a conditional block where you add the platform > specific sources and flags when appropriate. Yes, I fixed this. https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... platform_tools/android/gyp/dependencies.gypi:89: '../third_party/externals/png/arm/filter_neon_intrinsics.c', On 2015/03/26 19:51:06, djsollen wrote: > I don't think we can always depend on this. what happens if you try android_make > -d xoom which is a target that doesn't support neon? Agreed. https://codereview.chromium.org/1038863003/diff/1/platform_tools/android/gyp/... platform_tools/android/gyp/dependencies.gypi:130: #'../third_party/externals/jpeg/djpeg.c', On 2015/03/26 18:29:34, scroggo wrote: > On 2015/03/26 18:22:46, msarett wrote: > > Certain new files are causing android not to build: > > '../third_party/externals/jpeg/cjpeg.c', > > '../third_party/externals/jpeg/djpeg.c', > > '../third_party/externals/jpeg/jmem-ashmem.c', > > '../third_party/externals/jpeg/mips_jidctfst.c', > > '../third_party/externals/jpeg/mips_idct_le.S', > > > > Let me know if you have any idea why. Otherwise, I will look into this > further. > > I *think* some of these are not part of the library, but separate programs (e.g. > djpeg.c). > > mips will only compile on mips platforms, presumably, so these will need to be > conditional. You are right on both counts.
LGTM. When you land, warn the team (skia-staff, I think?) that they need to do a gclient sync. Also, when I landed a similar change for PNG, all kinds of platforms I hadn't tested on broke, so make sure to land it when you have time to watch the tree. Also it would be good to kick off some trybots. https://codereview.chromium.org/1038863003/diff/60001/platform_tools/android/... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/60001/platform_tools/android/... platform_tools/android/gyp/dependencies.gypi:131: [ 'skia_arch_type in [ "x86", "x86_64" ]', This could also be: '"x86" in skia_arch_type' Not sure if that's better.
https://codereview.chromium.org/1038863003/diff/80001/gyp/images.gyp File gyp/images.gyp (right): https://codereview.chromium.org/1038863003/diff/80001/gyp/images.gyp#newcode138 gyp/images.gyp:138: 'giflib.gyp:giflib', do all platforms use this now? If so it can be added to the main dependencies list and removed from all of these conditional dependencies. https://codereview.chromium.org/1038863003/diff/80001/platform_tools/android/... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/80001/platform_tools/android/... platform_tools/android/gyp/dependencies.gypi:68: 'sources' : [ don't these need to be accompanied by some defines as well.
https://codereview.chromium.org/1038863003/diff/80001/DEPS File DEPS (right): https://codereview.chromium.org/1038863003/diff/80001/DEPS#newcode9 DEPS:9: # - both Android and ChromeOS pull the same giflib; No longer need this comment, since you've switched them to using the same one. https://codereview.chromium.org/1038863003/diff/80001/gyp/images.gyp File gyp/images.gyp (right): https://codereview.chromium.org/1038863003/diff/80001/gyp/images.gyp#newcode138 gyp/images.gyp:138: 'giflib.gyp:giflib', On 2015/03/27 01:20:51, djsollen wrote: > do all platforms use this now? If so it can be added to the main dependencies > list and removed from all of these conditional dependencies. Windows does not.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
I've made some non-trivial changes to fix issues with the trybots. I'd appreciate getting another look before moving forward. https://codereview.chromium.org/1038863003/diff/60001/platform_tools/android/... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/60001/platform_tools/android/... platform_tools/android/gyp/dependencies.gypi:131: [ 'skia_arch_type in [ "x86", "x86_64" ]', On 2015/03/26 21:15:02, scroggo wrote: > This could also be: > > '"x86" in skia_arch_type' > > Not sure if that's better. Done. https://codereview.chromium.org/1038863003/diff/80001/DEPS File DEPS (right): https://codereview.chromium.org/1038863003/diff/80001/DEPS#newcode9 DEPS:9: # - both Android and ChromeOS pull the same giflib; On 2015/03/27 12:38:51, scroggo wrote: > No longer need this comment, since you've switched them to using the same one. Done. https://codereview.chromium.org/1038863003/diff/80001/platform_tools/android/... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/80001/platform_tools/android/... platform_tools/android/gyp/dependencies.gypi:68: 'sources' : [ On 2015/03/27 01:20:51, djsollen wrote: > don't these need to be accompanied by some defines as well. It seems that way, they are breaking the trybots. I have fixed this.
https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:65: 'conditions': [ as a style note we usually put the conditions after the main sources dictionary. https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:66: [ 'arm_neon == 1', need to add -DPNG_ARM_NEON_OPT=2 https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:117: 'sources' : [ -DNV_ARM_NEON https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:124: [ 'skia_arch_type == "mips" and mips_dsp == 2', -DANDROID_MIPS_IDCT https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:134: 'sources' : [ -DANDROID_INTELSSE2_IDCT
Added dependencies! and flags https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:65: 'conditions': [ On 2015/03/27 14:23:28, djsollen wrote: > as a style note we usually put the conditions after the main sources dictionary. Done. https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:66: [ 'arm_neon == 1', On 2015/03/27 14:23:28, djsollen wrote: > need to add -DPNG_ARM_NEON_OPT=2 Thanks! https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:117: 'sources' : [ On 2015/03/27 14:23:28, djsollen wrote: > -DNV_ARM_NEON Done. https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:124: [ 'skia_arch_type == "mips" and mips_dsp == 2', On 2015/03/27 14:23:28, djsollen wrote: > -DANDROID_MIPS_IDCT Done. https://codereview.chromium.org/1038863003/diff/220001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:134: 'sources' : [ On 2015/03/27 14:23:28, djsollen wrote: > -DANDROID_INTELSSE2_IDCT Done. https://codereview.chromium.org/1038863003/diff/240001/gyp/images.gyp File gyp/images.gyp (right): https://codereview.chromium.org/1038863003/diff/240001/gyp/images.gyp#newcode126 gyp/images.gyp:126: # FIXME: NaCl should be just like linux, etc, above, but it currently is separated out Just noticed this comment. If this is still the case, I think it means that I need an extra dependencies! for NaCl. If not, should I try to merge with linux?
lgtm with one style nit https://codereview.chromium.org/1038863003/diff/240001/platform_tools/android... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/240001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:90: 'flags' : [ if you use defines you can instead of flags you can drop the -D off the front.
https://codereview.chromium.org/1038863003/diff/240001/gyp/images.gyp File gyp/images.gyp (right): https://codereview.chromium.org/1038863003/diff/240001/gyp/images.gyp#newcode126 gyp/images.gyp:126: # FIXME: NaCl should be just like linux, etc, above, but it currently is separated out On 2015/03/27 14:47:29, msarett wrote: > Just noticed this comment. If this is still the case, I think it means that I > need an extra dependencies! for NaCl. If not, should I try to merge with linux? I think we don't support nacl anymore? Pretty sure we just killed the bot so we can support C++11, which nacl doesn't support. So I think we already don't work for nacl. Maybe a separate change should remove all the nacl code though. https://codereview.chromium.org/1038863003/diff/240001/platform_tools/android... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/240001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:90: 'flags' : [ I think this should be 'defines': [ 'PNG_ARM_NEON_OPT=2', ], Also, I think we generally follow everything with a comma for future proofing. Not sure whether 'flags' is supported, but you can use 'cflags' or 'cppflags' (in this case you do need the "-D"). For things that will get used by skia_for_android_framework, I prefer 'defines', since those are treated differently in that build, but this is not used for the android framework, so I don't have a strong preference.
Defines and style fixes https://codereview.chromium.org/1038863003/diff/240001/platform_tools/android... File platform_tools/android/gyp/dependencies.gypi (right): https://codereview.chromium.org/1038863003/diff/240001/platform_tools/android... platform_tools/android/gyp/dependencies.gypi:90: 'flags' : [ On 2015/03/27 14:55:46, scroggo wrote: > I think this should be > > 'defines': [ > 'PNG_ARM_NEON_OPT=2', > ], > > Also, I think we generally follow everything with a comma for future proofing. > > Not sure whether 'flags' is supported, but you can use 'cflags' or 'cppflags' > (in this case you do need the "-D"). > > For things that will get used by skia_for_android_framework, I prefer 'defines', > since those are treated differently in that build, but this is not used for the > android framework, so I don't have a strong preference. Done.
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, djsollen@google.com Link to the patchset: https://codereview.chromium.org/1038863003/#ps260001 (title: "Style stuff and defines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038863003/260001
Message was sent while issue was closed.
Committed patchset #8 (id:260001) as https://skia.googlesource.com/skia/+/255dcd11992ebe74eb54202c48cf5394d33a8ce6
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:260001) has been created in https://codereview.chromium.org/1048713003/ by borenet@google.com. The reason for reverting is: Trying out revert to see if it fixes Android bots..
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, djsollen@google.com Link to the patchset: https://codereview.chromium.org/1038863003/#ps300001 (title: "removing update to libpng")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038863003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for DEPS:
While running git apply --index -3 -p1;
error: patch failed: DEPS:23
Falling back to three-way merge...
Applied patch to 'DEPS' with conflicts.
U DEPS
Patch: DEPS
Index: DEPS
diff --git a/DEPS b/DEPS
index
0de145e3b8eb867dfba7efc5f53e2f6c5c16dcef..e369f804a576a33967ba4cec1d61961b7c7ec2d0
100644
--- a/DEPS
+++ b/DEPS
@@ -23,7 +23,7 @@ deps = {
"platform_tools/android/third_party/externals/expat" :
"https://android.googlesource.com/platform/external/expat.git@android-5.1.0_r3",
"platform_tools/android/third_party/externals/jpeg" :
"https://android.googlesource.com/platform/external/jpeg.git@android-5.1.0_r3",
- "platform_tools/android/third_party/externals/png" :
"https://android.googlesource.com/platform/external/libpng.git@android-5.1.0_r3",
+ "platform_tools/android/third_party/externals/png" :
"https://android.googlesource.com/platform/external/libpng.git@android-4.2.2_r1.2",
"platform_tools/chromeos/toolchain/src/third_party/chromite":
"https://chromium.googlesource.com/chromiumos/chromite.git@d6a4c7e0ee4d53ddc5238dbddfc0417796a70e54",
"platform_tools/chromeos/toolchain/src/third_party/pyelftools":
"https://chromium.googlesource.com/chromiumos/third_party/pyelftools.git@bdc1d380acd88d4bfaf47265008091483b0d614e",
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, djsollen@google.com Link to the patchset: https://codereview.chromium.org/1038863003/#ps320001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038863003/320001
Message was sent while issue was closed.
Committed patchset #11 (id:320001) as https://skia.googlesource.com/skia/+/6d0e7b2031f8eee4e88905082de0506cfbb2bfc3 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
