Closed Bug 1223692 Opened 9 years ago Closed 7 years ago

Update libvpx to 1.6.0

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: ionnv, Assigned: johannkoenig)

References

(
URL
)

Details

Attachments

(2 files, 4 obsolete files)

0001-Bug-1223692-Update-libvpx-to-v1.6.0.patch
8.22 MB, patch
rillian
: review+
Details | Diff | Splinter Review
Bug 1223692: Update libvpx to v1.6.0.
59 bytes, text/x-review-board-request
rillian
: review+
Details 
https://groups.google.com/a/webmproject.org/forum/#!topic/codec-devel/0rjNpFerYs8

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

"2015-11-09 v1.5.0 "Javan Whistling Duck"
  This release improves upon the VP9 encoder and speeds up the encoding and
  decoding processes.

  - Upgrading:
    This release is ABI incompatible with 1.4.0. It drops deprecated VP8
    controls and adds a variety of VP9 controls for testing.

    The vpxenc utility now prefers VP9 by default.

  - Enhancements:
    Faster VP9 encoding and decoding
    Smaller library size by combining functions used by VP8 and VP9

  - Bug Fixes:
    A variety of fuzzing issues"
Depends on: 1151175
Jean-Yves, we should probably benchmark libvpx 1.5.0 before continuing work on ffvp9. If libvpx's performance is now on par with ffvp9, then we shouldn't bother switching to ffvp9.
Blocks: ffvp9
Flags: needinfo?(jyavenard)
No longer blocks: ffvp9
Jean-Yves says he has nearly completed the ffvp9 port (bug 1210219 comment 2). Anthony says we still want to keep our libvpx up to date because it is used by WebRTC (and OS X and Linux without system ffmpeg until we enable ffvp9 on those platforms).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jyavenard)
backlog: --- → webrtc/webaudio+
Rank: 25
Component: Audio/Video → WebRTC: Audio/Video
Priority: -- → P2
Depends on: 1235924
Depends on: 1235925
Depends on: 1237848
Jan, would you be interested in taking a look at this?
Flags: needinfo?(j)
FYI we're hoping to release 1.6.0 soon. There is a release candidate on the branch 'khakicampbell' if you're interested.
https://chromium.googlesource.com/webm/libvpx/+/khakicampbell
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> Jan, would you be interested in taking a look at this?

Ryan, no time to look into this right now, busy with other work.
Flags: needinfo?(j)
v1.6.0 is now available and includes the following patches from your repository:

https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/1237848-check-lookahead-ctx.patch
https://chromium-review.googlesource.com/#/c/324510/

https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/cast-char-to-uint-before-shift.patch
https://chromium-review.googlesource.com/#/c/345470/

https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/clamp-abs-QIndex.patch
https://chromium-review.googlesource.com/#/c/315802/

https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/clamp_abs_lvl_seg.patch
https://chromium-review.googlesource.com/#/c/315754/

https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/vp9_filter_restore_aligment.patch
https://chromium-review.googlesource.com/#/c/276889/

https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/vpx_once.patch
https://chromium-review.googlesource.com/#/c/312467/

https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/disable_pthread_on_mingw.patch
https://chromium-review.googlesource.com/#/c/326487/

https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/input_frame_validation.patch
https://chromium-review.googlesource.com/#/c/295437/2

It does not contain the following patches:
https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/bug1137614.patch
https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/clang-cl.patch
 (there have been some changes to intrin.h usage which may obviate this patch)
https://hg.mozilla.org/mozilla-central/file/tip/media/libvpx/stdint.patch
Summary: Update libvpx to 1.5.0 → Update libvpx to 1.6.0
I'd actually be willing to attempt this, but seeing as how update.py requires a Mac, I'm afraid I'm out. Ralph, do you have the cycles to look at this by chance?
Flags: needinfo?(giles)
I've been pretty busy, but this usually falls to me if no one else has time.

Ryan, maintaining our own build system for libvpx has been an enormous time sink and prevents us from updating in a timely fashion. Last we discussed it, gps made positive noises about calling third-party build systems for in-tree third-party code. If you want to rewrite this to have moz.build call libvpx's configure && make that would (a) not require a mac and (b) make future updates easier.

On the other hand, if you can update the import script so 1.6 builds on your machine, I'll be happy to run the script again on a mac. Collaboration! :)
Flags: needinfo?(giles) → needinfo?(ryanvm)
(In reply to Ralph Giles (:rillian) needinfo me from comment #8)
> Ryan, maintaining our own build system for libvpx has been an enormous time
> sink and prevents us from updating in a timely fashion. Last we discussed
> it, gps made positive noises about calling third-party build systems for
> in-tree third-party code. If you want to rewrite this to have moz.build call
> libvpx's configure && make that would (a) not require a mac and (b) make
> future updates easier.

I definitely have no time/expertise for rewriting libvpx' build system.

> On the other hand, if you can update the import script so 1.6 builds on your
> machine, I'll be happy to run the script again on a mac. Collaboration! :)

So manually update without the script and see what happens?
Flags: needinfo?(ryanvm)
The update process Chromium and Android use has two parts:

Chromium points to a git checkout of upstream and uses 'roll_dep.py' (part of the chromium depot_tools) to update the code followed by 'generate_gypi.sh' to create the config headers and file lists for gyp as well as gn:
https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx/README.chromium
https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/roll_dep.py
https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx/generate_gypi.sh

Android checks in all the source files and uses the old Chromium script 'update_libvpx.sh' from when it did the same thing. It then uses a modified version of 'generate_gypi.sh', renamed 'generate_config.sh', to create the config headers and file lists for Android's Makefile based system.
https://android.googlesource.com/platform/external/libvpx/+/master/README.android
https://android.googlesource.com/platform/external/libvpx/+/master/update_libvpx.sh
https://android.googlesource.com/platform/external/libvpx/+/master/generate_config.sh

I've looked at update.py a little bit but it seems to take a different approach to generating the config files. If someone familiar with the Mozilla build system wants to sit down with me, I'd be happy to try and get one of the 'generate_*' scripts working for your system.
I hacked together an update script for linux 64 and 32 bit. It works for 64 bit, untested on 32. I made it work for the existing checkout, but if you're agreeable it would be much easier for me to make it work for v1.6.0 (soon to be v1.6.1) since all the patches can be dropped.

Basic instructions:
replace update.py
replace moz.build
replace all the patches since the source code moved down a directory
run update.py --commit e67d45d4ce92468ba193288b59093fef0a502662
run generate_sources_mozbuild.sh

(and now it turns out I don't see a way to attach my work ... I'll open a new bug tomorrow)
Assignee: nobody → johannkoenig
Copy of commit message:
Bug 1223692: Update libvpx to v1.6.0

This includes the following patches:
<patch file>
<upstream review commit>
<upstream hash>

1237848-check-lookahead-ctx.patch
https://chromium-review.googlesource.com/324510
4f780e94a1fa54f22256e0f4d42a77c340a38fa1

block_error_fp.patch
https://chromium-review.googlesource.com/282611
ff8505a54d0b3dda220f5c0695519c353c82b933

cast-char-to-uint-before-shift.patch
https://chromium-review.googlesource.com/345470
2240d83d7882ce2d5d0826b9ce33b86321d7a724

clamp_abs_lvl_seg.patch
https://chromium-review.googlesource.com/315754
2e693eb80e705ea68e23eed19616d22b4778b45a

clamp-abs-QIndex.patch
https://chromium-review.googlesource.com/315802
ff3674a15e5b1a006546e1edc64c3e778eb34ab1

input_frame_validation.patch
https://chromium-review.googlesource.com/295437
c0523090be68f578c612403d8844b583dcc685ed

input_frame_validation_vp9.patch
Can't pin it down to a revision but I'm sure it's fixed.

rename_duplicate_files.patch
https://chromium-review.googlesource.com/281967
6a82f0d7fb9ee908c389e8d55444bbaed3d54e9c
https://chromium-review.googlesource.com/317880
d36659cec7fab96cedc67db4d511ed7135637d0e

vp9_filter_restore_aligment.patch
https://chromium-review.googlesource.com/276889
33b3953c548a20c0aee705657df0440a740c28b7

vpx_once.patch
https://chromium-review.googlesource.com/312467
2635573a7f2e4bbd259379acf91efb97d983359f
Attachment #8826279 - Flags: review?(giles)

        Attachment #8826279 -
        Attachment is obsolete: true

        Attachment #8826279 -
        Flags: review?(giles)

        Attachment #8826390 -
        Flags: review?(giles)

    
    

    
  


  

        Attachment #8826390 -
        Attachment is obsolete: true

        Attachment #8826390 -
        Flags: review?(giles)

        Attachment #8826409 -
        Flags: review?(giles)

    
    

    
  


  
Changes update.py to get a tarball instead of the whole git repo.
Should correctly set all the executable bits now.

        Attachment #8826409 -
        Attachment is obsolete: true

        Attachment #8826409 -
        Flags: review?(giles)

        Attachment #8826422 -
        Flags: review?(giles)

    
    

    
  
Comment on attachment 8826422 [details] [diff] [review]
0001-Bug-1223692-Update-libvpx-to-v1.6.0.patch

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

Because of the removal of vp8/common/loopfilter.c you'll need to update CLOBBER again.

Per IRC discussion, update.py will remove `libvpx.tar.gz` in the next version of the patch. That's good.

I'm not happy with "I'm sure it's fixed" for the security issues guarded by the input_frame_validation patches, and in fact I was never clear how jzern's fix on the vp8 side was equivalent. I spent some time tracing the code unsuccessfully, so in the interests of getting this landed, please either add those patches back, or provide an analysis of how it's been fixed for both vp8 and vp9.

Looks like this shouldn't conflict with the FreeBSD moz.build changes from bug 1330873, so that's good.

Otherwise looks good to me. Please update the patch and request review again.

        Attachment #8826422 -
        Flags: review?(giles) → feedback+

    
    

    
  


  
(In reply to Ralph Giles (:rillian) | needinfo me from comment #18)
> Because of the removal of vp8/common/loopfilter.c you'll need to update
> CLOBBER again.

Done.
 
> I'm not happy with "I'm sure it's fixed" for the security issues guarded by
> the input_frame_validation patches, and in fact I was never clear how
> jzern's fix on the vp8 side was equivalent. I spent some time tracing the
> code unsuccessfully, so in the interests of getting this landed, please
> either add those patches back, or provide an analysis of how it's been fixed
> for both vp8 and vp9.

The vp8 one is fixed, but left in the patch. vp9 fix is pending:
https://chromium-review.googlesource.com/428304

        Attachment #8826422 -
        Attachment is obsolete: true

        Attachment #8827024 -
        Flags: review?(giles)

    
    

    
  
Comment on attachment 8827024 [details] [diff] [review]
0001-Bug-1223692-Update-libvpx-to-v1.6.0.patch

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

Thanks. I think this is ready to land.

        Attachment #8827024 -
        Flags: review?(giles) → review+

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8827230 [details]
Bug 1223692: Update libvpx to v1.6.0.

https://reviewboard.mozilla.org/r/104974/#review105760

Shipping it for the esteemed contributor.

        Attachment #8827230 -
        Flags: review?(giles) → review+

    
    

    
  
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70a5310085fa
Update libvpx to v1.6.0. r=rillian

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/70a5310085fa
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox53:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: