Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
300013:
Add yasm and ffmpeg into the build tree for linux. (Closed)
|
Created:
11 years, 2 months ago by awong Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Visibility:
Public. |
DescriptionAdd yasm and ffmpeg into the build tree for linux.
BUG=22307
TEST=builds correctly, and an html5 video runs.
Patch Set 1 #
Total comments: 12
Patch Set 2 : Add in stuff needed to build. #Patch Set 3 : Better version of the script. #Patch Set 4 : Add TODOs marking examples of issues #
Total comments: 25
Patch Set 5 : More change #Patch Set 6 : ffmpeg stable. #Patch Set 7 : Almost there #
Total comments: 100
Patch Set 8 : Address Mark's comments. #Patch Set 9 : EVerything works except -O0. #Patch Set 10 : Fixes for scons. #
Total comments: 1
Patch Set 11 : Move the libffmpegsumo.so into the right spot and fix the installer. #Patch Set 12 : Remove silly newline. #Patch Set 13 : Make things buildable w/o -fomit-frame-pointer #Patch Set 14 : The I HATE SCONS patch. #Patch Set 15 : The I HATE SCONS - 2: "typo hell" patch. #Patch Set 16 : Clean up wording. #Patch Set 17 : Try again #Patch Set 18 : use STATIC_LIB_PREFIX #Patch Set 19 : Use a brand-new-gyp feature to make code scons agnostic. I repeat. Scons needs to die. #
Total comments: 20
Patch Set 20 : Address Andrew's comments. #Patch Set 21 : gold can't do -l:libz.so. Rename zlib and bzip2 targets. #
Total comments: 20
Patch Set 22 : Fix mark's comments. #Messages
Total messages: 35 (0 generated)
I know you said it's not ready for review but here are my comments anyway :) What I'm wondering about is whether we're trading one set of maintenance nightmares for another (i.e., writing gyp files for N different flavours of FFmpeg source). Lesser of two evils I suppose! http://codereview.chromium.org/300013/diff/1/4 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/1/4#newcode26 Line 26: 1) Get a clean version of the patched tree. wrap non-command lines at 80 chars --- makes reviewing the change nicer in rietveld ;) http://codereview.chromium.org/300013/diff/1/4#newcode30 Line 30: 4) Copy that into the correct platform specific directory in what is "that"? config/version.h, the diff, the clean tree or the patched tree? http://codereview.chromium.org/300013/diff/1/4#newcode34 Line 34: make -j5 libavcodec/ibavcodec.so > /tmp/ffmpeg_build_log 2> /tmp/ffmpeg_build_err quick question for a make noob such as myself.. is it ok to have -j specified? i.e., is this a portable option that's safe to run? http://codereview.chromium.org/300013/diff/1/4#newcode35 Line 35: 7) Check /tmp/ffmpeg_build_err to see if there are any anomolies beyond anomolies -> anomalies http://codereview.chromium.org/300013/diff/1/4#newcode47 Line 47: This should find all gcc commands, exlude the dependency generation lines, the link lines, exlude -> exclude http://codereview.chromium.org/300013/diff/1/4#newcode57 Line 57: sed -e "s|.* -o .* \(.*\)$|'sources/patched-ffmpeg-mt/\1',|" | do you mean to have patched-ffmpeg-mt here? http://codereview.chromium.org/300013/diff/1/6 File third_party/yasm/README.chromium (right): http://codereview.chromium.org/300013/diff/1/6#newcode2 Line 2: -- Recreating the gyp file. this should contain some mention of our usage and the original sources etc... instead of duplicating it might be better to link to the other README.chromium (or vice-versa) http://codereview.chromium.org/300013/diff/1/7 File third_party/yasm/run_in_dir.sh (right): http://codereview.chromium.org/300013/diff/1/7#newcode5 Line 5: # Author: ajwong@google.com (Albert Wong) should be chromium copyright header
:-/ I'll fix those later. The instructions need an overhaul and style is...well...not fixed yet. Interesting development though. If I build < O0, there is no problem! Only if I build at -O0, does it spew this issue. Ideas? -Albert On Mon, Oct 19, 2009 at 6:26 PM, <scherkus@chromium.org> wrote: > I know you said it's not ready for review but here are my comments anyway > :) > > > What I'm wondering about is whether we're trading one set of maintenance > nightmares for another (i.e., writing gyp files for N different flavours of > FFmpeg source). Lesser of two evils I suppose! > > > http://codereview.chromium.org/300013/diff/1/4 > File third_party/ffmpeg/README.chromium (right): > > http://codereview.chromium.org/300013/diff/1/4#newcode26 > Line 26: 1) Get a clean version of the patched tree. > wrap non-command lines at 80 chars --- makes reviewing the change nicer > in rietveld ;) > > http://codereview.chromium.org/300013/diff/1/4#newcode30 > Line 30: 4) Copy that into the correct platform specific directory in > what is "that"? config/version.h, the diff, the clean tree or the > patched tree? > > http://codereview.chromium.org/300013/diff/1/4#newcode34 > Line 34: make -j5 libavcodec/ibavcodec.so > /tmp/ffmpeg_build_log 2> > /tmp/ffmpeg_build_err > quick question for a make noob such as myself.. is it ok to have -j > specified? i.e., is this a portable option that's safe to run? > > http://codereview.chromium.org/300013/diff/1/4#newcode35 > Line 35: 7) Check /tmp/ffmpeg_build_err to see if there are any > anomolies beyond > anomolies -> anomalies > > http://codereview.chromium.org/300013/diff/1/4#newcode47 > Line 47: This should find all gcc commands, exlude the dependency > generation lines, the link lines, > exlude -> exclude > > http://codereview.chromium.org/300013/diff/1/4#newcode57 > Line 57: sed -e "s|.* -o .* \(.*\)$|'sources/patched-ffmpeg-mt/\1',|" | > do you mean to have patched-ffmpeg-mt here? > > http://codereview.chromium.org/300013/diff/1/6 > File third_party/yasm/README.chromium (right): > > http://codereview.chromium.org/300013/diff/1/6#newcode2 > Line 2: -- Recreating the gyp file. > this should contain some mention of our usage and the original sources > etc... > > instead of duplicating it might be better to link to the other > README.chromium (or vice-versa) > > http://codereview.chromium.org/300013/diff/1/7 > File third_party/yasm/run_in_dir.sh (right): > > http://codereview.chromium.org/300013/diff/1/7#newcode5 > Line 5: # Author: ajwong@google.com (Albert Wong) > should be chromium copyright header > > http://codereview.chromium.org/300013 >
I went to build this to see if I could debug what you were seeing, but I got: mark@ebony bash$ hammer --mode=Debug scons: Reading SConscript files ... scons: *** While building `['/chrome/trunk/src/sconsbuild/Debug/obj/ffmpeg_source/_libavcodec_intermediate/dsputil_yasm.os']' from `['/chrome/trunk/src/sconsbuild/Debug/obj/ffmpeg_source/_libavcodec_intermediate/dsputil_yasm.o']': Don't know how to build from a source file with suffix `.o'. Expected a suffix in this list: ['.s', '.asm', '.ASM', '.spp', '.SPP', '.sx', '.S', '.c', '.m', '.cpp', '.cc', '.cxx', '.c++', '.C++', '.mm', '.C']. File "/chrome/trunk/src/third_party/ffmpeg/libavcodec.scons", line 423, in <module> http://codereview.chromium.org/300013/diff/1/7 File third_party/yasm/run_in_dir.sh (right): http://codereview.chromium.org/300013/diff/1/7#newcode1 Line 1: #!/bin/bash svn:executable, svn:eol-style http://codereview.chromium.org/300013/diff/1/7#newcode9 Line 9: pushd $1 Safety Dance: This should be "${1}" with, y'know, quotes. http://codereview.chromium.org/300013/diff/1/7#newcode11 Line 11: $* And this, "${@}". http://codereview.chromium.org/300013/diff/1/7#newcode12 Line 12: popd Since you never do anything else after this, it's pointless. The script should be: set -e cd "${1}" shift exec "${@}"
I think that what you've uploaded is incomplete because even when I bypass all of the yasm stuff to try to just start compiling some of ffmpeg, I get stuff like: Compiling /chrome/trunk/src/sconsbuild/Debug/obj/yasm/genperf_libs/source/yasm/libyasm/xmalloc.o In file included from /chrome/trunk/src/third_party/yasm/source/yasm/libyasm/phash.c:2: /chrome/trunk/src/third_party/yasm/source/yasm/util.h:34:20: error: config.h: No such file or directory /chrome/trunk/src/third_party/yasm/source/yasm/util.h:67:28: error: libyasm-stdint.h: No such file or directory [...]
:-/
Did you do a gclient sync? That file should be there..
The original patch was very not ready for use...I've uploaded a new patch
that includes some of the configuration files required. Hopefully that will
make things kick along if you use the make build (that's what I'm developing
on).
make yasm
make libavcodec.so
respectively.
Regarding the run_in_dir.sh, is there a better way to do that?
Also, I'm pretty sure the scons build won't work. There's a few places where
I used ${PWD} as a temproary hack to generate the path batch to the source
tree, cause I couldn't figure out what the right gyp define was for that.
Those probably need to be fixed before the scons build will work, unless
PWD happens to line up with the behavior in make.
-Albert
On Mon, Oct 19, 2009 at 7:01 PM, <mark@chromium.org> wrote:
> I went to build this to see if I could debug what you were seeing, but I
> got:
>
> mark@ebony bash$ hammer --mode=Debug
> scons: Reading SConscript files ...
>
> scons: *** While building
>
>
`['/chrome/trunk/src/sconsbuild/Debug/obj/ffmpeg_source/_libavcodec_intermediate/dsputil_yasm.os']'
> from
>
>
`['/chrome/trunk/src/sconsbuild/Debug/obj/ffmpeg_source/_libavcodec_intermediate/dsputil_yasm.o']':
> Don't know how to build from a source file with suffix `.o'. Expected a
> suffix
> in this list: ['.s', '.asm', '.ASM', '.spp', '.SPP', '.sx', '.S', '.c',
> '.m',
> '.cpp', '.cc', '.cxx', '.c++', '.C++', '.mm', '.C'].
> File "/chrome/trunk/src/third_party/ffmpeg/libavcodec.scons", line 423, in
> <module>
>
>
> http://codereview.chromium.org/300013/diff/1/7
> File third_party/yasm/run_in_dir.sh (right):
>
> http://codereview.chromium.org/300013/diff/1/7#newcode1
> Line 1: #!/bin/bash
> svn:executable, svn:eol-style
>
> http://codereview.chromium.org/300013/diff/1/7#newcode9
> Line 9: pushd $1
> Safety Dance:
>
> This should be "${1}" with, y'know, quotes.
>
> http://codereview.chromium.org/300013/diff/1/7#newcode11
> Line 11: $*
> And this, "${@}".
>
> http://codereview.chromium.org/300013/diff/1/7#newcode12
> Line 12: popd
> Since you never do anything else after this, it's pointless.
>
> The script should be:
>
> set -e
> cd "${1}"
> shift
> exec "${@}"
>
>
> http://codereview.chromium.org/300013
>
Hey Mark, Things work now, so I'm trying to clean things up. The yasm.gyp file is ready for a first pass review. I'm running into 4 problems that I can't solve though. 1) The "yasm" target starts building before its dependencies finish on a parallel build (make -j5 yasm) which causes the actions to fail. I tried adding "hard_dependency": 1 into the targets that yasm depends on, but no luck. Any ideas? 2) For the commands that require a chdir before invocation, I need the absolute path to the source file. Is there a gyp variable that will tell me the the absolute path to the top of the source tree? I was thinking of invoking realpath or readlink from the run_in_dir.sh script, but that's not a script dependency that I'd like to take. 3) The 'generate_x86_insn' action runs every time I type 'make yasm' causing a relink. This seems wrong. 4) This isn't in the yasm file, but will become an issue when I get to ffmpeg. Is there a way to override optimization levels on a per target basis? Aside from the dead-branch issue, building ffmpeg at O0 is going to make it painfully slow, and make video on the debug build kinda suck. It'd be nice to build debug, and still get an optimized binary for ffmpeg so we can do debugging on our code w/o optimizations and treat ffmpeg as a black-box. Ideally, I'd like to be able to do something like GYP_DEFINES='ffmpeg_opt=2' make media_bench chrome or similar and get an ffmpeg built with -O2. Any ideas on how to proceed with the above points?
I also added TODO(ajwong) in the lastest patch for each of the 4 issues I described. Search for that to see the problem points. On 2009/10/21 01:31:44, awong wrote: > Hey Mark, > > Things work now, so I'm trying to clean things up. The yasm.gyp file is ready > for a first pass review. > > I'm running into 4 problems that I can't solve though. > > 1) The "yasm" target starts building before its dependencies finish on a > parallel build (make -j5 yasm) which causes the actions to fail. > > I tried adding "hard_dependency": 1 into the targets that yasm depends on, but > no luck. Any ideas? > > 2) For the commands that require a chdir before invocation, I need the absolute > path to the source file. > > Is there a gyp variable that will tell me the the absolute path to the top of > the source tree? I was thinking of invoking realpath or readlink from the > run_in_dir.sh script, but that's not a script dependency that I'd like to take. > > 3) The 'generate_x86_insn' action runs every time I type 'make yasm' causing a > relink. This seems wrong. > > 4) This isn't in the yasm file, but will become an issue when I get to ffmpeg. > Is there a way to override optimization levels on a per target basis? > > Aside from the dead-branch issue, building ffmpeg at O0 is going to make it > painfully slow, and make video on the debug build kinda suck. It'd be nice to > build debug, and still get an optimized binary for ffmpeg so we can do debugging > on our code w/o optimizations and treat ffmpeg as a black-box. > > Ideally, I'd like to be able to do something like > > GYP_DEFINES='ffmpeg_opt=2' make media_bench chrome > > or similar and get an ffmpeg built with -O2. > > Any ideas on how to proceed with the above points?
This is not a full pass, but I don't know which ffmpeg gyp you want me to look at yet. I didn't scrutinize everything as carefully as I will on the next round. http://codereview.chromium.org/300013/diff/2015/2018 File media/base/media_posix.cc (right): http://codereview.chromium.org/300013/diff/2015/2018#newcode45 Line 45: return FILE_PATH_LITERAL("libffmpegsumo.so"); :) on the name. This should still do the .so/.dylib thing although I know you probably haven't tested this on the Mac yet. If you want to do the Mac work in a separate change, you need to make sure that the old code above is still operative on the Mac, right? http://codereview.chromium.org/300013/diff/2015/2019 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/2015/2019#newcode34 Line 34: make -j5 libavcodec/libavcodec.so libavformat/libavformat.so > /tmp/ffmpeg_build_log 2> /tmp/ffmpeg_build_err Wrap this (maybe with a backslash or something?) http://codereview.chromium.org/300013/diff/2015/2019#newcode39 Line 39: There should be two yasm files, but nothing else. Include those yasm files in 80 http://codereview.chromium.org/300013/diff/2015/2019#newcode41 Line 41: 8) Verify that the gcc commands all have the same compiler flags. Do that with the following. 80 More 80s follow. http://codereview.chromium.org/300013/diff/2015/2019#newcode60 Line 60: 4 blank lines at the end not helping anything. http://codereview.chromium.org/300013/diff/2015/2021 File third_party/ffmpeg/ffmpeg_source.gyp (right): http://codereview.chromium.org/300013/diff/2015/2021#newcode1 Line 1: # Copyright (c) 2009 The Chromium Authors. All rights reserved. Do we want to use this all-new file or the new sections in the other file? http://codereview.chromium.org/300013/diff/2015/2023 File third_party/yasm/README.chromium (right): http://codereview.chromium.org/300013/diff/2015/2023#newcode16 Line 16: There should be two yasm files, but nothing else. Include those yasm files in More 80 http://codereview.chromium.org/300013/diff/2015/2023#newcode39 Line 39: And more of this. http://codereview.chromium.org/300013/diff/2015/2023#newcode39 Line 39: And more of this. http://codereview.chromium.org/300013/diff/2015/2025 File third_party/yasm/source/config/linux/Makefile (right): http://codereview.chromium.org/300013/diff/2015/2025#newcode1 Line 1: # Makefile.in generated by automake 1.10.1 from Makefile.am. Do we need this checked in? http://codereview.chromium.org/300013/diff/2015/2028 File third_party/yasm/yasm.gyp (right): http://codereview.chromium.org/300013/diff/2015/2028#newcode4 Line 4: # Get rid of the # on this line so that it's entirely blank. That will help distinguish the boilerplace (or as I like to call it "the lice") which nobody cares about from your comments which at least I care about. http://codereview.chromium.org/300013/diff/2015/2028#newcode351 Line 351: # TODO(ajwong): Why does this run on every make even if nothing's Probably because the action isn't actually touching all of the declared outputs, or one of the declared inputs doesn't exist (or keeps getting touched by something else in between "make"s). http://codereview.chromium.org/300013/diff/2015/2028#newcode359 Line 359: '<(SHARED_INTERMEDIATE_DIR)/yasm/x86insns.c', These should be <(shared_generated_dir) instead of <(SHARED_INTERMEDIATE_DIR)/yasm, right? http://codereview.chromium.org/300013/diff/2015/2028#newcode367 Line 367: # TODO(ajwong): How do I fix this path? Oh, because run_in_dir is going to change directories, so the path that's relative to this directory won't work any more! Harder problem to solve now. I guess you could have an action before this one to copy gen_x86_insn.py to shared_generated_dir. What can you tell me about gen_x86_insn.py? Can we patch it to take the directory to output stuff into as an argument instead of having to wrap it in run_in_dir.sh? gyp is intentionally very anti-absolute path, because I'm very anti-absolute path. In a build system, they're your enemy.
Thanks Mark! I've responded to some of your comments. If you look at this again, I'd ignore most of the files beyond yasm.gyp and ffmpeg.gyp right now. They're not clean yet (working on it!) The big problems are as outlined in the notes from my last mailing. Specifically 1) yasm target actions running before its dependencies finish building 4) Wanting to specify a different optimization level for building ffmpeg from the rest of the system. Questions 2 & 3 got covered by your inline comments. -Albert http://codereview.chromium.org/300013/diff/2015/2018 File media/base/media_posix.cc (right): http://codereview.chromium.org/300013/diff/2015/2018#newcode45 Line 45: return FILE_PATH_LITERAL("libffmpegsumo.so"); On 2009/10/21 01:55:24, Mark Mentovai wrote: > :) on the name. He he...got the idea from xemacs's sumo tarball. > This should still do the .so/.dylib thing although I know you probably haven't > tested this on the Mac yet. > > If you want to do the Mac work in a separate change, you need to make sure that > the old code above is still operative on the Mac, right? Yep yep. This was just test code. Will clean it up after I get yasm to stop racing against itself. I will indeed be doing mac in a separate change. One platform at a time is quite enough. :P http://codereview.chromium.org/300013/diff/2015/2019 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/2015/2019#newcode34 Line 34: make -j5 libavcodec/libavcodec.so libavformat/libavformat.so > /tmp/ffmpeg_build_log 2> /tmp/ffmpeg_build_err On 2009/10/21 01:55:24, Mark Mentovai wrote: > Wrap this (maybe with a backslash or something?) I'm going to be copy-editing these instructions after I get the gyp files stable. Right now, they're pretty much "albert notes." http://codereview.chromium.org/300013/diff/2015/2021 File third_party/ffmpeg/ffmpeg_source.gyp (right): http://codereview.chromium.org/300013/diff/2015/2021#newcode1 Line 1: # Copyright (c) 2009 The Chromium Authors. All rights reserved. On 2009/10/21 01:55:24, Mark Mentovai wrote: > Do we want to use this all-new file or the new sections in the other file? The new section in the old file. http://codereview.chromium.org/300013/diff/2015/2025 File third_party/yasm/source/config/linux/Makefile (right): http://codereview.chromium.org/300013/diff/2015/2025#newcode1 Line 1: # Makefile.in generated by automake 1.10.1 from Makefile.am. On 2009/10/21 01:55:24, Mark Mentovai wrote: > Do we need this checked in? Unforutnately...yes. genmodule needs this. I guess we could checkin the file created by genmodule instead. But taking in the makefile means less steps when updating yasm. Also, all these dependencies are going to go out of this CL. http://codereview.chromium.org/300013/diff/2015/2028 File third_party/yasm/yasm.gyp (right): http://codereview.chromium.org/300013/diff/2015/2028#newcode351 Line 351: # TODO(ajwong): Why does this run on every make even if nothing's On 2009/10/21 01:55:24, Mark Mentovai wrote: > Probably because the action isn't actually touching all of the declared outputs, > or one of the declared inputs doesn't exist (or keeps getting touched by > something else in between "make"s). Huh...okay, I'll debug further on htis side. http://codereview.chromium.org/300013/diff/2015/2028#newcode359 Line 359: '<(SHARED_INTERMEDIATE_DIR)/yasm/x86insns.c', On 2009/10/21 01:55:24, Mark Mentovai wrote: > These should be <(shared_generated_dir) instead of > <(SHARED_INTERMEDIATE_DIR)/yasm, right? yep. Good catch! Fixed locally. http://codereview.chromium.org/300013/diff/2015/2028#newcode367 Line 367: # TODO(ajwong): How do I fix this path? On 2009/10/21 01:55:24, Mark Mentovai wrote: > Oh, because run_in_dir is going to change directories, so the path that's > relative to this directory won't work any more! > > Harder problem to solve now. I guess you could have an action before this one > to copy gen_x86_insn.py to shared_generated_dir. > > What can you tell me about gen_x86_insn.py? Can we patch it to take the > directory to output stuff into as an argument instead of having to wrap it in > run_in_dir.sh? > > gyp is intentionally very anti-absolute path, because I'm very anti-absolute > path. In a build system, they're your enemy. And relative paths make symlinks cry. :) But point taken. Anyways, genereate_x86_insn seems to generate a bunch of C constants that describe various asm statements. example output: static const x86_insn_info fcom2_insn[] = { { 0, 0, CPU_286, CPU_FPU, 0, {MOD_Op0Add, MOD_Op1Add, 0}, 0, 0, 0, 0, 2, {0x00, 0x00, 0}, 0, 1, 314 }, { 0, 0, CPU_286, CPU_FPU, 0, {MOD_Op0Add, MOD_Op1Add, 0}, 0, 0, 0, 0, 2, {0x00, 0x00, 0}, 0, 2, 313 } }; Patching this in yasm (as well as the other spot) is quite doable. However, the yasm build is currently pristine source. I guess I could copy the files from the source dir into the gen dir, patch them, and then run that result. Feels a bit hacky. Long term, I should probably just submit a patch to yasm upstream. I'll try patching inside INTERMEDIATE_DIR for now though.
OK, I've answered the rest of your TODO comments. Once you've worked through these, we'll do another round and see where things stand. I'm happy to see this work being done! http://codereview.chromium.org/300013/diff/2015/2020 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/2015/2020#newcode34 Line 34: 'variables': { We have release_optimize and debug_optimize for Linux. We have mac_release_optimization and mac_debug_optimization for Mac. You can set them to 0, 1, 2, 3, s, or anything else that would come after -O. It should work to set these as variables within a target that you want to override the defaults for. It's an unfortunate accident that maybe we'll clean up some day that Mac and Linux don't share the same variables. If you decide that you want to build debug-mode ffmpeg at -O1 always, overriding whatever else might be going on, you should be able to put here in this target's variables dict: 'debug_optimize': '1', 'mac_debug_optimization': '1', If you need to start adding custom flags, on Linux, you'll want to play with the cflags property, which is a list. Tweaking other build settings on the Mac is a little bit more involved. I know you're not there yet, but when you get there, we can talk about what, if anything, you need. We'll have the advantage of having whatever example you're setting first on Linux to use as guidance. http://codereview.chromium.org/300013/diff/2015/2020#newcode35 Line 35: 'target_for_binaries': 'libffmpegsumo', This still makes me smile. http://codereview.chromium.org/300013/diff/2015/2020#newcode44 Line 44: # TODO(ajwong): relocaiton error for fpic again for zlib. I'll need to try a build to see about these. Remind me about this tomorrow if you haven't figured it out. http://codereview.chromium.org/300013/diff/2015/2028 File third_party/yasm/yasm.gyp (right): http://codereview.chromium.org/300013/diff/2015/2028#newcode315 Line 315: '<(PRODUCT_DIR)/genmodule', OK, so genmodule's not ready yet here? You should add '<(PRODUCT_DIR)/genmodule' to inputs.
Okay...this time, *all* the code should be ready for a review. :) Thanks for dealing with the in-progress CLs previously. There is still one outstanding issue. In ffmpeg.gyp, the output of the yasm action is a .o file. I don't know how to feed that back into gyp so that it ends up in the link line for libffmpegsumo.so. We're still missing the config.hs for the chromium version of ffmpeg. Will add that tomorrow.
OK then, here's some feedback for you! http://codereview.chromium.org/300013/diff/8003/8004 File DEPS (right): http://codereview.chromium.org/300013/diff/8003/8004#newcode138 Line 138: "http://www.tortall.net/svn/yasm/trunk/yasm@2192", This would be a first. So far, all of the SVN severs we depend on fall into three categories: 1. our own server 2. googlecode.com 3. webkit.org I've historically been very reluctant to add any servers that don't fit into these categories because it adds another remote server that our builds need to depend on, and it potentially places a heavy load on that server when all of our bots and developers pick this up. This kind of thing has actually gotten our buildbots blacklisted by the third category in the past, when severe misconfigurations caused everything to clobber and try pulling fresh WebKit trees. For this one, webkit.org's right out, and since yasm isn't on googlecode.com, I would advise checking in a snapshot of yasm into /trunk/deps. http://codereview.chromium.org/300013/diff/8003/8006 File media/base/media_posix.cc (right): http://codereview.chromium.org/300013/diff/8003/8006#newcode32 Line 32: #if defined(OS_POSIX) Mac is OS_POSIX too. You want OS_LINUX. http://codereview.chromium.org/300013/diff/8003/8007 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/8003/8007#newcode21 Line 21: libraries to permit dynamically loading FFmpeg. On windows, the libraries are windows is Windows. http://codereview.chromium.org/300013/diff/8003/8007#newcode38 Line 38: The FFmpeg build is relatively straight forward. All files are build with straightforward http://codereview.chromium.org/300013/diff/8003/8007#newcode48 Line 48: this. An error like: "An error like: [error]" EOF? Finish your thought? http://codereview.chromium.org/300013/diff/8003/8007#newcode53 Line 53: 2) On ia32, FFmpeg cannot be built with -fPIC, again due to assembly I'll be curious to see how this plays out on the Mac where -fPIC is the default. http://codereview.chromium.org/300013/diff/8003/8007#newcode64 Line 64: cross-compiling for 32-bit on a 64-bit machine. Since yasm is build in built http://codereview.chromium.org/300013/diff/8003/8007#newcode72 Line 72: Detail Directions: Detailed http://codereview.chromium.org/300013/diff/8003/8007#newcode99 Line 99: Make to double-check that config.h and version.h are the only files of sure? Make ^ to double-check http://codereview.chromium.org/300013/diff/8003/8007#newcode108 Line 108: make -j5 libavcodec/libavcodec.so libavformat/libavformat.so \ To use a build log as a reference, -j might work against you. http://codereview.chromium.org/300013/diff/8003/8007#newcode112 Line 112: anomalies. FFmpeg source generates a lot of compiler warnings; it sigh. It wouldn't if I were on that project, I can tell you that much. :) http://codereview.chromium.org/300013/diff/8003/8007#newcode118 Line 118: grep -Ev '^gcc' ffmpeg_build_log There's nothing extended about this regular expression, you can just say "grep -v". http://codereview.chromium.org/300013/diff/8003/8007#newcode126 Line 126: grep -E '^gcc' ffmpeg_build_log | no -E http://codereview.chromium.org/300013/diff/8003/8007#newcode130 Line 130: sort | uniq -c One of my side hobbies is improving shell pipelines, but I'll resist the temptation this time because keeping it as you've written it is probably best for clarity. http://codereview.chromium.org/300013/diff/8003/8007#newcode154 Line 154: sort -u I do like how you've devised and included these pipelines. http://codereview.chromium.org/300013/diff/8003/8007#newcode164 Line 164: Removing extra blank lines is also a hobby of mine. :) http://codereview.chromium.org/300013/diff/8003/8008 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/8003/8008#newcode22 Line 22: # This condition is for migrating from a pre-build binaries to an in-tree "migrating from pre-built binaries to" http://codereview.chromium.org/300013/diff/8003/8008#newcode44 Line 44: # in their checkout. The Mac release-mode default is actually -O3. If ffmpeg can tolerate being built at -O3, we shouldn't force it to be lower. http://codereview.chromium.org/300013/diff/8003/8008#newcode52 Line 52: 'debug_optimize': '2', You might need to define these variables in this .gyp file in a top-level target_defaults in order for this to work? http://codereview.chromium.org/300013/diff/8003/8008#newcode59 Line 59: # TODO(ajwong): zlib and bzip2 are build w/o pic, which means we can't link Ah. We build them as PIC on the Mac (because it's the Mac default) so we won't have this problem there. But even so, linking an extra copy of our zlib and our bzip2 libraries into the ffmpegsumo library kind of sucks. Also, I think that we're pushing a custom logging function into our build of libbz2, and that logging function is defined in our base library, making it unsuitable for use without base. (Maybe we don't do that any more? If we do, and you find what I mean, you'll see that it's incredibly cheesey.) Let's leave this at TODO for now and just cope with using -lz -lbz2 on Linux for the time being. I'm sure you'll want to smooth over the rough edges a little bit still after this goes in, so we've got some time to figure things out. I assume that the Linux binaries you've got checked in now are linked against the system -lz -lbz2 as well, so doing this here is no worse than what we've already got? http://codereview.chromium.org/300013/diff/8003/8008#newcode170 Line 170: 'source/config/<(ffmpeg_branding)/<(OS)/<(target_arch)/config.h', Cool. http://codereview.chromium.org/300013/diff/8003/8008#newcode173 Line 173: 'source/config/<(ffmpeg_branding)/<(OS)/<(target_arch)', Again, cool. Nice and easy. http://codereview.chromium.org/300013/diff/8003/8008#newcode182 Line 182: '-fomit-frame-pointer', I love this optimization, but it interferes with Breakpad. If we ever crash with this library on the stack, the backtraces will be worthless. http://codereview.chromium.org/300013/diff/8003/8009 File third_party/ffmpeg/source/config/chrome/linux/ia32/config.h (right): http://codereview.chromium.org/300013/diff/8003/8009#newcode1 Line 1: /* Automatically generated by configure - do not modify! */ Don't forget svn:eol-style on new files. http://codereview.chromium.org/300013/diff/8003/8012 File third_party/yasm/source/config/linux/Makefile (right): http://codereview.chromium.org/300013/diff/8003/8012#newcode1 Line 1: # Makefile.in generated by automake 1.10.1 from Makefile.am. New files like this too. http://codereview.chromium.org/300013/diff/8003/8013 File third_party/yasm/source/config/linux/config.h (right): http://codereview.chromium.org/300013/diff/8003/8013#newcode1 Line 1: /* config.h. Generated from config.h.in by configure. */ There are no differences in the generated files for 32- and 64-bit, right? http://codereview.chromium.org/300013/diff/8003/8015 File third_party/yasm/source/patches/gen_x86_insn_outdir.patch (right): http://codereview.chromium.org/300013/diff/8003/8015#newcode1 Line 1: --- /tmp/i.py 2009-10-21 12:16:31.000000000 -0700 This works. http://codereview.chromium.org/300013/diff/8003/8017 File third_party/yasm/yasm.gyp (right): http://codereview.chromium.org/300013/diff/8003/8017#newcode20 Line 20: # 3) patch_sources -- Patches scripts and files to make them work better I'd avoid this step, because it generally makes things a little bit ludicrous. Instead, just check in the patched variants. Since you'll be checking in yasm anyway, this shouldn't impose an extra burden, and it will make the .gyp file less complicated. http://codereview.chromium.org/300013/diff/8003/8017#newcode32 Line 32: # large mount of repetative code. large amount of repetitive code. http://codereview.chromium.org/300013/diff/8003/8017#newcode150 Line 150: '<(shared_generated_dir)', Indent is silly. Next line too. http://codereview.chromium.org/300013/diff/8003/8017#newcode195 Line 195: 'infile': 'source/yasm/modules/parsers/nasm/nasm-std.mac', Variables corresponding to paths should end in _file, _files, _path, _paths, _dir, or _dirs GYP path reparenting support (which will probably not be used for this file but it might be in the future) depends on this. http://codereview.chromium.org/300013/diff/8003/8017#newcode226 Line 226: 'action_name': 'generate_win64_gas', I like this name. Not quite as much as ffmpegsumo, but still, "generate gas" sounds like a good time. http://codereview.chromium.org/300013/diff/8003/8017#newcode260 Line 260: }, One idea to reduce the duplication in all of these genmacro guys would be to put the body of the action into a .gypi file, and then in the action bodies here, you could just do: { 'variables': { 'varname': 'whatever', }, 'includes': [ 'genmacro.gypi', ], }, But maybe that wouldn't work. Might be worth trying. Or maybe you want to hold off. http://codereview.chromium.org/300013/diff/8003/8017#newcode335 Line 335: 'target_name': 'config_sources', I don't quite get what this target is supposed to do. http://codereview.chromium.org/300013/diff/8003/8017#newcode337 Line 337: 'hard_dependency': 1, I don't think you need to mark none-type dependencies as hard, they're always hard. hard should only have an effect on static_library targets. http://codereview.chromium.org/300013/diff/8003/8017#newcode345 Line 345: 'target_name': 'patch_sources', As above. http://codereview.chromium.org/300013/diff/8003/8017#newcode405 Line 405: 'inputs': [ '<(PRODUCT_DIR)/genperf' ], I didn't examine all of your dependency relationships terribly closely, but this one jumped out at me. Doesn't this mean that this target (generate_files) needs to depend on genperf? http://codereview.chromium.org/300013/diff/8003/8017#newcode477 Line 477: '-std=gnu99', Not yasm_c_flags? Happens in a few other 'cflags' that you have.
my eyes are bleeding :( http://codereview.chromium.org/300013/diff/8003/8004 File DEPS (right): http://codereview.chromium.org/300013/diff/8003/8004#newcode138 Line 138: "http://www.tortall.net/svn/yasm/trunk/yasm@2192", On 2009/10/22 02:49:35, Mark Mentovai wrote: > This would be a first. > > So far, all of the SVN severs we depend on fall into three categories: > > 1. our own server > 2. http://googlecode.com > 3. http://webkit.org > > I've historically been very reluctant to add any servers that don't fit into > these categories because it adds another remote server that our builds need to > depend on, and it potentially places a heavy load on that server when all of our > bots and developers pick this up. This kind of thing has actually gotten our > buildbots blacklisted by the third category in the past, when severe > misconfigurations caused everything to clobber and try pulling fresh WebKit > trees. > > For this one, webkit.org's right out, and since yasm isn't on http://googlecode.com, I > would advise checking in a snapshot of yasm into /trunk/deps. echoing this I once checked in a change that forced all the bots to clobber + re-checkout and webkit.org banned them :( http://codereview.chromium.org/300013/diff/8003/8007 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/8003/8007#newcode23 Line 23: POSIX systems, dlopen is used to achieve a similar effect. nit: dlopen -> dlopen() http://codereview.chromium.org/300013/diff/8003/8007#newcode38 Line 38: The FFmpeg build is relatively straight forward. All files are build with build -> built http://codereview.chromium.org/300013/diff/8003/8007#newcode101 Line 101: are the only files of interest? do you mean make sure you only copy those files? finding the statement a little unclear http://codereview.chromium.org/300013/diff/8003/8007#newcode120 Line 120: There should yasm commands for assembling two yasm files, but nothing There should yasm -> There should be yasm ? would it beneficial to have something like: grep -v '^gcc\|^yasm' should generate no output http://codereview.chromium.org/300013/diff/8003/8008 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/8003/8008#newcode41 Line 41: # Since we are not often debugging ffmpeg, and performance is ffmpeg -> FFmpeg http://codereview.chromium.org/300013/diff/8003/8008#newcode44 Line 44: # in their checkout. On 2009/10/22 02:49:35, Mark Mentovai wrote: > The Mac release-mode default is actually -O3. If ffmpeg can tolerate being > built at -O3, we shouldn't force it to be lower. fbarchard has some numbers (both size & performance) for building ffmpeg at differ -O levels we should dig it up and make a decision http://codereview.chromium.org/300013/diff/8003/8008#newcode165 Line 165: # The ffmpeg yasm files. nit: ffmpeg -> FFmpeg http://codereview.chromium.org/300013/diff/8003/8008#newcode182 Line 182: '-fomit-frame-pointer', On 2009/10/22 02:49:35, Mark Mentovai wrote: > I love this optimization, but it interferes with Breakpad. If we ever crash > with this library on the stack, the backtraces will be worthless. hrmm mark raises an interesting point we know ffmpeg isn't exactly a shining star for stability. we've already applied 40+ patches to fix known issues.. it might be handy to collect traces when linux is released in the wild sounds like a benchmark to me :) perhaps fbarchard has some numbers ready to go http://codereview.chromium.org/300013/diff/8003/8008#newcode212 Line 212: '-DPIC', just confirming.. we're always building these assuming we're on a 64-bit platform with PIC enabled? what about using yasm on ia32 builds of ffmpeg? http://codereview.chromium.org/300013/diff/8003/8011 File third_party/yasm/README.chromium (right): http://codereview.chromium.org/300013/diff/8003/8011#newcode15 Line 15: make -j5 yasm > yasm_build_log 2> yasm_build_err stealing mark's comment: re: -j might mess you up for build logs http://codereview.chromium.org/300013/diff/8003/8011#newcode17 Line 17: 4) Check /tmp/yasm_build_err to see if there are any anomalies beyond in previous step you're not outputting to /tmp http://codereview.chromium.org/300013/diff/8003/8011#newcode20 Line 20: 5) Grab the generated Makefile, libyasm-stdint.h, config.h, and put into stealing mark's comment: re: gyp doesn't like filenames with - in them (I believe) http://codereview.chromium.org/300013/diff/8003/8011#newcode31 Line 31: grep -E ^gcc yasm_build_log | stealing mark's comment: re: -E usage do you want ^gcc quoted? http://codereview.chromium.org/300013/diff/8003/8011#newcode36 Line 36: subprogram do not have -DHAVE_CONFIG_H as a cflag. "to subprograms the subprogram" ??? http://codereview.chromium.org/300013/diff/8003/8011#newcode53 Line 53: grep -E ^gcc yasm_build_log | stealing mark's comment: re: -E usage do you want ^gcc quoted?
I made a pass through and addressed most of your comments, minus unrolling a copy of yasm into the deps tree, and doing the ar/ranlib to build a .a. Also found another error in my ffmpeg linux build. I think I'm going to downgrade the optimization and build it PIC with ebp for the breakpad issues. Correctness first...cause without that, testing the whole thing gets a bit hard. http://codereview.chromium.org/300013/diff/8003/8004 File DEPS (right): http://codereview.chromium.org/300013/diff/8003/8004#newcode138 Line 138: "http://www.tortall.net/svn/yasm/trunk/yasm@2192", On 2009/10/22 02:49:35, Mark Mentovai wrote: > This would be a first. > > So far, all of the SVN severs we depend on fall into three categories: > > 1. our own server > 2. http://googlecode.com > 3. http://webkit.org > > I've historically been very reluctant to add any servers that don't fit into > these categories because it adds another remote server that our builds need to > depend on, and it potentially places a heavy load on that server when all of our > bots and developers pick this up. This kind of thing has actually gotten our > buildbots blacklisted by the third category in the past, when severe > misconfigurations caused everything to clobber and try pulling fresh WebKit > trees. > > For this one, webkit.org's right out, and since yasm isn't on http://googlecode.com, I > would advise checking in a snapshot of yasm into /trunk/deps. Sounds good. I'll send another CL for this tomorrow when I'm on a real machine (Rather than a laptop). http://codereview.chromium.org/300013/diff/8003/8006 File media/base/media_posix.cc (right): http://codereview.chromium.org/300013/diff/8003/8006#newcode32 Line 32: #if defined(OS_POSIX) On 2009/10/22 02:49:35, Mark Mentovai wrote: > Mac is OS_POSIX too. You want OS_LINUX. Erp...silly me. Fixed it different. I just remembered I coded this thing up to take multiple library names in order of preference. Best of both worlds! http://codereview.chromium.org/300013/diff/8003/8007 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/8003/8007#newcode21 Line 21: libraries to permit dynamically loading FFmpeg. On windows, the libraries are On 2009/10/22 02:49:35, Mark Mentovai wrote: > windows is Windows. (tm) Done. http://codereview.chromium.org/300013/diff/8003/8007#newcode38 Line 38: The FFmpeg build is relatively straight forward. All files are build with On 2009/10/22 02:49:35, Mark Mentovai wrote: > straightforward Done. http://codereview.chromium.org/300013/diff/8003/8007#newcode48 Line 48: this. An error like: On 2009/10/22 02:49:35, Mark Mentovai wrote: > "An error like: [error]" EOF? Finish your thought? reworded. http://codereview.chromium.org/300013/diff/8003/8007#newcode53 Line 53: 2) On ia32, FFmpeg cannot be built with -fPIC, again due to assembly On 2009/10/22 02:49:35, Mark Mentovai wrote: > I'll be curious to see how this plays out on the Mac where -fPIC is the default. We get slower code. :) There's a config.h option that says ebp is not available. I'm actually a bit annoyed to see a shared library built w/o -fPIC. It means that we increase the total amount of actual memory used per renderer by like 1.5 megs. :-/ I could build it -fPIC, -fno-omit-frame-pointer. I don't know how much of a perf hit that really will be. On the otherhand, I don't know if it matters that much for linux desktops since our slowest code is not here. http://codereview.chromium.org/300013/diff/8003/8007#newcode64 Line 64: cross-compiling for 32-bit on a 64-bit machine. Since yasm is build in On 2009/10/22 02:49:35, Mark Mentovai wrote: > built Fixed. *sigh*...I ran it through aspell -c...but apparently I need a grammar checker too. http://codereview.chromium.org/300013/diff/8003/8007#newcode72 Line 72: Detail Directions: On 2009/10/22 02:49:35, Mark Mentovai wrote: > Detailed Done. http://codereview.chromium.org/300013/diff/8003/8007#newcode99 Line 99: Make to double-check that config.h and version.h are the only files of On 2009/10/22 02:49:35, Mark Mentovai wrote: > sure? > Make ^ to double-check Done. http://codereview.chromium.org/300013/diff/8003/8007#newcode108 Line 108: make -j5 libavcodec/libavcodec.so libavformat/libavformat.so \ On 2009/10/22 02:49:35, Mark Mentovai wrote: > To use a build log as a reference, -j might work against you. True. Removed. Though, in this case, since all the post-processing commands completely disregard order, it's probably okay. http://codereview.chromium.org/300013/diff/8003/8007#newcode112 Line 112: anomalies. FFmpeg source generates a lot of compiler warnings; it On 2009/10/22 02:49:35, Mark Mentovai wrote: > sigh. It wouldn't if I were on that project, I can tell you that much. :) Don't get me started. :) http://codereview.chromium.org/300013/diff/8003/8007#newcode118 Line 118: grep -Ev '^gcc' ffmpeg_build_log On 2009/10/22 02:49:35, Mark Mentovai wrote: > There's nothing extended about this regular expression, you can just say "grep > -v". Oh! I thought anchors were part of the extended syntax. Good to know. Removed. http://codereview.chromium.org/300013/diff/8003/8007#newcode126 Line 126: grep -E '^gcc' ffmpeg_build_log | On 2009/10/22 02:49:35, Mark Mentovai wrote: > no -E Done. http://codereview.chromium.org/300013/diff/8003/8007#newcode130 Line 130: sort | uniq -c On 2009/10/22 02:49:35, Mark Mentovai wrote: > One of my side hobbies is improving shell pipelines, but I'll resist the > temptation this time because keeping it as you've written it is probably best > for clarity. he he he. I did consider combining all the greps? Or making one 1 big sed script and pipe to sort -u? Just curious. Ended up keeping like it is the for the clarity reason you stated. http://codereview.chromium.org/300013/diff/8003/8007#newcode164 Line 164: On 2009/10/22 02:49:35, Mark Mentovai wrote: > Removing extra blank lines is also a hobby of mine. :) Gha! Gone. Ya know...there's something about working on chromium that make me lose my style sense. It's embarrassing. http://codereview.chromium.org/300013/diff/8003/8008 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/8003/8008#newcode22 Line 22: # This condition is for migrating from a pre-build binaries to an in-tree On 2009/10/22 02:49:35, Mark Mentovai wrote: > "migrating from pre-built binaries to" Done. http://codereview.chromium.org/300013/diff/8003/8008#newcode44 Line 44: # in their checkout. On 2009/10/22 02:49:35, Mark Mentovai wrote: > The Mac release-mode default is actually -O3. If ffmpeg can tolerate being > built at -O3, we shouldn't force it to be lower. We actually benchmarked this on windows (using gcc-sljl) and -O3 produces noticeably slower + bigger binaries than -O2. fbarchard probably still has the numbers. Granted, mac gcc might behave differently, but I'd expect it to be more similar than different. I would be wary of going to -O3 without evidence showing it doesn't slow things down. http://codereview.chromium.org/300013/diff/8003/8008#newcode52 Line 52: 'debug_optimize': '2', On 2009/10/22 02:49:35, Mark Mentovai wrote: > You might need to define these variables in this .gyp file in a top-level > target_defaults in order for this to work? I tried...didn't work. Moved it up there again so you can see my attempt. http://codereview.chromium.org/300013/diff/8003/8008#newcode59 Line 59: # TODO(ajwong): zlib and bzip2 are build w/o pic, which means we can't link On 2009/10/22 02:49:35, Mark Mentovai wrote: > Ah. We build them as PIC on the Mac (because it's the Mac default) so we won't > have this problem there. > > But even so, linking an extra copy of our zlib and our bzip2 libraries into the > ffmpegsumo library kind of sucks. > > Also, I think that we're pushing a custom logging function into our build of > libbz2, and that logging function is defined in our base library, making it > unsuitable for use without base. (Maybe we don't do that any more? If we do, > and you find what I mean, you'll see that it's incredibly cheesey.) > > Let's leave this at TODO for now and just cope with using -lz -lbz2 on Linux for > the time being. I'm sure you'll want to smooth over the rough edges a little > bit still after this goes in, so we've got some time to figure things out. > > I assume that the Linux binaries you've got checked in now are linked against > the system -lz -lbz2 as well, so doing this here is no worse than what we've > already got? Yep, no worse. I'm fine leaving as it. I'm not even sure what ffmpeg does with bzip2...but whatever. It thinks it needs it. http://codereview.chromium.org/300013/diff/8003/8008#newcode182 Line 182: '-fomit-frame-pointer', On 2009/10/22 02:49:35, Mark Mentovai wrote: > I love this optimization, but it interferes with Breakpad. If we ever crash > with this library on the stack, the backtraces will be worthless. No worse than the current situation. :) I'll discuss the pic & frame-pointer tradeoff with the team here. For now, would rather leave as is. Added a TODO. http://codereview.chromium.org/300013/diff/8003/8009 File third_party/ffmpeg/source/config/chrome/linux/ia32/config.h (right): http://codereview.chromium.org/300013/diff/8003/8009#newcode1 Line 1: /* Automatically generated by configure - do not modify! */ On 2009/10/22 02:49:35, Mark Mentovai wrote: > Don't forget svn:eol-style on new files. Using git. I'll do the svn eol-style, executable, ignore stuff in another pass. http://codereview.chromium.org/300013/diff/8003/8012 File third_party/yasm/source/config/linux/Makefile (right): http://codereview.chromium.org/300013/diff/8003/8012#newcode1 Line 1: # Makefile.in generated by automake 1.10.1 from Makefile.am. On 2009/10/22 02:49:35, Mark Mentovai wrote: > New files like this too. punt! http://codereview.chromium.org/300013/diff/8003/8013 File third_party/yasm/source/config/linux/config.h (right): http://codereview.chromium.org/300013/diff/8003/8013#newcode1 Line 1: /* config.h. Generated from config.h.in by configure. */ On 2009/10/22 02:49:35, Mark Mentovai wrote: > There are no differences in the generated files for 32- and 64-bit, right? ...I didn't actually verify. However, looking through the file, it all seems sane to me for both 32 and 64-bit. I haven't actually gotten the 32-bit ffmpeg build working quite yet, but that should be a smoke test for if this file is broken. http://codereview.chromium.org/300013/diff/8003/8017 File third_party/yasm/yasm.gyp (right): http://codereview.chromium.org/300013/diff/8003/8017#newcode20 Line 20: # 3) patch_sources -- Patches scripts and files to make them work better On 2009/10/22 02:49:35, Mark Mentovai wrote: > I'd avoid this step, because it generally makes things a little bit ludicrous. > Instead, just check in the patched variants. > > Since you'll be checking in yasm anyway, this shouldn't impose an extra burden, > and it will make the .gyp file less complicated. Yeah, I did this initially because I was pulling pristine source. But, if I'm going to check it in, I'll gladly kill this step. http://codereview.chromium.org/300013/diff/8003/8017#newcode32 Line 32: # large mount of repetative code. On 2009/10/22 02:49:35, Mark Mentovai wrote: > large amount of repetitive code. Done. http://codereview.chromium.org/300013/diff/8003/8017#newcode150 Line 150: '<(shared_generated_dir)', On 2009/10/22 02:49:35, Mark Mentovai wrote: > Indent is silly. Next line too. Done. http://codereview.chromium.org/300013/diff/8003/8017#newcode226 Line 226: 'action_name': 'generate_win64_gas', On 2009/10/22 02:49:35, Mark Mentovai wrote: > I like this name. Not quite as much as ffmpegsumo, but still, "generate gas" > sounds like a good time. Now we need a target named beano. http://codereview.chromium.org/300013/diff/8003/8017#newcode260 Line 260: }, On 2009/10/22 02:49:35, Mark Mentovai wrote: > One idea to reduce the duplication in all of these genmacro guys would be to put > the body of the action into a .gypi file, and then in the action bodies here, > you could just do: > > { > 'variables': { > 'varname': 'whatever', > }, > 'includes': [ > 'genmacro.gypi', > ], > }, > > But maybe that wouldn't work. Might be worth trying. Or maybe you want to hold > off. Oh that would be lovely. I'm getting pretty sick of messing with this though. http://codereview.chromium.org/300013/diff/8003/8017#newcode335 Line 335: 'target_name': 'config_sources', On 2009/10/22 02:49:35, Mark Mentovai wrote: > I don't quite get what this target is supposed to do. Shared dependency on these source files so that when they change, the dependent targets get rebuilt? Since a bunch of things depend on these, and I don't think I can safely refer the same files from the sources array in multiple targets (I remember getting some circular reference warnings), I did it like this. Am I misunderstanding how targets work? I figured that if a target has a source listed, and the source changed, the target would be marked dirt and all dependent targets would be forced to be rebuilt. http://codereview.chromium.org/300013/diff/8003/8017#newcode337 Line 337: 'hard_dependency': 1, On 2009/10/22 02:49:35, Mark Mentovai wrote: > I don't think you need to mark none-type dependencies as hard, they're always > hard. hard should only have an effect on static_library targets. Removed them. http://codereview.chromium.org/300013/diff/8003/8017#newcode345 Line 345: 'target_name': 'patch_sources', On 2009/10/22 02:49:35, Mark Mentovai wrote: > As above. Will fix tomorrow when I get to a real machine. http://codereview.chromium.org/300013/diff/8003/8017#newcode405 Line 405: 'inputs': [ '<(PRODUCT_DIR)/genperf' ], On 2009/10/22 02:49:35, Mark Mentovai wrote: > I didn't examine all of your dependency relationships terribly closely, but this > one jumped out at me. Doesn't this mean that this target (generate_files) needs > to depend on genperf? Good catch! Though, I don't think I completely understand the ordering implications of having something in the inputs list of an action, version the dependencies list of the containing target. http://codereview.chromium.org/300013/diff/8003/8017#newcode477 Line 477: '-std=gnu99', On 2009/10/22 02:49:35, Mark Mentovai wrote: > Not yasm_c_flags? Happens in a few other 'cflags' that you have. nope. I guess I should add a note about that. The yasm_c_flags would probably safe, but the actual yasm build uses a very minimal set of flags for building these guys.
This makes everythign work *if* I can make it build > -O0. However, I can't make that work. Any ideas? http://codereview.chromium.org/300013/diff/8003/8007 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/8003/8007#newcode23 Line 23: POSIX systems, dlopen is used to achieve a similar effect. On 2009/10/22 06:27:49, scherkus wrote: > nit: dlopen -> dlopen() Done. http://codereview.chromium.org/300013/diff/8003/8007#newcode38 Line 38: The FFmpeg build is relatively straight forward. All files are build with On 2009/10/22 06:27:49, scherkus wrote: > build -> built Done. http://codereview.chromium.org/300013/diff/8003/8007#newcode101 Line 101: On 2009/10/22 06:27:49, scherkus wrote: > are the only files of interest? do you mean make sure you only copy those > files? finding the statement a little unclear Done. http://codereview.chromium.org/300013/diff/8003/8007#newcode120 Line 120: There should yasm commands for assembling two yasm files, but nothing On 2009/10/22 06:27:49, scherkus wrote: > There should yasm -> There should be yasm ? > > would it beneficial to have something like: > grep -v '^gcc\|^yasm' should generate no output Done. http://codereview.chromium.org/300013/diff/8003/8008 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/8003/8008#newcode41 Line 41: # Since we are not often debugging ffmpeg, and performance is On 2009/10/22 06:27:49, scherkus wrote: > ffmpeg -> FFmpeg Done. http://codereview.chromium.org/300013/diff/8003/8008#newcode165 Line 165: # The ffmpeg yasm files. On 2009/10/22 06:27:49, scherkus wrote: > nit: ffmpeg -> FFmpeg Done. http://codereview.chromium.org/300013/diff/8003/8008#newcode212 Line 212: '-DPIC', On 2009/10/22 06:27:49, scherkus wrote: > just confirming.. we're always building these assuming we're on a 64-bit > platform with PIC enabled? > > what about using yasm on ia32 builds of ffmpeg? It's a mistake. :( Added TODO to fix. http://codereview.chromium.org/300013/diff/8003/8011 File third_party/yasm/README.chromium (right): http://codereview.chromium.org/300013/diff/8003/8011#newcode15 Line 15: make -j5 yasm > yasm_build_log 2> yasm_build_err On 2009/10/22 06:27:49, scherkus wrote: > stealing mark's comment: re: -j might mess you up for build logs Done. http://codereview.chromium.org/300013/diff/8003/8011#newcode17 Line 17: 4) Check /tmp/yasm_build_err to see if there are any anomalies beyond On 2009/10/22 06:27:49, scherkus wrote: > in previous step you're not outputting to /tmp Done. http://codereview.chromium.org/300013/diff/8003/8011#newcode20 Line 20: 5) Grab the generated Makefile, libyasm-stdint.h, config.h, and put into On 2009/10/22 06:27:49, scherkus wrote: > stealing mark's comment: re: gyp doesn't like filenames with - in them (I > believe) I don't remember mark saying that....it would be significnatly annoying to change that file name. Seems to work as is. http://codereview.chromium.org/300013/diff/8003/8011#newcode31 Line 31: grep -E ^gcc yasm_build_log | On 2009/10/22 06:27:49, scherkus wrote: > stealing mark's comment: re: -E usage > > do you want ^gcc quoted? Doesn't matter, but added anyways. http://codereview.chromium.org/300013/diff/8003/8011#newcode36 Line 36: subprogram do not have -DHAVE_CONFIG_H as a cflag. On 2009/10/22 06:27:49, scherkus wrote: > "to subprograms the subprogram" ??? Done. http://codereview.chromium.org/300013/diff/8003/8011#newcode53 Line 53: grep -E ^gcc yasm_build_log | On 2009/10/22 06:27:49, scherkus wrote: > stealing mark's comment: re: -E usage > > do you want ^gcc quoted? Done.
This build now works correctly for linux Make and scons. The last hiccup is the finished libffmpegsumo.so does not show up in the <(PRODUCT_DIR). Should I just add a copy step somewhere? Or there a special flag I can set to make it show up? On 2009/10/23 23:53:40, awong wrote: > This makes everythign work *if* I can make it build > -O0. However, I can't > make that work. > > Any ideas? > > http://codereview.chromium.org/300013/diff/8003/8007 > File third_party/ffmpeg/README.chromium (right): > > http://codereview.chromium.org/300013/diff/8003/8007#newcode23 > Line 23: POSIX systems, dlopen is used to achieve a similar effect. > On 2009/10/22 06:27:49, scherkus wrote: > > nit: dlopen -> dlopen() > > Done. > > http://codereview.chromium.org/300013/diff/8003/8007#newcode38 > Line 38: The FFmpeg build is relatively straight forward. All files are build > with > On 2009/10/22 06:27:49, scherkus wrote: > > build -> built > > Done. > > http://codereview.chromium.org/300013/diff/8003/8007#newcode101 > Line 101: > On 2009/10/22 06:27:49, scherkus wrote: > > are the only files of interest? do you mean make sure you only copy those > > files? finding the statement a little unclear > > Done. > > http://codereview.chromium.org/300013/diff/8003/8007#newcode120 > Line 120: There should yasm commands for assembling two yasm files, but nothing > On 2009/10/22 06:27:49, scherkus wrote: > > There should yasm -> There should be yasm ? > > > > would it beneficial to have something like: > > grep -v '^gcc\|^yasm' should generate no output > > Done. > > http://codereview.chromium.org/300013/diff/8003/8008 > File third_party/ffmpeg/ffmpeg.gyp (right): > > http://codereview.chromium.org/300013/diff/8003/8008#newcode41 > Line 41: # Since we are not often debugging ffmpeg, and performance is > On 2009/10/22 06:27:49, scherkus wrote: > > ffmpeg -> FFmpeg > > Done. > > http://codereview.chromium.org/300013/diff/8003/8008#newcode165 > Line 165: # The ffmpeg yasm files. > On 2009/10/22 06:27:49, scherkus wrote: > > nit: ffmpeg -> FFmpeg > > Done. > > http://codereview.chromium.org/300013/diff/8003/8008#newcode212 > Line 212: '-DPIC', > On 2009/10/22 06:27:49, scherkus wrote: > > just confirming.. we're always building these assuming we're on a 64-bit > > platform with PIC enabled? > > > > what about using yasm on ia32 builds of ffmpeg? > > It's a mistake. :( Added TODO to fix. > > http://codereview.chromium.org/300013/diff/8003/8011 > File third_party/yasm/README.chromium (right): > > http://codereview.chromium.org/300013/diff/8003/8011#newcode15 > Line 15: make -j5 yasm > yasm_build_log 2> yasm_build_err > On 2009/10/22 06:27:49, scherkus wrote: > > stealing mark's comment: re: -j might mess you up for build logs > > Done. > > http://codereview.chromium.org/300013/diff/8003/8011#newcode17 > Line 17: 4) Check /tmp/yasm_build_err to see if there are any anomalies beyond > On 2009/10/22 06:27:49, scherkus wrote: > > in previous step you're not outputting to /tmp > > Done. > > http://codereview.chromium.org/300013/diff/8003/8011#newcode20 > Line 20: 5) Grab the generated Makefile, libyasm-stdint.h, config.h, and put > into > On 2009/10/22 06:27:49, scherkus wrote: > > stealing mark's comment: re: gyp doesn't like filenames with - in them (I > > believe) > I don't remember mark saying that....it would be significnatly annoying to > change that file name. Seems to work as is. > > http://codereview.chromium.org/300013/diff/8003/8011#newcode31 > Line 31: grep -E ^gcc yasm_build_log | > On 2009/10/22 06:27:49, scherkus wrote: > > stealing mark's comment: re: -E usage > > > > do you want ^gcc quoted? > > Doesn't matter, but added anyways. > > http://codereview.chromium.org/300013/diff/8003/8011#newcode36 > Line 36: subprogram do not have -DHAVE_CONFIG_H as a cflag. > On 2009/10/22 06:27:49, scherkus wrote: > > "to subprograms the subprogram" ??? > > Done. > > http://codereview.chromium.org/300013/diff/8003/8011#newcode53 > Line 53: grep -E ^gcc yasm_build_log | > On 2009/10/22 06:27:49, scherkus wrote: > > stealing mark's comment: re: -E usage > > > > do you want ^gcc quoted? > > Done.
I'm not sure why it wouldn't show up in <(PRODUCT_DIR), unless there's something specific to the make generator that's preventing it. http://codereview.chromium.org/300013/diff/16001/17006 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/16001/17006#newcode52 Line 52: 'target_name': 'libffmpegsumo', Should be 'ffmpegsumo' or the output will be 'liblibffmpegsumo.so'
Okay! I think this is finally ready! For r3@Lz!!!! Mark, I think the most useful incremental diff for you would be either agianst patch set 10, or patch set 7. Michael, I've included you because I touched installer.gyp. Basically, for linux, we're switching from distributing 3 DLLs to one sumo one. Main changes this time around: 1) overriding of debug_optimization is in the right spot. 2) fixes to installer.gyp and chrome.gyp 3) Update to neuter the copy of ffmpeg_binaries for linux. Andrew, if you want to sanity check the media_posix change (and anything else), feel free. Thanks everyone for all the help! This is pretty darned exciting (for me at least). Hopefully after this is done, mac will be comparatively simple. -Albert
Oh, and I tests a release, branded build locally using make. Seems to work. Scons is running, but scons worked for debug, so I don't expect problems in release. On 2009/10/28 02:11:35, awong wrote: > Okay! I think this is finally ready! For r3@Lz!!!! > > Mark, I think the most useful incremental diff for you would be either agianst > patch set 10, or patch set 7. > > Michael, I've included you because I touched installer.gyp. Basically, for > linux, we're switching from distributing 3 DLLs to one sumo one. > > Main changes this time around: > 1) overriding of debug_optimization is in the right spot. > 2) fixes to installer.gyp and chrome.gyp > 3) Update to neuter the copy of ffmpeg_binaries for linux. > > Andrew, if you want to sanity check the media_posix change (and anything else), > feel free. > > Thanks everyone for all the help! This is pretty darned exciting (for me at > least). Hopefully after this is done, mac will be comparatively simple. > > -Albert
The branded build on scons actually did break. Go figure. For some reasons, scons decides to add an -fPIC into all C++ shared library targets. It doesn't do the same for C targets. This is scons's mainline behavior, not just scons via gyp. I've created a gyp CL to make the scons gyp generator override the -fPIC appending on the SHCCFLAGS in scons which fixes things. Assuming that CL is approved, committed, and rolled, this CL works. -Albert On 2009/10/28 02:12:22, awong wrote: > Oh, and I tests a release, branded build locally using make. Seems to work. > Scons is running, but scons worked for debug, so I don't expect problems in > release. > > On 2009/10/28 02:11:35, awong wrote: > > Okay! I think this is finally ready! For r3@Lz!!!! > > > > Mark, I think the most useful incremental diff for you would be either agianst > > patch set 10, or patch set 7. > > > > Michael, I've included you because I touched installer.gyp. Basically, for > > linux, we're switching from distributing 3 DLLs to one sumo one. > > > > Main changes this time around: > > 1) overriding of debug_optimization is in the right spot. > > 2) fixes to installer.gyp and chrome.gyp > > 3) Update to neuter the copy of ffmpeg_binaries for linux. > > > > Andrew, if you want to sanity check the media_posix change (and anything > else), > > feel free. > > > > Thanks everyone for all the help! This is pretty darned exciting (for me at > > least). Hopefully after this is done, mac will be comparatively simple. > > > > -Albert
I'm pretty sure it's ready for another pass now. Changes since last time: 1) Added hack to work around scons shadowing libz and libz2 with the static versions 2) Remove --as-needed flag cause, we don't need it, can it broke the scons hack. 3) Removed -fomit-frame-pointer (yay! breakpad!) 4) Added a dummy executable to break the direct dependency from chrome, etc., on libffmpegsumo.so. 5) Hack to work around evan's "-fvisibility=hidden" change in common.gypi. Real fix should probably be to make his change conditionally on _type=shared_library. Related to this was a gyp change to the scons generator to override scons's brain-dead inclusion of -fPIC in the world. Did I mention, I hate scons? For tests, I've successfully built locally in the following configurations: 1) Google Chrome, 32-bit, scons, Release 2) Google Chrome, 32-bit, scons, Debug 3) Google Chrome, 32-bit, make, Release 4) Google Chrome, 32-bit, make, Debug 5) Chromium, 64-bit, scons, Debug 6) Chromium, 64-bit, scons, Release 7) Chromium, 64-bit, make, Release 8) Chromium, 64-bit, make, Release Trying the other 8 configurations (64-bit Google Chrome, and 32-bit Chromium now). For each config, verified: a) Chrome/Chromium started w/o LD_LIBRARY_PATH b) Chrome/Chromium could play the expected videos. c) Chrome/Chromium loaded libffmpegsumo and not avcodec (grep sumo /proc/???/maps, etc.) d) Chrome/Chromium could run with libffmpegsumo.so deleted. This should hopefully be correct now. Other than possibly grammar/spelling errors, I'm not sure what other Is are left to dot. -Albert On 2009/10/28 21:01:51, awong wrote: > The branded build on scons actually did break. Go figure. > > For some reasons, scons decides to add an -fPIC into all C++ shared library > targets. It doesn't do the same for C targets. This is scons's mainline > behavior, not just scons via gyp. > > I've created a gyp CL to make the scons gyp generator override the -fPIC > appending on the SHCCFLAGS in scons which fixes things. > > Assuming that CL is approved, committed, and rolled, this CL works. > > -Albert > > > On 2009/10/28 02:12:22, awong wrote: > > Oh, and I tests a release, branded build locally using make. Seems to work. > > Scons is running, but scons worked for debug, so I don't expect problems in > > release. > > > > On 2009/10/28 02:11:35, awong wrote: > > > Okay! I think this is finally ready! For r3@Lz!!!! > > > > > > Mark, I think the most useful incremental diff for you would be either > agianst > > > patch set 10, or patch set 7. > > > > > > Michael, I've included you because I touched installer.gyp. Basically, for > > > linux, we're switching from distributing 3 DLLs to one sumo one. > > > > > > Main changes this time around: > > > 1) overriding of debug_optimization is in the right spot. > > > 2) fixes to installer.gyp and chrome.gyp > > > 3) Update to neuter the copy of ffmpeg_binaries for linux. > > > > > > Andrew, if you want to sanity check the media_posix change (and anything > > else), > > > feel free. > > > > > > Thanks everyone for all the help! This is pretty darned exciting (for me at > > > least). Hopefully after this is done, mac will be comparatively simple. > > > > > > -Albert
And wow. I jinxed myself. try servers failed. On 2009/10/29 21:33:10, awong wrote: > I'm pretty sure it's ready for another pass now. > > Changes since last time: > 1) Added hack to work around scons shadowing libz and libz2 with the static > versions > 2) Remove --as-needed flag cause, we don't need it, can it broke the scons > hack. > 3) Removed -fomit-frame-pointer (yay! breakpad!) > 4) Added a dummy executable to break the direct dependency from chrome, etc., > on libffmpegsumo.so. > 5) Hack to work around evan's "-fvisibility=hidden" change in common.gypi. > Real fix should probably be to make his change conditionally on > _type=shared_library. > > Related to this was a gyp change to the scons generator to override scons's > brain-dead inclusion of -fPIC in the world. > > Did I mention, I hate scons? > > For tests, I've successfully built locally in the following configurations: > 1) Google Chrome, 32-bit, scons, Release > 2) Google Chrome, 32-bit, scons, Debug > 3) Google Chrome, 32-bit, make, Release > 4) Google Chrome, 32-bit, make, Debug > 5) Chromium, 64-bit, scons, Debug > 6) Chromium, 64-bit, scons, Release > 7) Chromium, 64-bit, make, Release > 8) Chromium, 64-bit, make, Release > > Trying the other 8 configurations (64-bit Google Chrome, and 32-bit Chromium > now). > > For each config, verified: > a) Chrome/Chromium started w/o LD_LIBRARY_PATH > b) Chrome/Chromium could play the expected videos. > c) Chrome/Chromium loaded libffmpegsumo and not avcodec (grep sumo > /proc/???/maps, etc.) > d) Chrome/Chromium could run with libffmpegsumo.so deleted. > > This should hopefully be correct now. Other than possibly grammar/spelling > errors, I'm not sure what other Is are left to dot. > > -Albert > > On 2009/10/28 21:01:51, awong wrote: > > The branded build on scons actually did break. Go figure. > > > > For some reasons, scons decides to add an -fPIC into all C++ shared library > > targets. It doesn't do the same for C targets. This is scons's mainline > > behavior, not just scons via gyp. > > > > I've created a gyp CL to make the scons gyp generator override the -fPIC > > appending on the SHCCFLAGS in scons which fixes things. > > > > Assuming that CL is approved, committed, and rolled, this CL works. > > > > -Albert > > > > > > On 2009/10/28 02:12:22, awong wrote: > > > Oh, and I tests a release, branded build locally using make. Seems to work. > > > > Scons is running, but scons worked for debug, so I don't expect problems in > > > release. > > > > > > On 2009/10/28 02:11:35, awong wrote: > > > > Okay! I think this is finally ready! For r3@Lz!!!! > > > > > > > > Mark, I think the most useful incremental diff for you would be either > > agianst > > > > patch set 10, or patch set 7. > > > > > > > > Michael, I've included you because I touched installer.gyp. Basically, for > > > > linux, we're switching from distributing 3 DLLs to one sumo one. > > > > > > > > Main changes this time around: > > > > 1) overriding of debug_optimization is in the right spot. > > > > 2) fixes to installer.gyp and chrome.gyp > > > > 3) Update to neuter the copy of ffmpeg_binaries for linux. > > > > > > > > Andrew, if you want to sanity check the media_posix change (and anything > > > else), > > > > feel free. > > > > > > > > Thanks everyone for all the help! This is pretty darned exciting (for me > at > > > > least). Hopefully after this is done, mac will be comparatively simple. > > > > > > > > -Albert
I'm optimistic. If you can figure out the try server problem, I hope to have time to do another pass tonight. I'll have my red pen out as usual, but I think that if we've got something demonstrably working, we should check it in and iterate. Thanks again for your work on this. Mark ajwong@chromium.org wrote: > And wow. I jinxed myself. =A0try servers failed. > > On 2009/10/29 21:33:10, awong wrote: >> >> I'm pretty sure it's ready for another pass now. > >> Changes since last time: >> =A0 1) Added hack to work around scons shadowing libz and libz2 with the >> static >> versions >> =A0 2) Remove --as-needed flag cause, we don't need it, can it broke the >> scons >> hack. >> =A0 3) Removed -fomit-frame-pointer (yay! breakpad!) >> =A0 4) Added a dummy executable to break the direct dependency from chro= me, > > etc., >> >> on libffmpegsumo.so. >> =A0 5) Hack to work around evan's "-fvisibility=3Dhidden" change in >> common.gypi. >> Real fix should probably be to make his change conditionally on >> _type=3Dshared_library. > >> Related to this was a gyp change to the scons generator to override >> scons's >> brain-dead inclusion of -fPIC in the world. > >> Did I mention, I hate scons? > >> For tests, I've successfully built locally in the following >> configurations: >> =A0 1) Google Chrome, 32-bit, scons, Release >> =A0 2) Google Chrome, 32-bit, scons, Debug >> =A0 3) Google Chrome, 32-bit, make, Release >> =A0 4) Google Chrome, 32-bit, make, Debug >> =A0 5) Chromium, 64-bit, scons, Debug >> =A0 6) Chromium, 64-bit, scons, Release >> =A0 7) Chromium, 64-bit, make, Release >> =A0 8) Chromium, 64-bit, make, Release > >> Trying the other 8 configurations (64-bit Google Chrome, and 32-bit >> Chromium >> now). > >> For each config, verified: >> =A0a) Chrome/Chromium started w/o LD_LIBRARY_PATH >> =A0b) Chrome/Chromium could play the expected videos. >> =A0c) Chrome/Chromium loaded libffmpegsumo and not avcodec (grep sumo >> /proc/???/maps, etc.) >> =A0d) Chrome/Chromium could run with libffmpegsumo.so deleted. > >> This should hopefully be correct now. Other than possibly grammar/spelli= ng >> errors, I'm not sure what other Is are left to dot. > >> -Albert > >> On 2009/10/28 21:01:51, awong wrote: >> > The branded build on scons actually did break. =A0Go figure. >> > >> > For some reasons, scons decides to add an -fPIC into all C++ shared >> > library >> > targets. =A0It doesn't do the same for C targets. This is scons's main= line >> > behavior, not just scons via gyp. >> > >> > I've created a gyp CL to make the scons gyp generator override the -fP= IC >> > appending on the SHCCFLAGS in scons which fixes things. >> > >> > Assuming that CL is approved, committed, and rolled, this CL works. >> > >> > -Albert >> > >> > >> > On 2009/10/28 02:12:22, awong wrote: >> > > Oh, and I tests a release, branded build locally using make. =A0Seem= s to > > work. > >> > > Scons is running, but scons worked for debug, so I don't expect >> > > problems > > in >> >> > > release. >> > > >> > > On 2009/10/28 02:11:35, awong wrote: >> > > > Okay! =A0I think this is finally ready! =A0For r3@Lz!!!! >> > > > >> > > > Mark, I think the most useful incremental diff for you would be >> > > > either >> > agianst >> > > > patch set 10, or patch set 7. >> > > > >> > > > Michael, I've included you because I touched installer.gyp. >> > > > Basically, > > for >> >> > > > linux, we're switching from distributing 3 DLLs to one sumo one. >> > > > >> > > > Main changes this time around: >> > > > =A0 1) overriding of debug_optimization is in the right spot. >> > > > =A0 2) fixes to installer.gyp and chrome.gyp >> > > > =A0 3) Update to neuter the copy of ffmpeg_binaries for linux. >> > > > >> > > > Andrew, if you want to sanity check the media_posix change (and >> > > > anything >> > > else), >> > > > feel free. >> > > > >> > > > Thanks everyone for all the help! This is pretty darned exciting >> > > > (for me >> at >> > > > least). =A0Hopefully after this is done, mac will be comparatively >> > > > simple. >> > > > >> > > > -Albert > > > > http://codereview.chromium.org/300013 >
Ahah! The try servers haven't picked up the rolled gyp deps! That means scons is building with -fPIC and breaking. I can hack DEPS to give it a sensible try run. Unfortunately, piman's cross-compilation change (which I rolled in) broke me. -Albert On Thu, Oct 29, 2009 at 3:06 PM, Mark Mentovai <mark@chromium.org> wrote: > I'm optimistic. If you can figure out the try server problem, I hope > to have time to do another pass tonight. I'll have my red pen out as > usual, but I think that if we've got something demonstrably working, > we should check it in and iterate. Thanks again for your work on > this. > > Mark > > ajwong@chromium.org wrote: > > And wow. I jinxed myself. try servers failed. > > > > On 2009/10/29 21:33:10, awong wrote: > >> > >> I'm pretty sure it's ready for another pass now. > > > >> Changes since last time: > >> 1) Added hack to work around scons shadowing libz and libz2 with the > >> static > >> versions > >> 2) Remove --as-needed flag cause, we don't need it, can it broke the > >> scons > >> hack. > >> 3) Removed -fomit-frame-pointer (yay! breakpad!) > >> 4) Added a dummy executable to break the direct dependency from > chrome, > > > > etc., > >> > >> on libffmpegsumo.so. > >> 5) Hack to work around evan's "-fvisibility=hidden" change in > >> common.gypi. > >> Real fix should probably be to make his change conditionally on > >> _type=shared_library. > > > >> Related to this was a gyp change to the scons generator to override > >> scons's > >> brain-dead inclusion of -fPIC in the world. > > > >> Did I mention, I hate scons? > > > >> For tests, I've successfully built locally in the following > >> configurations: > >> 1) Google Chrome, 32-bit, scons, Release > >> 2) Google Chrome, 32-bit, scons, Debug > >> 3) Google Chrome, 32-bit, make, Release > >> 4) Google Chrome, 32-bit, make, Debug > >> 5) Chromium, 64-bit, scons, Debug > >> 6) Chromium, 64-bit, scons, Release > >> 7) Chromium, 64-bit, make, Release > >> 8) Chromium, 64-bit, make, Release > > > >> Trying the other 8 configurations (64-bit Google Chrome, and 32-bit > >> Chromium > >> now). > > > >> For each config, verified: > >> a) Chrome/Chromium started w/o LD_LIBRARY_PATH > >> b) Chrome/Chromium could play the expected videos. > >> c) Chrome/Chromium loaded libffmpegsumo and not avcodec (grep sumo > >> /proc/???/maps, etc.) > >> d) Chrome/Chromium could run with libffmpegsumo.so deleted. > > > >> This should hopefully be correct now. Other than possibly > grammar/spelling > >> errors, I'm not sure what other Is are left to dot. > > > >> -Albert > > > >> On 2009/10/28 21:01:51, awong wrote: > >> > The branded build on scons actually did break. Go figure. > >> > > >> > For some reasons, scons decides to add an -fPIC into all C++ shared > >> > library > >> > targets. It doesn't do the same for C targets. This is scons's > mainline > >> > behavior, not just scons via gyp. > >> > > >> > I've created a gyp CL to make the scons gyp generator override the > -fPIC > >> > appending on the SHCCFLAGS in scons which fixes things. > >> > > >> > Assuming that CL is approved, committed, and rolled, this CL works. > >> > > >> > -Albert > >> > > >> > > >> > On 2009/10/28 02:12:22, awong wrote: > >> > > Oh, and I tests a release, branded build locally using make. Seems > to > > > > work. > > > >> > > Scons is running, but scons worked for debug, so I don't expect > >> > > problems > > > > in > >> > >> > > release. > >> > > > >> > > On 2009/10/28 02:11:35, awong wrote: > >> > > > Okay! I think this is finally ready! For r3@Lz!!!! > >> > > > > >> > > > Mark, I think the most useful incremental diff for you would be > >> > > > either > >> > agianst > >> > > > patch set 10, or patch set 7. > >> > > > > >> > > > Michael, I've included you because I touched installer.gyp. > >> > > > Basically, > > > > for > >> > >> > > > linux, we're switching from distributing 3 DLLs to one sumo one. > >> > > > > >> > > > Main changes this time around: > >> > > > 1) overriding of debug_optimization is in the right spot. > >> > > > 2) fixes to installer.gyp and chrome.gyp > >> > > > 3) Update to neuter the copy of ffmpeg_binaries for linux. > >> > > > > >> > > > Andrew, if you want to sanity check the media_posix change (and > >> > > > anything > >> > > else), > >> > > > feel free. > >> > > > > >> > > > Thanks everyone for all the help! This is pretty darned exciting > >> > > > (for me > >> at > >> > > > least). Hopefully after this is done, mac will be comparatively > >> > > > simple. > >> > > > > >> > > > -Albert > > > > > > > > http://codereview.chromium.org/300013 > > >
I pushed a new gyp change (r736) that fixes the breakage for library targets. Rolled into chromium as 30536. However, on the try server, having a problem that libz.so and libbz2.so are not available. Suck. I'm going to look into getting those added to the build bots. -Albert On Thu, Oct 29, 2009 at 3:24 PM, Albert J. Wong (=E7=8E=8B=E9=87=8D=E5=82= =91) <ajwong@chromium.org>wrote: > Ahah! The try servers haven't picked up the rolled gyp deps! That means > scons is building with -fPIC and breaking. I can hack DEPS to give it a > sensible try run. > > Unfortunately, piman's cross-compilation change (which I rolled in) broke > me. > > -Albert > > > On Thu, Oct 29, 2009 at 3:06 PM, Mark Mentovai <mark@chromium.org> wrote: > >> I'm optimistic. If you can figure out the try server problem, I hope >> to have time to do another pass tonight. I'll have my red pen out as >> usual, but I think that if we've got something demonstrably working, >> we should check it in and iterate. Thanks again for your work on >> this. >> >> Mark >> >> ajwong@chromium.org wrote: >> > And wow. I jinxed myself. try servers failed. >> > >> > On 2009/10/29 21:33:10, awong wrote: >> >> >> >> I'm pretty sure it's ready for another pass now. >> > >> >> Changes since last time: >> >> 1) Added hack to work around scons shadowing libz and libz2 with th= e >> >> static >> >> versions >> >> 2) Remove --as-needed flag cause, we don't need it, can it broke th= e >> >> scons >> >> hack. >> >> 3) Removed -fomit-frame-pointer (yay! breakpad!) >> >> 4) Added a dummy executable to break the direct dependency from >> chrome, >> > >> > etc., >> >> >> >> on libffmpegsumo.so. >> >> 5) Hack to work around evan's "-fvisibility=3Dhidden" change in >> >> common.gypi. >> >> Real fix should probably be to make his change conditionally on >> >> _type=3Dshared_library. >> > >> >> Related to this was a gyp change to the scons generator to override >> >> scons's >> >> brain-dead inclusion of -fPIC in the world. >> > >> >> Did I mention, I hate scons? >> > >> >> For tests, I've successfully built locally in the following >> >> configurations: >> >> 1) Google Chrome, 32-bit, scons, Release >> >> 2) Google Chrome, 32-bit, scons, Debug >> >> 3) Google Chrome, 32-bit, make, Release >> >> 4) Google Chrome, 32-bit, make, Debug >> >> 5) Chromium, 64-bit, scons, Debug >> >> 6) Chromium, 64-bit, scons, Release >> >> 7) Chromium, 64-bit, make, Release >> >> 8) Chromium, 64-bit, make, Release >> > >> >> Trying the other 8 configurations (64-bit Google Chrome, and 32-bit >> >> Chromium >> >> now). >> > >> >> For each config, verified: >> >> a) Chrome/Chromium started w/o LD_LIBRARY_PATH >> >> b) Chrome/Chromium could play the expected videos. >> >> c) Chrome/Chromium loaded libffmpegsumo and not avcodec (grep sumo >> >> /proc/???/maps, etc.) >> >> d) Chrome/Chromium could run with libffmpegsumo.so deleted. >> > >> >> This should hopefully be correct now. Other than possibly >> grammar/spelling >> >> errors, I'm not sure what other Is are left to dot. >> > >> >> -Albert >> > >> >> On 2009/10/28 21:01:51, awong wrote: >> >> > The branded build on scons actually did break. Go figure. >> >> > >> >> > For some reasons, scons decides to add an -fPIC into all C++ shared >> >> > library >> >> > targets. It doesn't do the same for C targets. This is scons's >> mainline >> >> > behavior, not just scons via gyp. >> >> > >> >> > I've created a gyp CL to make the scons gyp generator override the >> -fPIC >> >> > appending on the SHCCFLAGS in scons which fixes things. >> >> > >> >> > Assuming that CL is approved, committed, and rolled, this CL works. >> >> > >> >> > -Albert >> >> > >> >> > >> >> > On 2009/10/28 02:12:22, awong wrote: >> >> > > Oh, and I tests a release, branded build locally using make. See= ms >> to >> > >> > work. >> > >> >> > > Scons is running, but scons worked for debug, so I don't expect >> >> > > problems >> > >> > in >> >> >> >> > > release. >> >> > > >> >> > > On 2009/10/28 02:11:35, awong wrote: >> >> > > > Okay! I think this is finally ready! For r3@Lz!!!! >> >> > > > >> >> > > > Mark, I think the most useful incremental diff for you would be >> >> > > > either >> >> > agianst >> >> > > > patch set 10, or patch set 7. >> >> > > > >> >> > > > Michael, I've included you because I touched installer.gyp. >> >> > > > Basically, >> > >> > for >> >> >> >> > > > linux, we're switching from distributing 3 DLLs to one sumo one= . >> >> > > > >> >> > > > Main changes this time around: >> >> > > > 1) overriding of debug_optimization is in the right spot. >> >> > > > 2) fixes to installer.gyp and chrome.gyp >> >> > > > 3) Update to neuter the copy of ffmpeg_binaries for linux. >> >> > > > >> >> > > > Andrew, if you want to sanity check the media_posix change (and >> >> > > > anything >> >> > > else), >> >> > > > feel free. >> >> > > > >> >> > > > Thanks everyone for all the help! This is pretty darned excitin= g >> >> > > > (for me >> >> at >> >> > > > least). Hopefully after this is done, mac will be comparativel= y >> >> > > > simple. >> >> > > > >> >> > > > -Albert >> > >> > >> > >> > http://codereview.chromium.org/300013 >> > >> > >
thoughts on upper-case Chromium versus lowercase chromium? everything else in chrome tree is lowercase all the way (WebKit excluded, of course) http://codereview.chromium.org/300013/diff/23005/23009 File media/base/media_posix.cc (right): http://codereview.chromium.org/300013/diff/23005/23009#newcode63 Line 63: // Add the more specific ffmpeg library name. nit: ffmpeg -> FFmpeg http://codereview.chromium.org/300013/diff/23005/23010 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/23005/23010#newcode43 Line 43: Other than the configure step, ffmpeg just compiles its .c files, assembles a ffmpeg -> FFmpeg http://codereview.chromium.org/300013/diff/23005/23010#newcode76 Line 76: EBP register, otherwise some of ffmpeg's inline assembly will cause ffmpeg -> FFmpeg http://codereview.chromium.org/300013/diff/23005/23010#newcode130 Line 130: make libavcodec/libavcodec.so libavformat/libavformat.so \ do we care about libavutil, or are they built as an implicit dependency? http://codereview.chromium.org/300013/diff/23005/23012 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/23005/23012#newcode218 Line 218: # of ia32 where due to a slew of inline assembly using ebx, wasn't the register ebp? http://codereview.chromium.org/300013/diff/23005/23012#newcode219 Line 219: # ffmpeg CANNOT be built with PIC. ffmpeg -> FFmpeg http://codereview.chromium.org/300013/diff/23005/23012#newcode224 Line 224: ['ffmpeg_branding!="Chrome"', { hrmm.. curious: is it better to include files when branding = Chrome? doing exclusion-after-include increases the size of the file and makes me scared of having open source Chromium users build files they shouldn't be building :( http://codereview.chromium.org/300013/diff/23005/23012#newcode281 Line 281: 'source/patched-ffmpeg-mt/libavcodec/x86/dsputil_yasm.asm', how do we plan on configuring this section for non-x86 platforms? I'm fine leaving as is, just curious http://codereview.chromium.org/300013/diff/23005/23012#newcode372 Line 372: # On Make and Scons builds, the library does not end up in TODO ? http://codereview.chromium.org/300013/diff/23005/23013 File third_party/ffmpeg/munge_config_optimizations.sh (right): http://codereview.chromium.org/300013/diff/23005/23013#newcode19 Line 19: # However, not all the assembly blocks requiring 6 registers are excluded by add TODO: upstream fix to FFmpeg ?
Thanks for the review. Addressed the comments. As for the capital versus lower case, I'd rather keep it capital. Othewrise, I have to add another variable just to lower-case the name which seems silly. http://codereview.chromium.org/300013/diff/23005/23009 File media/base/media_posix.cc (right): http://codereview.chromium.org/300013/diff/23005/23009#newcode63 Line 63: // Add the more specific ffmpeg library name. On 2009/10/30 00:25:11, scherkus wrote: > nit: ffmpeg -> FFmpeg Done. http://codereview.chromium.org/300013/diff/23005/23010 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/23005/23010#newcode43 Line 43: Other than the configure step, ffmpeg just compiles its .c files, assembles a On 2009/10/30 00:25:11, scherkus wrote: > ffmpeg -> FFmpeg Done. http://codereview.chromium.org/300013/diff/23005/23010#newcode76 Line 76: EBP register, otherwise some of ffmpeg's inline assembly will cause On 2009/10/30 00:25:11, scherkus wrote: > ffmpeg -> FFmpeg Done. http://codereview.chromium.org/300013/diff/23005/23010#newcode130 Line 130: make libavcodec/libavcodec.so libavformat/libavformat.so \ On 2009/10/30 00:25:11, scherkus wrote: > do we care about libavutil, or are they built as an implicit dependency? Implicit, and I think leaving it implicit is better. http://codereview.chromium.org/300013/diff/23005/23012 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/23005/23012#newcode218 Line 218: # of ia32 where due to a slew of inline assembly using ebx, On 2009/10/30 00:25:11, scherkus wrote: > wasn't the register ebp? ebp -> -fomit-frame-pointer ebx -> -fPIC So in this case, it's ebx. http://codereview.chromium.org/300013/diff/23005/23012#newcode219 Line 219: # ffmpeg CANNOT be built with PIC. On 2009/10/30 00:25:11, scherkus wrote: > ffmpeg -> FFmpeg Done. http://codereview.chromium.org/300013/diff/23005/23012#newcode224 Line 224: ['ffmpeg_branding!="Chrome"', { On 2009/10/30 00:25:11, scherkus wrote: > hrmm.. curious: is it better to include files when branding = Chrome? > > doing exclusion-after-include increases the size of the file and makes me scared > of having open source Chromium users build files they shouldn't be building :( Style elsewhere was to explicitly exclude. I can do either way, but this CL, I'd rather not churn any further. http://codereview.chromium.org/300013/diff/23005/23012#newcode281 Line 281: 'source/patched-ffmpeg-mt/libavcodec/x86/dsputil_yasm.asm', On 2009/10/30 00:25:11, scherkus wrote: > how do we plan on configuring this section for non-x86 platforms? > > I'm fine leaving as is, just curious Punted. Was going to deal with it when it came. http://codereview.chromium.org/300013/diff/23005/23012#newcode372 Line 372: # On Make and Scons builds, the library does not end up in On 2009/10/30 00:25:11, scherkus wrote: > TODO ? Done. http://codereview.chromium.org/300013/diff/23005/23013 File third_party/ffmpeg/munge_config_optimizations.sh (right): http://codereview.chromium.org/300013/diff/23005/23013#newcode19 Line 19: # However, not all the assembly blocks requiring 6 registers are excluded by On 2009/10/30 00:25:11, scherkus wrote: > add TODO: upstream fix to FFmpeg ? Nope. They *want* to build w/o PIC on ia32. I'm pretty certain it's intentional. There's sooo much code that just ignores the EBX things. I think the EBX flag is meant for 64-bit only.
Oh, and it turns out gold doesn't like the -l:libz.so syntax. Go figure. I'm going to rename the zlib and bzip2 outputs. On 2009/10/30 00:32:02, awong wrote: > Thanks for the review. Addressed the comments. > > As for the capital versus lower case, I'd rather keep it capital. Othewrise, I > have to add another variable just to lower-case the name which seems silly. > > http://codereview.chromium.org/300013/diff/23005/23009 > File media/base/media_posix.cc (right): > > http://codereview.chromium.org/300013/diff/23005/23009#newcode63 > Line 63: // Add the more specific ffmpeg library name. > On 2009/10/30 00:25:11, scherkus wrote: > > nit: ffmpeg -> FFmpeg > > Done. > > http://codereview.chromium.org/300013/diff/23005/23010 > File third_party/ffmpeg/README.chromium (right): > > http://codereview.chromium.org/300013/diff/23005/23010#newcode43 > Line 43: Other than the configure step, ffmpeg just compiles its .c files, > assembles a > On 2009/10/30 00:25:11, scherkus wrote: > > ffmpeg -> FFmpeg > > Done. > > http://codereview.chromium.org/300013/diff/23005/23010#newcode76 > Line 76: EBP register, otherwise some of ffmpeg's inline assembly will cause > On 2009/10/30 00:25:11, scherkus wrote: > > ffmpeg -> FFmpeg > > Done. > > http://codereview.chromium.org/300013/diff/23005/23010#newcode130 > Line 130: make libavcodec/libavcodec.so libavformat/libavformat.so \ > On 2009/10/30 00:25:11, scherkus wrote: > > do we care about libavutil, or are they built as an implicit dependency? > > Implicit, and I think leaving it implicit is better. > > http://codereview.chromium.org/300013/diff/23005/23012 > File third_party/ffmpeg/ffmpeg.gyp (right): > > http://codereview.chromium.org/300013/diff/23005/23012#newcode218 > Line 218: # of ia32 where due to a slew of inline assembly using ebx, > On 2009/10/30 00:25:11, scherkus wrote: > > wasn't the register ebp? > > ebp -> -fomit-frame-pointer > ebx -> -fPIC > > So in this case, it's ebx. > > http://codereview.chromium.org/300013/diff/23005/23012#newcode219 > Line 219: # ffmpeg CANNOT be built with PIC. > On 2009/10/30 00:25:11, scherkus wrote: > > ffmpeg -> FFmpeg > > Done. > > http://codereview.chromium.org/300013/diff/23005/23012#newcode224 > Line 224: ['ffmpeg_branding!="Chrome"', { > On 2009/10/30 00:25:11, scherkus wrote: > > hrmm.. curious: is it better to include files when branding = Chrome? > > > > doing exclusion-after-include increases the size of the file and makes me > scared > > of having open source Chromium users build files they shouldn't be building :( > > Style elsewhere was to explicitly exclude. I can do either way, but this CL, > I'd rather not churn any further. > > http://codereview.chromium.org/300013/diff/23005/23012#newcode281 > Line 281: 'source/patched-ffmpeg-mt/libavcodec/x86/dsputil_yasm.asm', > On 2009/10/30 00:25:11, scherkus wrote: > > how do we plan on configuring this section for non-x86 platforms? > > > > I'm fine leaving as is, just curious > > Punted. Was going to deal with it when it came. > > http://codereview.chromium.org/300013/diff/23005/23012#newcode372 > Line 372: # On Make and Scons builds, the library does not end up in > On 2009/10/30 00:25:11, scherkus wrote: > > TODO ? > > Done. > > http://codereview.chromium.org/300013/diff/23005/23013 > File third_party/ffmpeg/munge_config_optimizations.sh (right): > > http://codereview.chromium.org/300013/diff/23005/23013#newcode19 > Line 19: # However, not all the assembly blocks requiring 6 registers are > excluded by > On 2009/10/30 00:25:11, scherkus wrote: > > add TODO: upstream fix to FFmpeg ? > > Nope. They *want* to build w/o PIC on ia32. I'm pretty certain it's > intentional. There's sooo much code that just ignores the EBX things. I think > the EBX flag is meant for 64-bit only.
Got it up on the try servers again. Tony make sure that libzip2.so and libz.so were installed on all the try servers & buildbots, so this should work. The latest change was to just rename our produced libz and libbz2. Why do we let them class with the system libraries anyways? -Albert On 2009/10/30 00:33:09, awong wrote: > Oh, and it turns out gold doesn't like the -l:libz.so syntax. Go figure. > > I'm going to rename the zlib and bzip2 outputs. > > On 2009/10/30 00:32:02, awong wrote: > > Thanks for the review. Addressed the comments. > > > > As for the capital versus lower case, I'd rather keep it capital. Othewrise, > I > > have to add another variable just to lower-case the name which seems silly. > > > > http://codereview.chromium.org/300013/diff/23005/23009 > > File media/base/media_posix.cc (right): > > > > http://codereview.chromium.org/300013/diff/23005/23009#newcode63 > > Line 63: // Add the more specific ffmpeg library name. > > On 2009/10/30 00:25:11, scherkus wrote: > > > nit: ffmpeg -> FFmpeg > > > > Done. > > > > http://codereview.chromium.org/300013/diff/23005/23010 > > File third_party/ffmpeg/README.chromium (right): > > > > http://codereview.chromium.org/300013/diff/23005/23010#newcode43 > > Line 43: Other than the configure step, ffmpeg just compiles its .c files, > > assembles a > > On 2009/10/30 00:25:11, scherkus wrote: > > > ffmpeg -> FFmpeg > > > > Done. > > > > http://codereview.chromium.org/300013/diff/23005/23010#newcode76 > > Line 76: EBP register, otherwise some of ffmpeg's inline assembly will cause > > On 2009/10/30 00:25:11, scherkus wrote: > > > ffmpeg -> FFmpeg > > > > Done. > > > > http://codereview.chromium.org/300013/diff/23005/23010#newcode130 > > Line 130: make libavcodec/libavcodec.so libavformat/libavformat.so \ > > On 2009/10/30 00:25:11, scherkus wrote: > > > do we care about libavutil, or are they built as an implicit dependency? > > > > Implicit, and I think leaving it implicit is better. > > > > http://codereview.chromium.org/300013/diff/23005/23012 > > File third_party/ffmpeg/ffmpeg.gyp (right): > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode218 > > Line 218: # of ia32 where due to a slew of inline assembly using ebx, > > On 2009/10/30 00:25:11, scherkus wrote: > > > wasn't the register ebp? > > > > ebp -> -fomit-frame-pointer > > ebx -> -fPIC > > > > So in this case, it's ebx. > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode219 > > Line 219: # ffmpeg CANNOT be built with PIC. > > On 2009/10/30 00:25:11, scherkus wrote: > > > ffmpeg -> FFmpeg > > > > Done. > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode224 > > Line 224: ['ffmpeg_branding!="Chrome"', { > > On 2009/10/30 00:25:11, scherkus wrote: > > > hrmm.. curious: is it better to include files when branding = Chrome? > > > > > > doing exclusion-after-include increases the size of the file and makes me > > scared > > > of having open source Chromium users build files they shouldn't be building > :( > > > > Style elsewhere was to explicitly exclude. I can do either way, but this CL, > > I'd rather not churn any further. > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode281 > > Line 281: 'source/patched-ffmpeg-mt/libavcodec/x86/dsputil_yasm.asm', > > On 2009/10/30 00:25:11, scherkus wrote: > > > how do we plan on configuring this section for non-x86 platforms? > > > > > > I'm fine leaving as is, just curious > > > > Punted. Was going to deal with it when it came. > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode372 > > Line 372: # On Make and Scons builds, the library does not end up in > > On 2009/10/30 00:25:11, scherkus wrote: > > > TODO ? > > > > Done. > > > > http://codereview.chromium.org/300013/diff/23005/23013 > > File third_party/ffmpeg/munge_config_optimizations.sh (right): > > > > http://codereview.chromium.org/300013/diff/23005/23013#newcode19 > > Line 19: # However, not all the assembly blocks requiring 6 registers are > > excluded by > > On 2009/10/30 00:25:11, scherkus wrote: > > > add TODO: upstream fix to FFmpeg ? > > > > Nope. They *want* to build w/o PIC on ia32. I'm pretty certain it's > > intentional. There's sooo much code that just ignores the EBX things. I > think > > the EBX flag is meant for 64-bit only.
I see 3 greens on the try servers! linux_view is being annoying...but the compile failure isn't mine as far as I can tell. I declare this ready to CL back in business! Until the next case of bitrot tears the platform out from under it again. -Albert On 2009/10/30 01:11:29, awong wrote: > Got it up on the try servers again. Tony make sure that libzip2.so and libz.so > were installed on all the try servers & buildbots, so this should work. > > The latest change was to just rename our produced libz and libbz2. Why do we let > them class with the system libraries anyways? > > -Albert > > > On 2009/10/30 00:33:09, awong wrote: > > Oh, and it turns out gold doesn't like the -l:libz.so syntax. Go figure. > > > > I'm going to rename the zlib and bzip2 outputs. > > > > On 2009/10/30 00:32:02, awong wrote: > > > Thanks for the review. Addressed the comments. > > > > > > As for the capital versus lower case, I'd rather keep it capital. > Othewrise, > > I > > > have to add another variable just to lower-case the name which seems silly. > > > > > > http://codereview.chromium.org/300013/diff/23005/23009 > > > File media/base/media_posix.cc (right): > > > > > > http://codereview.chromium.org/300013/diff/23005/23009#newcode63 > > > Line 63: // Add the more specific ffmpeg library name. > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > nit: ffmpeg -> FFmpeg > > > > > > Done. > > > > > > http://codereview.chromium.org/300013/diff/23005/23010 > > > File third_party/ffmpeg/README.chromium (right): > > > > > > http://codereview.chromium.org/300013/diff/23005/23010#newcode43 > > > Line 43: Other than the configure step, ffmpeg just compiles its .c files, > > > assembles a > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > ffmpeg -> FFmpeg > > > > > > Done. > > > > > > http://codereview.chromium.org/300013/diff/23005/23010#newcode76 > > > Line 76: EBP register, otherwise some of ffmpeg's inline assembly will cause > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > ffmpeg -> FFmpeg > > > > > > Done. > > > > > > http://codereview.chromium.org/300013/diff/23005/23010#newcode130 > > > Line 130: make libavcodec/libavcodec.so libavformat/libavformat.so \ > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > do we care about libavutil, or are they built as an implicit dependency? > > > > > > Implicit, and I think leaving it implicit is better. > > > > > > http://codereview.chromium.org/300013/diff/23005/23012 > > > File third_party/ffmpeg/ffmpeg.gyp (right): > > > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode218 > > > Line 218: # of ia32 where due to a slew of inline assembly using ebx, > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > wasn't the register ebp? > > > > > > ebp -> -fomit-frame-pointer > > > ebx -> -fPIC > > > > > > So in this case, it's ebx. > > > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode219 > > > Line 219: # ffmpeg CANNOT be built with PIC. > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > ffmpeg -> FFmpeg > > > > > > Done. > > > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode224 > > > Line 224: ['ffmpeg_branding!="Chrome"', { > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > hrmm.. curious: is it better to include files when branding = Chrome? > > > > > > > > doing exclusion-after-include increases the size of the file and makes me > > > scared > > > > of having open source Chromium users build files they shouldn't be > building > > :( > > > > > > Style elsewhere was to explicitly exclude. I can do either way, but this > CL, > > > I'd rather not churn any further. > > > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode281 > > > Line 281: 'source/patched-ffmpeg-mt/libavcodec/x86/dsputil_yasm.asm', > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > how do we plan on configuring this section for non-x86 platforms? > > > > > > > > I'm fine leaving as is, just curious > > > > > > Punted. Was going to deal with it when it came. > > > > > > http://codereview.chromium.org/300013/diff/23005/23012#newcode372 > > > Line 372: # On Make and Scons builds, the library does not end up in > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > TODO ? > > > > > > Done. > > > > > > http://codereview.chromium.org/300013/diff/23005/23013 > > > File third_party/ffmpeg/munge_config_optimizations.sh (right): > > > > > > http://codereview.chromium.org/300013/diff/23005/23013#newcode19 > > > Line 19: # However, not all the assembly blocks requiring 6 registers are > > > excluded by > > > On 2009/10/30 00:25:11, scherkus wrote: > > > > add TODO: upstream fix to FFmpeg ? > > > > > > Nope. They *want* to build w/o PIC on ia32. I'm pretty certain it's > > > intentional. There's sooo much code that just ignores the EBX things. I > > think > > > the EBX flag is meant for 64-bit only.
LG Take care, young (gyp) padawan. As a final sanity check, does anyone need to do/run/delete/move anything after getting this update, other than gclient sync? Deleting .gclient_entries comes to mind, etc....
No issues with that yet. Everything is still mapped in. We just stopped copying the file over. Preemption in the media_posix.cc shoudl handle having both DSOs in the PRODUCT_DIR. -Albert On Thu, Oct 29, 2009 at 7:01 PM, <scherkus@chromium.org> wrote: > LG > > Take care, young (gyp) padawan. > > As a final sanity check, does anyone need to do/run/delete/move anything > after > getting this update, other than gclient sync? > > Deleting .gclient_entries comes to mind, etc.... > > > http://codereview.chromium.org/300013 >
OK, at long last, you're the recipient of the coveted LGTM award. The remaining comments are all nitty and not big deals. Fix 'em, give it a last shot through the try server if you like, and check it in. Then we can go back to square one all over again. Nice job, Albert! http://codereview.chromium.org/300013/diff/21050/28007 File third_party/bzip2/bzip2.gyp (right): http://codereview.chromium.org/300013/diff/21050/28007#newcode37 Line 37: 'product_name': 'chrome_bz2', Don't do this on the Mac. Make this Linux-only. http://codereview.chromium.org/300013/diff/21050/28008 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/21050/28008#newcode2 Line 2: crate the ffmpeg.gyp file + related configurations. say "and" aloud, not "+" http://codereview.chromium.org/300013/diff/21050/28008#newcode38 Line 38: The FFmpeg build is relatively straightforward. All files are build with are built with http://codereview.chromium.org/300013/diff/21050/28010 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/21050/28010#newcode14 Line 14: # unacceptable w/o optimization, freeze the optimizations to -O2. without http://codereview.chromium.org/300013/diff/21050/28010#newcode311 Line 311: 'action_name': 'make_library', We need a way to add .o files directly to sources in GYP. If GYP doesn't have a bug for that, we should file one. http://codereview.chromium.org/300013/diff/21050/28010#newcode354 Line 354: # TODO(ajwong): Fix gyp, fix the world. :) http://codereview.chromium.org/300013/diff/21050/28012 File third_party/ffmpeg/source/config/Chrome/linux/ia32/config.h (right): http://codereview.chromium.org/300013/diff/21050/28012#newcode1 Line 1: /* Automatically generated by configure - do not modify! */ I'm not reading these! http://codereview.chromium.org/300013/diff/21050/28015 File third_party/ffmpeg/source/config/Chrome/linux/x64/version.h (right): http://codereview.chromium.org/300013/diff/21050/28015#newcode1 Line 1: #define FFMPEG_VERSION "SVN-r28636" OK, well maybe I'll read some of them. This is fine for now, but ultimately, we don't need so many copies of this. One copy will be much more than enough. http://codereview.chromium.org/300013/diff/21050/28020 File third_party/yasm/README.chromium (right): http://codereview.chromium.org/300013/diff/21050/28020#newcode1 Line 1: See also the yasm.gyp file for a description of the yasm build process. And I've already read this before, so I won't reread it. http://codereview.chromium.org/300013/diff/21050/28021 File third_party/yasm/source/config/linux/Makefile (right): http://codereview.chromium.org/300013/diff/21050/28021#newcode1 Line 1: # Makefile.in generated by automake 1.10.1 from Makefile.am. And I'm not reading this either. http://codereview.chromium.org/300013/diff/21050/28024 File third_party/yasm/yasm.gyp (right): http://codereview.chromium.org/300013/diff/21050/28024#newcode376 Line 376: 'gen_insn_path': 'source/patched-yasm/modules/arch/x86/gen_x86_insn.py', Pity that this is the only line that goes past 80 in the whole file. Lately in .gyp files, I've been doing a hanging 4-space indent in cases like this to keep things a little tidier. http://codereview.chromium.org/300013/diff/21050/28025 File third_party/zlib/zlib.gyp (right): http://codereview.chromium.org/300013/diff/21050/28025#newcode61 Line 61: 'product_name': 'chrome_zlib', Again, don't do this on the Mac.
I take back what I said about not doing chrome_bz2 and chrome_zlib on the Mac. You can leave what you did.
Comments fixed. Will submit when it feels safe. http://codereview.chromium.org/300013/diff/21050/28007 File third_party/bzip2/bzip2.gyp (right): http://codereview.chromium.org/300013/diff/21050/28007#newcode37 Line 37: 'product_name': 'chrome_bz2', On 2009/10/30 03:38:02, Mark Mentovai wrote: > Don't do this on the Mac. Make this Linux-only. leaving as is per e-mail. http://codereview.chromium.org/300013/diff/21050/28008 File third_party/ffmpeg/README.chromium (right): http://codereview.chromium.org/300013/diff/21050/28008#newcode2 Line 2: crate the ffmpeg.gyp file + related configurations. On 2009/10/30 03:38:02, Mark Mentovai wrote: > say "and" aloud, not "+" But...but...I speak punctuation. ? ! ... . . http://codereview.chromium.org/300013/diff/21050/28008#newcode38 Line 38: The FFmpeg build is relatively straightforward. All files are build with On 2009/10/30 03:38:02, Mark Mentovai wrote: > are built with Done. http://codereview.chromium.org/300013/diff/21050/28010 File third_party/ffmpeg/ffmpeg.gyp (right): http://codereview.chromium.org/300013/diff/21050/28010#newcode14 Line 14: # unacceptable w/o optimization, freeze the optimizations to -O2. On 2009/10/30 03:38:02, Mark Mentovai wrote: > without Done. http://codereview.chromium.org/300013/diff/21050/28010#newcode311 Line 311: 'action_name': 'make_library', On 2009/10/30 03:38:02, Mark Mentovai wrote: > We need a way to add .o files directly to sources in GYP. If GYP doesn't have a > bug for that, we should file one. Filed: http://code.google.com/p/gyp/issues/detail?id=102 http://codereview.chromium.org/300013/diff/21050/28015 File third_party/ffmpeg/source/config/Chrome/linux/x64/version.h (right): http://codereview.chromium.org/300013/diff/21050/28015#newcode1 Line 1: #define FFMPEG_VERSION "SVN-r28636" On 2009/10/30 03:38:02, Mark Mentovai wrote: > OK, well maybe I'll read some of them. > > This is fine for now, but ultimately, we don't need so many copies of this. One > copy will be much more than enough. Actually, ultimately, I'd like to not have it checked in at all, and have it auto-generate the string "ffmpeg-<(ffmpeg_branding)-<(svn_revision)" if I can figure out how to get svn_revision. http://codereview.chromium.org/300013/diff/21050/28024 File third_party/yasm/yasm.gyp (right): http://codereview.chromium.org/300013/diff/21050/28024#newcode376 Line 376: 'gen_insn_path': 'source/patched-yasm/modules/arch/x86/gen_x86_insn.py', On 2009/10/30 03:38:02, Mark Mentovai wrote: > Pity that this is the only line that goes past 80 in the whole file. > > Lately in .gyp files, I've been doing a hanging 4-space indent in cases like > this to keep things a little tidier. Done. http://codereview.chromium.org/300013/diff/21050/28025 File third_party/zlib/zlib.gyp (right): http://codereview.chromium.org/300013/diff/21050/28025#newcode61 Line 61: 'product_name': 'chrome_zlib', On 2009/10/30 03:38:02, Mark Mentovai wrote: > Again, don't do this on the Mac. leaving as-is per e-mail. |
