Status

()

defect
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: ionnv, Assigned: j)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, firefox41 fixed)

Details

()

Attachments

(2 attachments, 5 obsolete attachments)

https://groups.google.com/a/webmproject.org/forum/#!topic/codec-devel/2zYWenmdUM8

https://code.google.com/p/webm/source/browse/CHANGELOG?repo=libvpx

"libvpx is long overdue for a release. We have made 1.4.0 available for download and in the indianrunnerduck branch. Much like the Indian Runner Duck, the theme of this release is less bugs, less bloat and more speed."
Depends on: 918550
Depends on: 1148639
QA Contact: giles
Assignee: nobody → giles
QA Contact: giles
- Update update.py to to work with libvpx 1.4
- Remove asm offsets from Makefile.in since they no longer exist
- update/drop patches
Assignee: giles → j
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8623013 - Flags: review?(giles)
update libvpx to 1.4.0 (c74bf6d889992c3cabe017ec353ca85c323107cd)
Attachment #8623015 - Flags: review?(giles)
Comment on attachment 8623013 [details] [diff] [review]
Bug1151175_update_update.py.patch

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

Thanks for taking this on. I'm surprised you can drop arm asm conversion entirely? Let me know how that works and ask for review again, please.

::: media/libvpx/Makefile.in
@@ -20,5 @@
> -
> -
> -ifdef VPX_AS_CONVERSION
> -# The ARM asm is written in ARM RVCT syntax, but we actually build it with
> -# gas using GNU syntax. Add some rules to perform the conversion.

What about the vp9 neon asm?

::: media/libvpx/msvc2015.patch
@@ +9,2 @@
>  +# if _MSC_VER < 1900
> + # define snprintf _snprintf

This should still have a two-space indent, no?
Attachment #8623013 - Flags: review?(giles)
On Mac:

 0:39.81 Undefined symbols for architecture x86_64:
 0:39.81   "_CONVERT_TO_SHORTPTR", referenced from:
 0:39.81       _vp9_highbd_get16x16var_sse2 in vp9_highbd_variance_sse2.o
 0:39.81       _vp9_highbd_10_get16x16var_sse2 in vp9_highbd_variance_sse2.o
 0:39.81       _vp9_highbd_12_get16x16var_sse2 in vp9_highbd_variance_sse2.o
 0:39.81       _vp9_highbd_get8x8var_sse2 in vp9_highbd_variance_sse2.o
 0:39.81       _vp9_highbd_10_get8x8var_sse2 in vp9_highbd_variance_sse2.o
 0:39.81       _vp9_highbd_12_get8x8var_sse2 in vp9_highbd_variance_sse2.o
 0:39.81       _vp9_highbd_variance64x64_sse2 in vp9_highbd_variance_sse2.o
 0:39.81       ...
 0:39.81   "_total_adj_strong_thresh", referenced from:
 0:39.81       _vp9_denoiser_NxM_sse2_small in vp9_denoiser_sse2.o
 0:39.81       _vp9_denoiser_NxM_sse2_big in vp9_denoiser_sse2.o
 0:39.81 ld: symbol(s) not found for architecture x86_64
 0:39.81 clang: error: linker command failed with exit code 1 (use -v to see invocation)
 0:39.81 make[6]: *** [XUL] Error 1
(In reply to Ralph Giles (:rillian) from comment #3)
> Comment on attachment 8623013 [details] [diff] [review]
> Bug1151175_update_update.py.patch
> 
> Review of attachment 8623013 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking this on. I'm surprised you can drop arm asm conversion
> entirely? Let me know how that works and ask for review again, please.
> 
> ::: media/libvpx/Makefile.in
> @@ -20,5 @@
> > -
> > -
> > -ifdef VPX_AS_CONVERSION
> > -# The ARM asm is written in ARM RVCT syntax, but we actually build it with
> > -# gas using GNU syntax. Add some rules to perform the conversion.
> 
> What about the vp9 neon asm?

ups, that was indeed to much, will add back in.


> ::: media/libvpx/msvc2015.patch
> @@ +9,2 @@
> >  +# if _MSC_VER < 1900
> > + # define snprintf _snprintf
> 
> This should still have a two-space indent, no?

ok
(In reply to Ralph Giles (:rillian) from comment #4)
> On Mac:
> 
>  0:39.81 Undefined symbols for architecture x86_64:
>  0:39.81   "_CONVERT_TO_SHORTPTR", referenced from:
>  0:39.81       _vp9_highbd_get16x16var_sse2 in vp9_highbd_variance_sse2.o
>  0:39.81       _vp9_highbd_10_get16x16var_sse2 in vp9_highbd_variance_sse2.o
>  0:39.81       _vp9_highbd_12_get16x16var_sse2 in vp9_highbd_variance_sse2.o
>  0:39.81       _vp9_highbd_get8x8var_sse2 in vp9_highbd_variance_sse2.o
>  0:39.81       _vp9_highbd_10_get8x8var_sse2 in vp9_highbd_variance_sse2.o
>  0:39.81       _vp9_highbd_12_get8x8var_sse2 in vp9_highbd_variance_sse2.o
>  0:39.81       _vp9_highbd_variance64x64_sse2 in vp9_highbd_variance_sse2.o
>  0:39.81       ...
>  0:39.81   "_total_adj_strong_thresh", referenced from:
>  0:39.81       _vp9_denoiser_NxM_sse2_small in vp9_denoiser_sse2.o
>  0:39.81       _vp9_denoiser_NxM_sse2_big in vp9_denoiser_sse2.o
>  0:39.81 ld: symbol(s) not found for architecture x86_64
>  0:39.81 clang: error: linker command failed with exit code 1 (use -v to see
> invocation)
>  0:39.81 make[6]: *** [XUL] Error 1

fixed my universal build setup and fixed 32bit build.
Attachment #8623013 - Attachment is obsolete: true
Attachment #8623599 - Flags: review?(giles)
Attachment #8623015 - Attachment is obsolete: true
Attachment #8623015 - Flags: review?(giles)
Attachment #8623600 - Flags: review?(giles)
replace with hg version of patch
Attachment #8623599 - Attachment is obsolete: true
Attachment #8623599 - Flags: review?(giles)
Attachment #8623601 - Flags: review?(giles)
Comment on attachment 8623601 [details] [diff] [review]
Bug1151175_update_update.py.patch

Thanks, changes look good. Still some issues:

undefined reference to `CONVERT_TO_SHORTPTR' on linux64
undefined reference to `total_adj_strong_thresh' on linux
moz.build parse errors on windows
multiple definitions of vp9 neon asm functions on android

See build reports at https://treeherder.mozilla.org/#/jobs?repo=try&revision=56db250ea91a
Attachment #8623601 - Flags: review?(giles) → review-
yes found 2 other issue
- with neon if both intrinsics and asm exist (now only using asm)
- and some unused options (VP9_TEMPORAL_DENOISING and VP9_HIGHBITDEPTH)

waiting for my local windows build and will post an updated patch once that is done.
new patch fixing some more issue:

- with neon if both intrinsics and asm exist (now only using asm)
- and some unused options (VP9_TEMPORAL_DENOISING and VP9_HIGHBITDEPTH)

now tested build on
- linux x86_64
- osx x86_64 / i386
- osx cross compile android

compiles on win32 (VC2013) but my vm runs out of memory to link. might be faster to test on tryserver.
Attachment #8623601 - Attachment is obsolete: true
Attachment #8623911 - Flags: review?(giles)
and the corresponding libvpx patch
Attachment #8623600 - Attachment is obsolete: true
Attachment #8623600 - Flags: review?(giles)
Attachment #8623912 - Flags: review?(giles)
try is closed. I'll push again when it opens.
Comment on attachment 8623911 [details] [diff] [review]
Bug1151175_update_update.py.patch

This one looks good, except I think we need to clobber the build to work around mach not handling vp9_thread.c moving locations. I've pushed again to verify. If that makes it green, we should be ready to land with the clobber change folded in.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6048116a5b0b
Attachment #8623911 - Flags: review?(giles) → review+
Comment on attachment 8623912 [details] [diff] [review]
Bug1151175_update_libvpx.patch

r=me with CLOBBER updated and a green try push.
Attachment #8623912 - Flags: review?(giles) → review+
Try looks green.
https://hg.mozilla.org/mozilla-central/rev/13d883caaf74
https://hg.mozilla.org/mozilla-central/rev/de1ddaec57b7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1176730
Comment on attachment 8623912 [details] [diff] [review]
Bug1151175_update_libvpx.patch

Requesting aurora uplift for both patches on this bug. This gets the new, stable upstream code to users sooner, and we've had one report of a rendering regression fixed by this update in bug 1175220.

Approval Request Comment
[Feature/regressing bug #]: 1148639
[User impact if declined]: Rendering issues, likely security issues, with webm video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, green on media tests.
[Risks and why]: Risk is low. We're moving from a snapshot to a released version of a third-party library which has been out for a couple of months.
[String/UUID change made/needed]: None.
Attachment #8623912 - Flags: approval-mozilla-aurora?
Comment on attachment 8623912 [details] [diff] [review]
Bug1151175_update_libvpx.patch

Landed a few days ago in m-c without obvious regressions. As a Linux packager, I am familiar with update of libraries and I am not concerned about that. taking it.
Attachment #8623912 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1223692
Blocks: 963460
You need to log in before you can comment on or make changes to this bug.