947979 - Build parts of media/libvpx in unified mode

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

Note that I wanted to make as few changes to the libvpx code as possible, so I ended up lowering the number of files per unified file to minimize the impact of name clashes in this code, which seem to be very common.  The good news however is that this is imported code so it won't change much.

https://tbpl.mozilla.org/?tree=Try&rev=f3b210375f1f

Ignore the Windows red, that's actually a green but the slave has a bad filesystem.

Last but not least, I would be happy to submit the patch upstream.  In fact I tried to do that but I'm getting an error when submitting my patch:

 git push ssh://gerrit.chromium.org:29418/webm/projectname HEAD:refs/for/master
 Permission denied (publickey).
 fatal: Could not read from remote repository.

 Please make sure you have the correct access rights
 and the repository exists.


Product: Core
Component: Video/Audio

Comment 1

[(no longer active)]

Attachment: Build parts of media/libvpx in unified mode

Comment 2

[(no longer active)]

Comment on attachment 8344710 [details] [diff] [review]
Build parts of media/libvpx in unified mode

(Finally managed to submit the upstream patch: https://gerrit.chromium.org/gerrit/#/c/68041/)

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8344710 [details] [diff] [review]
Build parts of media/libvpx in unified mode

Review of attachment 8344710 [details] [diff] [review]:
-----------------------------------------------------------------

Ralph maintains libvpx, flicking this over to him.

Comment 4

[(no longer active)]

Note that I renamed the include guard macros upstream.  If you'd like, I can change them to match what will be committed upstream before landing this.

Comment 5

[(no longer active)]

Sigh, bug 947160 ruined my work here.

Comment 6

[(no longer active)]

Attachment: Build parts of media/libvpx in unified mode; r=giles

Comment 7

[(no longer active)]

Attachment: Build parts of media/libvpx in unified mode; r=giles

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8345662 [details] [diff] [review]
Build parts of media/libvpx in unified mode; r=giles

Review of attachment 8345662 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. Sorry about the bitrot, and thanks for upstreaming your patch!

Comment 9

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc689fd8899e

Comment 10

[Jan Gerber]

update.py should be updated to write a sources.mozbuild with those changes.

Comment 11

[(no longer active)]

(In reply to comment #10)
> update.py should be updated to write a sources.mozbuild with those changes.

Hmm, can you please explain the logic of what that code is trying to do?  It seems to me like it's reading the existing sources.mozbuild file, and somehow writing a new one, but I don't understand what changes go inside the new one (sorry, my python fu is pretty weak.)

If the idea is that update.py just writes a new sources.mozbuild file with all of the new source files post the update, then we probably can't automate the full process -- we would need to move everything to UNIFIED_SOURCES and then let people to move individual files to SOURCES to fix the build.

Comment 12

[Jan Gerber]

update.py overwrites sources.mozbuild with the files collected from the libvpx makefiles.
So instead of manually editing sources.mozbuild the changes should be in update.py.
I will upload a patch shortly.

Comment 13

[Jan Gerber]

Attachment: 0001-Bug-947979-Update-update.py-to-keep-unified-mode-r-g.patch

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8346040 [details] [diff] [review]
0001-Bug-947979-Update-update.py-to-keep-unified-mode-r-g.patch

Review of attachment 8346040 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the fixup. Please address and re-submit.

::: media/libvpx/update.py
@@ +209,4 @@
>      'X86-64_ASM': [
>          'third_party/x86inc/x86inc.asm',
>          'vp8/common/x86/loopfilter_block_sse2.asm',
> +        'vp9/encoder/x86/vp9_quantize_ssse3.asm',

This is an unrealated fix, right? Separate patch please.

@@ +364,4 @@
>                      t = unknown[key]
>                  t.append(f)
>  
> +    files['UNIFIED_SOURCES'] = [f for f in files['UNIFIED_SOURCES'] if f not in files['SOURCES']]

Wouldn't it be better to check for overlap and error, instead of silently stripping duplicates?

Comment 15

[Jan Gerber]

Attachment: 0001-Handle-CodecDelay-and-SeekPreroll.patch

(In reply to Ralph Giles (:rillian) from comment #14)
> Comment on attachment 8346040 [details] [diff] [review]
> 0001-Bug-947979-Update-update.py-to-keep-unified-mode-r-g.patch
> 
> Review of attachment 8346040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the fixup. Please address and re-submit.
> 
> ::: media/libvpx/update.py
> @@ +209,4 @@
> >      'X86-64_ASM': [
> >          'third_party/x86inc/x86inc.asm',
> >          'vp8/common/x86/loopfilter_block_sse2.asm',
> > +        'vp9/encoder/x86/vp9_quantize_ssse3.asm',
> 
> This is an unrealated fix, right? Separate patch please.

yes, unrelated, will send as separate patch please.

> @@ +364,4 @@
> >                      t = unknown[key]
> >                  t.append(f)
> >  
> > +    files['UNIFIED_SOURCES'] = [f for f in files['UNIFIED_SOURCES'] if f not in files['SOURCES']]
> 
> Wouldn't it be better to check for overlap and error, instead of silently
> stripping duplicates?

UNIFIED_SOURCES contains all source files and this line removes the once we manually put into SOURCES.
This is needed since livpx modules do not match the files that can not be build in unified mode.

Comment 16

[Jan Gerber]

Attachment: 0001-Bug-947979-Update-update.py-to-keep-unified-mode-r-g.patch

this time with the right patch.

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8346115 [details] [diff] [review]
0001-Bug-947979-Update-update.py-to-keep-unified-mode-r-g.patch

Review of attachment 8346115 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libvpx/update.py
@@ -213,4 @@
>          'third_party/x86inc/x86inc.asm',
>          'vp8/common/x86/loopfilter_block_sse2.asm',
>      ],
> -    'X86_ASM': [

Technically, this should be in the other patch too.

@@ +366,4 @@
>                      t = unknown[key]
>                  t.append(f)
>  
> +    files['UNIFIED_SOURCES'] = [f for f in files['UNIFIED_SOURCES'] if f not in files['SOURCES']]

I see now. Thanks for clarifying.

Comment 18

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03ba45ce9804

Comment 19

[(no longer active)]

This part of the patch is wrong: <https://hg.mozilla.org/integration/mozilla-inbound/rev/03ba45ce9804#l1.32>.  There is nothing specific to these files which makes them not build in unified mode.  The next time that we update the code from upstream, this is probably going to be a different set of files.  This is what I meant when I said that part of the work should happen manually.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc689fd8899e
https://hg.mozilla.org/mozilla-central/rev/03ba45ce9804