Build parts of media/libvpx in unified mode

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 8344710 [details] [diff] [review]
Build parts of media/libvpx in unified mode
(Assignee)

Comment 2

5 years ago
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/)
Attachment #8344710 - Flags: review?(cpearce)
(Assignee)

Updated

5 years ago
Assignee: nobody → ehsan
Blocks: 939583
Component: General → Video/Audio
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.
Attachment #8344710 - Flags: review?(cpearce) → review?(giles)
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Sigh, bug 947160 ruined my work here.
(Assignee)

Updated

5 years ago
Attachment #8344710 - Attachment is obsolete: true
Attachment #8344710 - Flags: review?(giles)
(Assignee)

Comment 6

5 years ago
Created attachment 8345658 [details] [diff] [review]
Build parts of media/libvpx in unified mode; r=giles
(Assignee)

Updated

5 years ago
Attachment #8345658 - Flags: review?(giles)
(Assignee)

Comment 7

5 years ago
Created attachment 8345662 [details] [diff] [review]
Build parts of media/libvpx in unified mode; r=giles
(Assignee)

Updated

5 years ago
Attachment #8345658 - Attachment is obsolete: true
Attachment #8345658 - Flags: review?(giles)
(Assignee)

Updated

5 years ago
Attachment #8345662 - Flags: review?(giles)
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!
Attachment #8345662 - Flags: review?(giles) → review+

Comment 10

5 years ago
update.py should be updated to write a sources.mozbuild with those changes.
(Assignee)

Comment 11

5 years ago
(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

5 years ago
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

5 years ago
Created attachment 8346040 [details] [diff] [review]
0001-Bug-947979-Update-update.py-to-keep-unified-mode-r-g.patch
Attachment #8346040 - Flags: review?(giles)
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?
Attachment #8346040 - Flags: review?(giles) → review-

Comment 15

5 years ago
Created attachment 8346109 [details] [diff] [review]
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.
Attachment #8346040 - Attachment is obsolete: true
Attachment #8346109 - Flags: review?(giles)

Comment 16

5 years ago
Created attachment 8346115 [details] [diff] [review]
0001-Bug-947979-Update-update.py-to-keep-unified-mode-r-g.patch

this time with the right patch.
Attachment #8346109 - Attachment is obsolete: true
Attachment #8346109 - Flags: review?(giles)
Attachment #8346115 - Flags: review?(giles)
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.
Attachment #8346115 - Flags: review?(giles) → review+
(Assignee)

Comment 19

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/fc689fd8899e
https://hg.mozilla.org/mozilla-central/rev/03ba45ce9804
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.