To enable vp9 playback we need to update our in-tree libvpx to a release or, in the near term, git checkout, which supports it. This should land after we update to the last libvpx release so we have a rollback point.

replace update.sh/update_update.sh.py with update.py update.py can update the build system to a given commit id - checking out upstream git - creating platform dependend config files - adding/removing changed libvpx files - updating moz.build

Until 1.3.0 is released, use the git branch forest. This patch is based on: 4fbc1210ed81705b14f00f02c4508a0c413b04dc

Both patches are stacked on top of the libvpx 1.2.0 patches from Bug 763495

Rebase on top of latest 1.2.0 patch (Bug 763495)

also build C asm files on android

rebase patch on top of https://hg.mozilla.org/integration/mozilla-inbound/rev/f0144998bab0

Update patch to git tag v1.3.0

Comment on attachment 8341364 [details] [diff] [review] 0002-Update-libvpx-to-1.3.0v2.patch Review of attachment 8341364 [details] [diff] [review]: ----------------------------------------------------------------- Don't we need to check if the return value of nestegg_track_codec_id() is NESTEGG_CODEC_VP8 (or NESTEGG_CODEC_VP9) in WebMReader::ReadMetadata(), else we'll be playing VP9 by accident once this lands? We do want to playing VP9, but we don't want to be playing it by accident! I'm assuming that libnestegg supports VP9, since it has a codec ID defined for VP9, perhaps that's not the case?

(In reply to Chris Pearce (:cpearce) from comment #8) > Comment on attachment 8341364 [details] [diff] [review] > 0002-Update-libvpx-to-1.3.0v2.patch > > Review of attachment 8341364 [details] [diff] [review]: > ----------------------------------------------------------------- > > Don't we need to check if the return value of nestegg_track_codec_id() is > NESTEGG_CODEC_VP8 (or NESTEGG_CODEC_VP9) in WebMReader::ReadMetadata(), else > we'll be playing VP9 by accident once this lands? We do want to playing VP9, > but we don't want to be playing it by accident! The patch for Bug 833023 will fix that. Without it, it would fail trying to initialize the vp8 decoder with vp9 data. > I'm assuming that libnestegg supports VP9, since it has a codec ID defined > for VP9, perhaps that's not the case? that part already landed

update libvpx part of patch: - use hg addremove to prepare patch - include updated generated files: - 32bit osx needs --enable-pic - manually disable AVX2 on linux since mozilla build setup does not support it currently

update build part of patch: - 32bit osx needs --enable-pic - manually disable AVX2 on Linux since mozilla build setup does not support it currently

Green on try. https://tbpl.mozilla.org/?tree=Try&rev=05add4b0e858

Comment on attachment 8342317 [details] [diff] [review] vpx1.3.0v5_build.patch Review of attachment 8342317 [details] [diff] [review]: ----------------------------------------------------------------- I won't block you on the mozpack stuff and linux, but the rest should be addressed. ::: media/libvpx/update.py @@ +270,5 @@ > + os.makedirs(target_objdir) > + os.chdir(target_objdir) > + configure = ['../../configure', '--target=%s' % target, '--disable-examples', '--disable-install-docs'] > + > + if 'darwin9' in target: trailing whitespace @@ +276,5 @@ > + if 'linux' in target: > + configure += ['--enable-pic'] > + # mozilla linux toolchain currently does not support avx2, > + # remove once gcc is updated > + configure += ['--disable-avx2'] You're adding a test to configure.in and are enabling the avx source file when the test passes. Doesn't this essentially mean this code will be built for nothing if the compiler passes the configure.in check, since hardcoded in the required_files, you have avx disabled? @@ +306,5 @@ > +def get_libvpx_files(prefix): > + for root, folders, files in os.walk(prefix): > + for f in files: > + f = os.path.join(root, f)[len(prefix):] > + if f.split('.')[-1] in extensions \ use os.path.splitext. @@ +307,5 @@ > + for root, folders, files in os.walk(prefix): > + for f in files: > + f = os.path.join(root, f)[len(prefix):] > + if f.split('.')[-1] in extensions \ > + and '/' in f \ os.sep in f @@ +309,5 @@ > + f = os.path.join(root, f)[len(prefix):] > + if f.split('.')[-1] in extensions \ > + and '/' in f \ > + and f not in ignore_files \ > + and not filter(lambda folder: folder in f, ignore_folders): and not all(folder in f for folder in ignore_folders) with that being said that whole function should probably be rewritten to use mozpack.files.FileFinder when bug 941245 lands. @@ +322,5 @@ > + for mk in mk_files: > + with open(os.path.join(prefix, mk)) as f: > + base = os.path.dirname(mk) > + data = f.read().split('\n') > + for l in data: for l in f: @@ +331,5 @@ > + value = os.path.join(base, value) > + if not key.startswith('#') and value.split('.')[-1] in extensions: > + if key not in source: > + source[key] = [] > + source[key].append(value) If those .mk even change to use = for the first assignment, you'll miss them. You should probably investigate using the pymake api. @@ +337,5 @@ > + for key in source: > + for f in source[key]: > + if key.endswith('EXPORTS') and f.endswith('.h'): > + files['EXPORTS'].append(f) > + if f.split('.')[-1] in ('c', 'asm') and not f in manual: os.path.splitext @@ +367,5 @@ > + moz_build_new = re.sub(re.compile('files = {.*?}\n', re.DOTALL), vpx_files, moz_build) > + if moz_build != moz_build_new: > + print 'updating moz.build' > + with open('moz.build', 'w') as f: > + f.write(moz_build_new) Might as well overwrite a sources.mozbuild file and include that from moz.build. That avoids the regexp munging of moz.build. @@ +378,5 @@ > + if 'upstream/' in f or not '/' in f: > + continue > + if f.split('.')[-1] in extensions and '/': > + current_files.append(f) > + return current_files Same remarks as for get_libvpx_files. @@ +420,5 @@ > + first = True > + for f in ( > + 'vpx_config.h', 'vpx_config.c', > + 'vp8_rtcd.h', 'vp9_rtcd.h', 'vpx_scale_rtcd.h', > + 'vpx_config.asm' This list is partly shared with required_files in prepare_upstream. You should probably use a common variable, and only run make for the files that don't exist in prepare_upstream. @@ +443,5 @@ > + if removed_files: > + print "Remove files:" > + for f in removed_files: > + os.unlink(f) > + print ' ', f The whole business of updating/copying/removing files could be handled with mozpack.copier.FileCopier. @@ +493,5 @@ > + pprint(unknown) > + > +def usage(): > + print 'Usage: %s /path/to/ndk [commit]' % sys.argv[0] > + print 'This script only works on Mac OS X since the OS X Toolchain is not available on other platforms.' Want to bet this can work on linux with clang? ;) @@ +510,5 @@ > + commit = sys.argv[2] > + else: > + commit = None > + > + DEBUG = sys.argv[-1] == '--debug' You should use argparse.

(In reply to Mike Hommey [:glandium] from comment #13) > Comment on attachment 8342317 [details] [diff] [review] > vpx1.3.0v5_build.patch > > Review of attachment 8342317 [details] [diff] [review]: > ----------------------------------------------------------------- > > I won't block you on the mozpack stuff and linux, but the rest should be > addressed. > > ::: media/libvpx/update.py > @@ +270,5 @@ > > + os.makedirs(target_objdir) > > + os.chdir(target_objdir) > > + configure = ['../../configure', '--target=%s' % target, '--disable-examples', '--disable-install-docs'] > > + > > + if 'darwin9' in target: > > trailing whitespace ok > > @@ +276,5 @@ > > + if 'linux' in target: > > + configure += ['--enable-pic'] > > + # mozilla linux toolchain currently does not support avx2, > > + # remove once gcc is updated > > + configure += ['--disable-avx2'] > > You're adding a test to configure.in and are enabling the avx source file > when the test passes. Doesn't this essentially mean this code will be built > for nothing if the compiler passes the configure.in check, since hardcoded > in the required_files, you have avx disabled? Ok backed out the AVX2 test in configure for now and just use AVX2 on OS X. Filed Bug 946639 to enable it on Linux once build servers support it. > @@ +306,5 @@ > > +def get_libvpx_files(prefix): > > + for root, folders, files in os.walk(prefix): > > + for f in files: > > + f = os.path.join(root, f)[len(prefix):] > > + if f.split('.')[-1] in extensions \ > > use os.path.splitext. ok > > @@ +307,5 @@ > > + for root, folders, files in os.walk(prefix): > > + for f in files: > > + f = os.path.join(root, f)[len(prefix):] > > + if f.split('.')[-1] in extensions \ > > + and '/' in f \ > > os.sep in f ok > > @@ +309,5 @@ > > + f = os.path.join(root, f)[len(prefix):] > > + if f.split('.')[-1] in extensions \ > > + and '/' in f \ > > + and f not in ignore_files \ > > + and not filter(lambda folder: folder in f, ignore_folders): > > and not all(folder in f for folder in ignore_folders) almost, this must be: f is not in any of the ignored folders, so updated to: and not any(folder in f for folder in ignore_folders) > with that being said that whole function should probably be rewritten to use > mozpack.files.FileFinder when bug 941245 lands. Filed Bug 946619 to use mozpack.files in update.py > > @@ +322,5 @@ > > + for mk in mk_files: > > + with open(os.path.join(prefix, mk)) as f: > > + base = os.path.dirname(mk) > > + data = f.read().split('\n') > > + for l in data: > > for l in f: ok > > @@ +331,5 @@ > > + value = os.path.join(base, value) > > + if not key.startswith('#') and value.split('.')[-1] in extensions: > > + if key not in source: > > + source[key] = [] > > + source[key].append(value) > > If those .mk even change to use = for the first assignment, you'll miss > them. You should probably investigate using the pymake api. Yes this update script will need to evolve as libvpx gets updated. So far this was a manual process, update.py documents this in a script. pymake api sounds like it could be a good improvement in the future. Filed Bug 946621 to make use of pymake api. > @@ +337,5 @@ > > + for key in source: > > + for f in source[key]: > > + if key.endswith('EXPORTS') and f.endswith('.h'): > > + files['EXPORTS'].append(f) > > + if f.split('.')[-1] in ('c', 'asm') and not f in manual: > > os.path.splitext ok > @@ +367,5 @@ > > + moz_build_new = re.sub(re.compile('files = {.*?}\n', re.DOTALL), vpx_files, moz_build) > > + if moz_build != moz_build_new: > > + print 'updating moz.build' > > + with open('moz.build', 'w') as f: > > + f.write(moz_build_new) > > Might as well overwrite a sources.mozbuild file and include that from > moz.build. That avoids the regexp munging of moz.build. ok > > @@ +378,5 @@ > > + if 'upstream/' in f or not '/' in f: > > + continue > > + if f.split('.')[-1] in extensions and '/': > > + current_files.append(f) > > + return current_files > > Same remarks as for get_libvpx_files. > ok > @@ +420,5 @@ > > + first = True > > + for f in ( > > + 'vpx_config.h', 'vpx_config.c', > > + 'vp8_rtcd.h', 'vp9_rtcd.h', 'vpx_scale_rtcd.h', > > + 'vpx_config.asm' > > This list is partly shared with required_files in prepare_upstream. You > should probably use a common variable, and only run make for the files that > don't exist in prepare_upstream. all those files are generated by configure/make but > ok > @@ +443,5 @@ > > + if removed_files: > > + print "Remove files:" > > + for f in removed_files: > > + os.unlink(f) > > + print ' ', f > > The whole business of updating/copying/removing files could be handled with > mozpack.copier.FileCopier. Added this to Bug 946619 > > @@ +493,5 @@ > > + pprint(unknown) > > + > > +def usage(): > > + print 'Usage: %s /path/to/ndk [commit]' % sys.argv[0] > > + print 'This script only works on Mac OS X since the OS X Toolchain is not available on other platforms.' > > Want to bet this can work on linux with clang? ;) gcc(4.8)/clang(3.2) do not support -mmacosx-version-min=10.5 on Linux. As a result libvpx configure failes with: Unable to invoke compiler: clang -mmacosx-version-min=10.5 -m64 -arch x86_64 clang: error: the clang compiler does not support '-mmacosx-version-min=10.5' Unable to invoke compiler: gcc -mmacosx-version-min=10.5 -m64 -arch x86_64 gcc-4.8.real: error: unrecognized command line option ‘-mmacosx-version-min=10.5’ still want to bet? :) > > @@ +510,5 @@ > > + commit = sys.argv[2] > > + else: > > + commit = None > > + > > + DEBUG = sys.argv[-1] == '--debug' > > You should use argparse. ok

(In reply to Jan Gerber from comment #14) > > Want to bet this can work on linux with clang? ;) > > gcc(4.8)/clang(3.2) do not support -mmacosx-version-min=10.5 on Linux. > As a result libvpx configure failes with: > > Unable to invoke compiler: clang -mmacosx-version-min=10.5 -m64 -arch > x86_64 > clang: error: the clang compiler does not support > '-mmacosx-version-min=10.5' Well, you need -target x86_64-apple-darwin. -mmacosx-version-min is accepted just fine (with a warning that the driver doesn't *do* anything with it, sadly) if you use the correct -target.

(In reply to Nathan Froyd (:froydnj) from comment #15) > Well, you need -target x86_64-apple-darwin. -mmacosx-version-min is > accepted just fine (with a warning that the driver doesn't *do* anything > with it, sadly) if you use the correct -target. good to know, my point remains though: libvpx/configure is not able to run on linux (gcc or clang) to produce the files we need for os x. In any case, this seams outside of the scope of this bug, so lets focus on getting libvpx 1.3.0 into m-c. And improve update.py to work on Linux if there is a real need for it later. (Since one has to compile on all platforms to test the result of update.py, it does not really matter where it runs, so os x is just fine for me)

(In reply to Jan Gerber from comment #16) > (In reply to Nathan Froyd (:froydnj) from comment #15) > > Well, you need -target x86_64-apple-darwin. -mmacosx-version-min is > > accepted just fine (with a warning that the driver doesn't *do* anything > > with it, sadly) if you use the correct -target. > > good to know, my point remains though: > libvpx/configure is not able to run on linux (gcc or clang) to produce the > files we need for os x. Why not? clang on linux can compile things just fine for mac; we're working on just that in bug 921040. If there's some fundamental reason libvpx/configure won't work in that scenario, then we need to fix libvpx.

That's fine, but it's not this bug.

Comment on attachment 8343024 [details] [diff] [review] vpx1.3.0v6_build.patch Review of attachment 8343024 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libvpx/update.py @@ +336,5 @@ > + for l in f: > + if '+=' in l: > + l = l.split('+=') > + key = l[0].strip() > + value = '+='.join(l[1:]).strip().replace('$(ASM)', '.asm') '+='.join(l[1:]) seems useless. l.split('+=') is not going to give you more than two elements, or the original .mk is broken, which means '+='.join(l[1:]) is just l[1]. @@ +372,5 @@ > +def update_sources_mozbuild(files, sources_mozbuild): > + f = StringIO() > + pprint(files, stream=f) > + f.seek(0) > + sources_mozbuild_new = "files = {\n %s\n}\n" % f.read().strip()[1:-1] you can use f.getvalue() instead of seek() and read() @@ +376,5 @@ > + sources_mozbuild_new = "files = {\n %s\n}\n" % f.read().strip()[1:-1] > + if sources_mozbuild != sources_mozbuild_new: > + print 'updating sources.mozbuild' > + with open('sources.mozbuild', 'w') as f: > + f.write(sources_mozbuild_new) You can use mozbuild.util.FileAvoidWrite instead of passing around the previous sources_mozbuild content. @@ +380,5 @@ > + f.write(sources_mozbuild_new) > + > +def get_current_files(): > + current_files = [] > + for root, folders, files in os.walk('.%s'%os.sep): You don't need a path separator at all here. @@ +399,5 @@ > + with open('sources.mozbuild') as f: > + sources_mozbuild = f.read() > + moz_build_files = re.compile('files = ({.*?})\n', re.DOTALL).findall(sources_mozbuild) > + if moz_build_files: > + moz_build_files = eval(moz_build_files[0]) you can just exec(sources_mozbuild), that will set files in the local scope, which you can then return.

(In reply to Mike Hommey [:glandium] from comment #19) > Comment on attachment 8343024 [details] [diff] [review] > vpx1.3.0v6_build.patch > > Review of attachment 8343024 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/libvpx/update.py > @@ +336,5 @@ > > + for l in f: > > + if '+=' in l: > > + l = l.split('+=') > > + key = l[0].strip() > > + value = '+='.join(l[1:]).strip().replace('$(ASM)', '.asm') > > '+='.join(l[1:]) seems useless. l.split('+=') is not going to give you more > than two elements, or the original .mk is broken, which means > '+='.join(l[1:]) is just l[1]. ok > @@ +372,5 @@ > > +def update_sources_mozbuild(files, sources_mozbuild): > > + f = StringIO() > > + pprint(files, stream=f) > > + f.seek(0) > > + sources_mozbuild_new = "files = {\n %s\n}\n" % f.read().strip()[1:-1] > > you can use f.getvalue() instead of seek() and read() ok > @@ +376,5 @@ > > + sources_mozbuild_new = "files = {\n %s\n}\n" % f.read().strip()[1:-1] > > + if sources_mozbuild != sources_mozbuild_new: > > + print 'updating sources.mozbuild' > > + with open('sources.mozbuild', 'w') as f: > > + f.write(sources_mozbuild_new) > > You can use mozbuild.util.FileAvoidWrite instead of passing around the > previous sources_mozbuild content. Added this to Bug 946619 > @@ +380,5 @@ > > + f.write(sources_mozbuild_new) > > + > > +def get_current_files(): > > + current_files = [] > > + for root, folders, files in os.walk('.%s'%os.sep): > > You don't need a path separator at all here. ok > @@ +399,5 @@ > > + with open('sources.mozbuild') as f: > > + sources_mozbuild = f.read() > > + moz_build_files = re.compile('files = ({.*?})\n', re.DOTALL).findall(sources_mozbuild) > > + if moz_build_files: > > + moz_build_files = eval(moz_build_files[0]) > > you can just exec(sources_mozbuild), that will set files in the local scope, > which you can then return. ok

Carrying forward r=glandium to v7 of the patch.

Folded the two patches together for landing. Carrying forward r=cpearce for media and r=glandium for build. Pushed the v5+v7 patches to try as https://tbpl.mozilla.org/?tree=Try&rev=4cb83fb09dcf