1331498 - Update libvpx to 1.6.1

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Ryan VanderMeulen [:RyanVM]]

+++ 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.

Comment 1

[johannkoenig]

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?

Comment 2

[Ralph Giles (:rillian)]

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

Comment 3

[johannkoenig]

Attachment: 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

Comment 4

[Ralph Giles (:rillian)]

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

[Ralph Giles (:rillian)]

Attachment: Bug 1331498: Update libvpx to v1.6.1.

Removes armv6 and mmx optimizations.

Removes x86inc.asm distinction. This was put in place because of issues
with 32bit PIC builds but x86inc.asm has since been improved to support
those.

Switch to '.S' for arm assembly. There is now an #include for a helper
file. All build systems appear to support .S so switch to it for
consistency.

Remove clang-cl.patch. There have been numerous changes upstream to
improve clang support.

Review commit: https://reviewboard.mozilla.org/r/105228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105228/

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8827535 [details]
0001-Bug-1331498-Update-libvpx-to-v1.6.1.patch.bz2

Moving to reviewboard.

Comment 7

[Ralph Giles (:rillian)]

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.

Comment 8

[Ralph Giles (:rillian)]

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
```

Comment 9

[johannkoenig]

(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

Comment 10

[johannkoenig]

Attachment: 0001-Bug-1331498-Update-libvpx-to-v1.6.1.patch.bz2

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8827599 [details]
Bug 1331498: Update libvpx to v1.6.1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105228/diff/1-2/

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8827616 [details]
0001-Bug-1331498-Update-libvpx-to-v1.6.1.patch.bz2

Pushed to reviewboard.

Comment 13

[Ralph Giles (:rillian)]

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.

Comment 14

[Ralph Giles (:rillian)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58764883978d
Update libvpx to v1.6.1. r=rillian

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58764883978d