Update libvpx to a version 1.3.0

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: rillian, Assigned: j)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 10 obsolete attachments)

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)
Attachment #8337715 - Attachment is obsolete: true
Attachment #8340371 - Flags: review?(giles)
Posted patch vpx-1.3.0v3_build.patch (obsolete) — Splinter Review
also build C asm files on android
Attachment #8340371 - Attachment is obsolete: true
Attachment #8340371 - Flags: review?(giles)
Attachment #8341008 - Flags: review?(mh+mozilla)
Posted patch vpx-1.3.0v4.build.patch (obsolete) — Splinter Review
rebase patch on top of
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0144998bab0
Attachment #8341008 - Attachment is obsolete: true
Attachment #8341008 - Flags: review?(mh+mozilla)
Attachment #8341324 - Flags: review?(mh+mozilla)
Update patch to git tag v1.3.0
Attachment #8337716 - Attachment is obsolete: true
Attachment #8341364 - Flags: review?(cpearce)
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?
Attachment #8341364 - Flags: review?(cpearce) → review+
(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
Summary: Update libvpx to a version supporting vp9 → Update libvpx to a version 1.3.0
Posted patch vpx1.3.0v5_libvpx.patch (obsolete) — Splinter Review
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
Attachment #8341364 - Attachment is obsolete: true
Posted patch vpx1.3.0v5_build.patch (obsolete) — Splinter Review
update build part of patch:
 - 32bit osx needs --enable-pic
 - manually disable AVX2 on Linux since
   mozilla build setup does not support it currently
Attachment #8341324 - Attachment is obsolete: true
Attachment #8341324 - Flags: review?(mh+mozilla)
Attachment #8342317 - Flags: review?(mh+mozilla)
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.
Attachment #8342317 - Flags: review?(mh+mozilla) → feedback+
Blocks: 946619
Blocks: 946621
Blocks: 946639
Posted patch vpx1.3.0v6_build.patch (obsolete) — Splinter Review
(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
Attachment #8342317 - Attachment is obsolete: true
Attachment #8343024 - Flags: review?(mh+mozilla)
(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.
Attachment #8343024 - Flags: review?(mh+mozilla) → review+
(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
Posted patch vpx-1.3.0v7_build.patch (obsolete) — Splinter Review
Carrying forward r=glandium to v7 of the patch.
Attachment #8343024 - Attachment is obsolete: true
Blocks: 947160
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
Attachment #8342316 - Attachment is obsolete: true
Attachment #8343674 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f4f8faa3771c
Assignee: nobody → j
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 952048
Whiteboard: [qa-]
Depends on: CVE-2014-1578
Blocks: 1151175
You need to log in before you can comment on or make changes to this bug.