Update libvpx to 1.6.1

RESOLVED FIXED in Firefox 53

Status

()

Core
WebRTC: Audio/Video
P2
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: RyanVM, Assigned: johannkoenig)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 months ago
+++ This bug was initially created as a clone of Bug #1223692 +++

https://groups.google.com/a/webmproject.org/d/msg/codec-devel/Vt6c2Z96cWI/aTYeP5y6BwAJ

https://chromium.googlesource.com/webm/libvpx/+/v1.6.1

2017-01-09 v1.6.1 "Long Tailed Duck"
  This release improves upon the VP9 encoder and speeds up the encoding and
  decoding processes.

  - Upgrading:
    This release is ABI compatible with 1.6.0.

  - Enhancements:
    Faster VP9 encoding and decoding.
    High bit depth builds now provide similar speed for 8 bit encode and decode
    for x86 targets. Other platforms and higher bit depth improvements are in
    progress.

  - Bug Fixes:
    A variety of fuzzing issues.
(Assignee)

Updated

3 months ago
Assignee: nobody → johannkoenig
(Assignee)

Comment 1

3 months ago
I've run into a bit of an issue. Upstream created a "header" file for the neon assembly for some common macros. Unfortunately it still needs to be run through ads2gas.pl

I see two ways to fix this:
1) pre-run it through ads2gas.pl because FF only supports *nix builds so I don't need to care about whether to use ads2gas.pl or ads2gas_apple.pl

2) Add the generated assembly directory to ASFLAGS with -I but I don't know what variable to use for that directory. The file in question is obj-arm-linux-androideabi/media/libvpx/libvpx/vpx_dsp/arm/idct_neon.asm.S and is include with the full path, so I need something like ASFLAGS += -Iobj-arm-linux-androideabi/media/libvpx/libvpx/

Preference for either approach? Suggested moz.build variable for the include path?
I think running the generator sounds safer.

You can add paths to the build with the `LOCAL_INCLUDES` variable (a list) in moz.build. You can use `OBJDIR` to reference the target directory for generated files. https://gecko.readthedocs.io/en/latest/build/buildsystem/mozbuild-symbols.html#objdir
(Assignee)

Comment 3

3 months ago
Created attachment 8827535 [details]
0001-Bug-1331498-Update-libvpx-to-v1.6.1.patch.bz2

Apologies for the bz2. The patch went over the 10mb limit.

(In reply to Ralph Giles (:rillian) | needinfo me from comment #2)
> You can add paths to the build with the `LOCAL_INCLUDES` variable (a list)

Does that include ASFLAGS? It didn't appear to from the documentation. If it does (or even just for CFLAGS) there are a number of places in media/libvpx/moz.build that should probably use it. Unfortunately I don't really have a good setup for experimenting and testing different build rules.

> in moz.build. You can use `OBJDIR` to reference the target directory for

Thanks. I was able to use OBJDIR
Attachment #8827535 - Flags: review?(giles)
(In reply to johannkoenig from comment #3)

> > You can add paths to the build with the `LOCAL_INCLUDES` variable (a list)
> 
> Does that include ASFLAGS?

Looks like it gets appended after ASFLAGS. Of course you can use ASFLAGS if you you want the include to be specific to the assembler. https://dxr.mozilla.org/mozilla-central/source/config/rules.mk#983

> Thanks. I was able to use OBJDIR

Great.
Comment hidden (mozreview-request)
Comment on attachment 8827535 [details]
0001-Bug-1331498-Update-libvpx-to-v1.6.1.patch.bz2

Moving to reviewboard.
Attachment #8827535 - Attachment is obsolete: true
Attachment #8827535 - Flags: review?(giles)

Comment 7

3 months ago
mozreview-review
Comment on attachment 8827599 [details]
Bug 1331498: Update libvpx to v1.6.1.

https://reviewboard.mozilla.org/r/105228/#review106044

::: media/libvpx/rename_duplicate_files.patch:4
(Diff revision 1)
>  diff --git a/libvpx/vpx_dsp/x86/loopfilter_sse2.c b/libvpx/vpx_dsp/x86/loopfilter_intrin_sse2.c
>  similarity index 100%
>  rename from libvpx/vpx_dsp/x86/loopfilter_sse2.c
>  rename to libvpx/vpx_dsp/x86/loopfilter_intrin_sse2.c

When I applied your 1.6.0 patch this worked, but today when I try running update.py on my mac with this patch, this rename doesn't happen, and this part of the diff ends up reverted.

I guess Apple's `patch` doesn't implement the rename syntax. Not sure how best to address this: vcs moves are good, but it's bad that the script doesn't work on Mac. Maybe the best thing is to replace this section of the patch with an explicit `os.system('mv ..')` in update.py?

::: media/libvpx/update.py:49
(Diff revision 1)
>  def update_readme(commit):
>      with open('README_MOZILLA') as f:
>          readme = f.read()
>  
>      if 'The git commit ID used was' in readme:
>          new_readme = re.sub('The git commit ID used was [a-f0-9]+',

Sorry, I didn't catch this in the previous commit, but the regex no longer works now that we have release tags we can update against. (This is a good thing!)

Can you change this and re-run the update so README_MOZILLA gets updated properly? Just `[v\.a-f0-9]+` would be sufficient for the release tags along with commit hashes. Or a general 'any string without whitespace' is probably fine too.
Attachment #8827599 - Flags: review?(giles)

Comment 8

3 months ago
mozreview-review
Comment on attachment 8827599 [details]
Bug 1331498: Update libvpx to v1.6.1.

https://reviewboard.mozilla.org/r/105228/#review106052

Looks good in general but needs a few fixes. Please address issues and update the patch.

::: media/libvpx/moz.build:50
(Diff revision 1)
>      arm_asm_files = files['ARM_SOURCES']
>  
>      if CONFIG['VPX_AS_CONVERSION']:
>          SOURCES += sorted([
> -            "!%s.s" % f if f.endswith('.asm') else f for f in arm_asm_files
> +            "!%s.S" % f if f.endswith('.asm') else f for f in arm_asm_files
>          ])

This seems to be failing in the arm builds. Maybe looking in the source instead of the build directory for the translated file?
```
/usr/bin/perl /builds/slave/try-and-api-15-000000000000000/build/src/media/libvpx/libvpx/build/make/ads2gas.pl < /builds/slave/try-and-api-15-000000000000000/build/src/media/libvpx/libvpx/vpx_dsp/arm/idct16x16_1_add_neon.asm > libvpx/vpx_dsp/arm/idct16x16_1_add_neon.asm.S
/bin/sh: libvpx/vpx_dsp/arm/idct16x16_1_add_neon.asm.S: No such file or directory
gmake[5]: *** [libvpx/vpx_dsp/arm/idct16x16_1_add_neon.asm.S] Error 1
```
(Assignee)

Comment 9

3 months ago
(In reply to Ralph Giles (:rillian) | needinfo me from comment #7)
> >  rename from libvpx/vpx_dsp/x86/loopfilter_sse2.c
> >  rename to libvpx/vpx_dsp/x86/loopfilter_intrin_sse2.c
> 
> I guess Apple's `patch` doesn't implement the rename syntax.

Switched to os.system("mv ...")

> Sorry, I didn't catch this in the previous commit, but the regex no longer
> works now that we have release tags we can update against. (This is a good
> thing!)

Huh, I guess I updated it manually. Fixed.
 
(In reply to Ralph Giles (:rillian) | needinfo me from comment #8)
> This seems to be failing in the arm builds. Maybe looking in the source

in media/libvpx/Makefile.in:
GENERATED_DIRS += $(dir $(ASFILES))

.S files are not added to ASFILES. This doesn't appear to affect the build in any other way. Forced the directory creation with:
GENERATED_DIRS += libvpx/vpx_dsp/arm
(Assignee)

Comment 10

3 months ago
Created attachment 8827616 [details]
0001-Bug-1331498-Update-libvpx-to-v1.6.1.patch.bz2
Attachment #8827599 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8827616 - Flags: review?(giles)
Comment hidden (mozreview-request)
Comment on attachment 8827616 [details]
0001-Bug-1331498-Update-libvpx-to-v1.6.1.patch.bz2

Pushed to reviewboard.
Attachment #8827616 - Attachment is obsolete: true
Attachment #8827616 - Flags: review?(giles)

Comment 13

3 months ago
mozreview-review
Comment on attachment 8827599 [details]
Bug 1331498: Update libvpx to v1.6.1.

https://reviewboard.mozilla.org/r/105228/#review106082

lgtm. r+ assuming the android try push is green.
Attachment #8827599 - Flags: review?(giles) → review+

Comment 14

3 months ago
mozreview-review-reply
Comment on attachment 8827599 [details]
Bug 1331498: Update libvpx to v1.6.1.

https://reviewboard.mozilla.org/r/105228/#review106052

> This seems to be failing in the arm builds. Maybe looking in the source instead of the build directory for the translated file?
> ```
> /usr/bin/perl /builds/slave/try-and-api-15-000000000000000/build/src/media/libvpx/libvpx/build/make/ads2gas.pl < /builds/slave/try-and-api-15-000000000000000/build/src/media/libvpx/libvpx/vpx_dsp/arm/idct16x16_1_add_neon.asm > libvpx/vpx_dsp/arm/idct16x16_1_add_neon.asm.S
> /bin/sh: libvpx/vpx_dsp/arm/idct16x16_1_add_neon.asm.S: No such file or directory
> gmake[5]: *** [libvpx/vpx_dsp/arm/idct16x16_1_add_neon.asm.S] Error 1
> ```

This is fixed with the `GENERATED_DIRS` change.

Comment 15

3 months ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58764883978d
Update libvpx to v1.6.1. r=rillian
(Reporter)

Comment 16

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58764883978d
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

3 months ago
Blocks: 1332139
Depends on: 1337250
Depends on: 1344889
You need to log in before you can comment on or make changes to this bug.