Update libvpx to 1.2.0

RESOLVED FIXED in Firefox 28

Status

()

Core
Audio/Video
--
enhancement
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: MrX1980, Assigned: rillian)

Tracking

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed)

Details

(Whiteboard: [qa-], URL)

Attachments

(1 attachment, 16 obsolete attachments)

2.78 MB, patch
rillian
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
A new version of libvpx 1.1.0 "Eider" is available (2012-05-10).

"In addition to a number of enhancements, this release fixes a decoder bug first introduced in v1.0.0, "Duclair," so all users of that release are encouraged to upgrade."
(In reply to MrX1980 from comment #0)
> A new version of libvpx 1.1.0 "Eider" is available (2012-05-10).

This is on my TODO list, but...

> "In addition to a number of enhancements, this release fixes a decoder bug
> first introduced in v1.0.0, "Duclair," so all users of that release are
> encouraged to upgrade."

...this is fixed in local patch media/libvpx/I9713c9f0.patch that we applied on top of the Duclair release (see media/libvpx/update.sh for a complete list of the patches we're carrying).
(Reporter)

Comment 2

5 years ago
http://code.google.com/p/webm/source/browse/CHANGELOG?repo=libvpx
http://git.chromium.org/gitweb/?p=webm/libvpx.git;a=commitdiff;h=b9ce43029298182668d4dcb0e0814189e4a63c2a

2012-12-21 v1.2.0
This release acts as a checkpoint for a large amount of internal refactoring
and testing. It also contains a number of small bugfixes, so all users are
encouraged to upgrade.

- Upgrading:
This release is ABI and API compatible with Duclair (v1.0.0). Users
of older releases should refer to the Upgrading notes in this
document for that release.

- Enhancements:
VP8 optimizations for MIPS dspr2
vpxenc: add -quiet option

- Speed:
Encoder and decoder speed is consistent with the Eider release.

- Quality:
In general, quality is consistent with the Eider release.

Minor tweaks to ARNR filtering
Minor improvements to real time encoding with multiple temporal layers

- Bug Fixes:
Fixes multithreaded encoder race condition in loopfilter
Fixes multi-resolution threaded encoding
Fix potential encoder dead-lock after picture resize
Summary: Update libvpx to 1.1.0 "Eider" → Update libvpx to 1.2.0

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

4 years ago
http://blog.webmproject.org/2013/05/vp9-codec-nears-completion.html


"We’ll freeze the VP9 bitstream on June 17, allowing Chrome and Chrome OS to enable VP9 by default."

I don't think Mozilla should addto use the VP9 encoder
VP9 shows promising results for video encoding and the quality improvements can be adapted to WebP.

Comment 4

4 years ago
VP9 bitstream finalized but can it be implemented in FF24ESR??

For anyone not following video standards, VP9 is the "open" successor to VP8, and a competitor of HEVC, which is the successor to h264.

VP9 promises the same video quality as VP8 with a 50% smaller file size.

HEVC isnt yet finalized, but currently performs 7 to 41% better than VP9 (ie. file sizes are 7 to 41% smaller for equal quality, depending whos stats you look at), but will probably require the payment of license/patent royalties.

Basicly its gonna boil down to "I want higher quality down my broadband connection" vs. "I want FOSS video decoding".

Online video providers will probably choose HEVC since saving 41% on their bandwidth bill is a massive amount of money, probably less than the royalties they would need to pay for the encoders.

https://groups.google.com/a/webmproject.org/forum/#!msg/webm-discuss/UzoX7owhwB0/XVIZVDG9rm0J

Comment 5

4 years ago
Google has just enabled in chromium - VP9 by default


https://code.google.com/p/chromium/issues/detail?id=180280
Blocks: 833023

Updated

4 years ago
Assignee: nobody → joshmoz

Updated

4 years ago
Assignee: joshmoz → nobody
(Assignee)

Comment 6

4 years ago
I'd want to land the 1.2.0 has a reference point before updating to newer git for vp9 support.
Assignee: nobody → giles
(Assignee)

Updated

4 years ago
Blocks: 918550
(Assignee)

Comment 7

4 years ago
Created attachment 830601 [details] [diff] [review]
Build libvpx externally WIP v1

Our old build system has bit-rotted. We can update, but I saw a comment from gps in another bug recommending EXTERNAL_MAKE_DIRS, which seems like a better idea for upstream libraries like this.

Patch calls the libvpx configure to generate some of the target-specific source, then runs its makefile to generate the rest and build a library.

The patch assumes there's a libvpx 1.2.0 checkout in media/libvpx/src because I got a loop with 'EXTERNAL_MAKE_DIRS += ['.']'. Would be nice to fix that so the moz.build can just say to run 'make' in the current directory, like we do with the hacked up Makefile.in. We have a hacked-up Makefile.in because just using EXTERNAL_MAKE_DIRS was invoking 'make export' on the upstream makefile.

Patch combines work by myself and j^.
Attachment #830601 - Flags: feedback?(ted)

Comment 8

4 years ago
Created attachment 830925 [details] [diff] [review]
Update libvpx to 1.2.0 WIP v2

Improved version of the patch:
 - works on 64bit linux
 - sets target based on build target
 - only build encoder if encoder is enabled

This patch removes the old libvpx code and pulls in libvpx 1.2.0
Attachment #830925 - Flags: feedback?
(Assignee)

Updated

4 years ago
Attachment #830601 - Attachment is obsolete: true
Attachment #830601 - Flags: feedback?(ted)

Updated

4 years ago
Attachment #830925 - Flags: feedback? → feedback?(ted)
(Assignee)

Updated

4 years ago
Attachment #830925 - Attachment filename: mozilla-central-vpx-1.2.0.patch → Update
(Assignee)

Updated

4 years ago
Attachment #830925 - Attachment description: mozilla-central-vpx-1.2.0.patch → Update libvpx to 1.2.0 WIP v2
Attachment #830925 - Attachment filename: Update → mozilla-central-vpx-1.2.0.patch
(Assignee)

Comment 9

4 years ago
Created attachment 831074 [details]
Mac decode corruption

I get playback corruption with this patch on MacOS. J cannot reproduce on linux.

This is https://people.mozilla.org/~rgiles/2013/demo.html
Test windows and android builds: https://tbpl.mozilla.org/?tree=Try&rev=be0b75716fdc
The Windows builds failed because pymake doesn't understand the libvpx makefile. Mozmake is ok. I'm trying to get it to work with $(GMAKE) replacing pymake like the gaia Makefile.in does.

Android builds fail because the gyp files don't generate an appropriate include lines to use the srcdir copy. We probably still need an export step, but I'm not clear why that isn't a problem for the desktop builds.

I talked to glandium a bit on irc, who was not in favour of invoking the upstream build system. The most persuasive argument was that it would break PGO, but we apparently don't have video playback in the profile anyway. We should look at setting the proper flags for LTO though.
(In reply to Jan Gerber from comment #8)
> This patch removes the old libvpx code and pulls in libvpx 1.2.0

Note that mercurial is not smart and besides losing history, this also means making the repository larger than it needs to be.
Real history is available upstream. I agree it would be better to keep the source directly in media/libvpx though. I only moved it down because I thought EXTERNAL_MAKE_DIRS would work for us here.
(In reply to Ralph Giles (:rillian) from comment #13)
> Real history is available upstream. I agree it would be better to keep the
> source directly in media/libvpx though. I only moved it down because I
> thought EXTERNAL_MAKE_DIRS would work for us here.

Moving the source under media/libvpx/src is not a problem. It would be nicer to hg mv the source, instead of hg rming the old and hg adding the new.
Which, by the way, hg addremove can do for you. It even does file renaming guesses with a similarity percentage.
Created attachment 831429 [details] [diff] [review]
Update libvpx to 1.2.0 WIP v3

Updated patch addressing some of the build issues on Windows and Android.
Attachment #830925 - Attachment is obsolete: true
Attachment #830925 - Flags: feedback?(ted)
Attachment #831429 - Flags: feedback?(ted)
Created attachment 832110 [details] [diff] [review]
Update libvpx to 1.2.0 WIP (moz.build) v1

Alternate approach, updating update.sh and build files to use libvpx source with the mozilla build system. Tested on MacOS, some compile issues on Android.

Moved some but not all of the source lists from Makefile.in to moz.build.

Based in part of work by j^.
Created attachment 832646 [details] [diff] [review]
Update libvpx to 1.2.0 WIP (moz.build) v2

J and I got this working on Android.
Attachment #832110 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=a9246091c0f8
Created attachment 8333605 [details] [diff] [review]
Update libvpx to 1.2.0 WIP (moz.build) v3

More updates from Jan. Fixes 32-bit build, update-update.sh.py script.
Attachment #832646 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=fcde1bfab5fa
Created attachment 8334040 [details] [diff] [review]
Update libvpx to 1.2.0 WIP (moz.build) v4

Updated patch; 32 bit and Windows fixes.
Attachment #8333605 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=f8d3c97b2e17
Comment on attachment 8334040 [details] [diff] [review]
Update libvpx to 1.2.0 WIP (moz.build) v4

Green on try. Passing to Chris for media module review.
Attachment #8334040 - Flags: review?(cpearce)
Comment on attachment 8334040 [details] [diff] [review]
Update libvpx to 1.2.0 WIP (moz.build) v4

And gladium for build peer review.
Attachment #8334040 - Flags: review?(mh+mozilla)
Comment on attachment 831429 [details] [diff] [review]
Update libvpx to 1.2.0 WIP v3

We've decided not to do try invoking the upstream build system.
Attachment #831429 - Flags: feedback?(ted)
(Assignee)

Updated

4 years ago
Attachment #831074 - Attachment is obsolete: true
Comment on attachment 8334040 [details] [diff] [review]
Update libvpx to 1.2.0 WIP (moz.build) v4

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

::: media/libvpx/update.sh
@@ +360,5 @@
>  
>  # This has to be renamed because there's already a scalesystemdependent.c in
>  # vpx_scale/generic/
> +#cp -v $1/vpx_scale/arm/scalesystemdependent.c \
> +#         vpx_scale/arm/arm_scalesystemdependent.c

:(

@@ +365,3 @@
>  
>  # Patch to use VARIANCE_INVOKE in multiframe_quality_enhance_block().
> +#patch -p3 < variance-invoke.patch

:( ?
Attachment #8334040 - Flags: review?(cpearce) → review+
Created attachment 8334340 [details] [diff] [review]
Update libvpx to 1.2.0 (moz.build) v5

Update patch to remove commented-out lines from cpearce's review. Talked to derf on irc about the variance-invoke patch.

Rebased against glandium's moz.build changes on m-c.
Attachment #831429 - Attachment is obsolete: true
Attachment #8334040 - Attachment is obsolete: true
Attachment #8334040 - Flags: review?(mh+mozilla)
Attachment #8334340 - Flags: review?(mh+mozilla)
Carrying forward r=cpearce to v5 of the patch.
Created attachment 8334358 [details] [diff] [review]
Update libvpx source to 1.2.0.

Same patch, but with a little hg addremove help, it marks 6 files as copy/renames.
Attachment #8334358 - Flags: review?(mh+mozilla)
Attachment #8334340 - Attachment is obsolete: true
Attachment #8334340 - Flags: review?(mh+mozilla)
Assignee: giles → mh+mozilla
Status: NEW → ASSIGNED
Assignee: mh+mozilla → giles
Comment on attachment 8334358 [details] [diff] [review]
Update libvpx source to 1.2.0.

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

I haven't looked at the update script yet, but i'd like answers to the questions below (and the remarks addressed)

::: media/libvpx/Makefile.in
@@ +83,2 @@
>  
>  ASFILES += \

So, you moved C sources. Why not assembly sources too?

@@ +165,1 @@
>    yv12extend_arm.c \

Why are some files left around?

@@ +303,5 @@
>  endif
>  
>  else
>  
> +idct_blk_sse2.o: CFLAGS += -msse2

$(OBJ_SUFFIX)

::: media/libvpx/moz.build
@@ +79,5 @@
>      'vpx_scale/generic/yv12config.c',
>      'vpx_scale/generic/yv12extend.c',
>  ]
>  
> +if CONFIG['MOZ_VP8_ENCODER']: SOURCES += [

new line

@@ +105,5 @@
> +    'vp8/encoder/treewriter.c',
> +    'vp8/vp8_cx_iface.c',
> +]
> +#postproc is only enabled on x86 with asm
> +if CONFIG['VPX_X86_ASM']: SOURCES += [

new line

@@ +111,5 @@
> +    'vp8/common/postproc.c',
> +    'vp8/encoder/temporal_filter.c',
> +]
> +
> +if CONFIG['VPX_X86_ASM']: SOURCES += [

new line

@@ +125,5 @@
> +    'vp8/common/x86/vp8_asm_stubs.c',
> +    'vpx_ports/x86_cpuid.c',
> +]
> +
> +if CONFIG['VPX_X86_ASM'] and CONFIG['MOZ_VP8_ENCODER']: SOURCES += [

new line

@@ +134,5 @@
> +if CONFIG['MOZ_VP8_ENCODER']:
> +    SOURCES += [
> +        'vp8/encoder/denoising.c',
> +    ]
> +    if CONFIG['VPX_X86_ASM']: SOURCES += [

new line

@@ +138,5 @@
> +    if CONFIG['VPX_X86_ASM']: SOURCES += [
> +        'vp8/encoder/x86/denoising_sse2.c',
> +    ]
> +
> +if CONFIG['VPX_ARM_ASM']: SOURCES += [

new line

@@ +150,5 @@
> +    'vp8/common/arm/variance_arm.c',
> +    'vpx_ports/arm_cpudetect.c',
> +]
> +
> +if CONFIG['VPX_ARM_ASM'] and CONFIG['MOZ_VP8_ENCODER']: SOURCES += [

new line
Attachment #8334358 - Flags: review?(mh+mozilla)

Comment 32

4 years ago
Created attachment 8334640 [details] [diff] [review]
libvpx to 1.2.0 (moz.build) v6.patch

new version of the patch addressing the review suggestions:
 - adding more newlines to moz.build
 - using $(OBJ_SUFFIX)
 - moving all c and asm files to moz.build and adjusting Makefile.in accordingly
Created attachment 8335025 [details] [diff] [review]
Update libvpx to 1.2.0 (moz.build) v7

Apply fixes from j^'s v6 patch on top of glandium's port to hg. Carrying forward r=cpearce.


Glandium asked:

> So, you moved C sources. Why not assembly sources too?

No good reason. The C sources were already split between the files. They're moved now.

>>    yv12extend_arm.c \
>
> Why are some files left around?

It's an assembly file. :)

>> +idct_blk_sse2.o: CFLAGS += -msse2
>
> $(OBJ_SUFFIX)

Fixed here. We left filter.o in the SunOS compiler bug workaround.

> new line

Re-indented.
Attachment #8334358 - Attachment is obsolete: true
Attachment #8334640 - Attachment is obsolete: true
Attachment #8335025 - Flags: review?(mh+mozilla)
https://tbpl.mozilla.org/?tree=Try&rev=838abb292533
Comment on attachment 8335025 [details] [diff] [review]
Update libvpx to 1.2.0 (moz.build) v7

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

Would you mind separating the libvpx code import itself with the build system and update script changes for next iteration? Will make reviewing much easier.

::: media/libvpx/Makefile.in
@@ +63,5 @@
>    $(srcdir)/vp8/encoder/arm/neon \
>    $(srcdir)/vp8/encoder/generic \
>    $(srcdir)/vp8/encoder/x86 \
>    $(srcdir)/vpx_scale/arm \
>    $(srcdir)/vpx_scale/arm/neon \

With all the things you moved to moz.build, you probably don't need most if not all of those VPATHS.

@@ -350,4 @@
>  GARBAGE += asm_com_offsets.$(OBJ_SUFFIX) asm_com_offsets.asm
>  
>  ifdef MOZ_VP8_ENCODER
> -CSRCS += asm_enc_offsets.c

Moving these two to SOURCES means they'll end up linked into libxul, which is not really expected. They're only in CSRCS so that the rules to create the assembly from the C source work. Doing that with moz.build is not supported yet.

@@ +176,5 @@
> +idct_blk_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> +loopfilter_block_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> +recon_wrapper_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> +variance_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> +vp8_enc_stubs_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2

I don't think that actually works without the full path. You should be able to use $(filter %_sse2.$(OBJ_SUFFIX),$(COBJS)) though.

::: media/libvpx/update_update.sh.py
@@ +56,5 @@
> +        if f.split('.')[-1] in extensions \
> +          and '/' in f \
> +          and f not in ignore_files \
> +          and not filter(lambda folder: folder in f, ignore_folders):
> +            common_files.append(f)

It would probably be better to use FileFinder on top of bug 941245
Attachment #8335025 - Flags: review?(mh+mozilla) → feedback+

Comment 36

4 years ago
(In reply to Mike Hommey [:glandium] from comment #35)
> Would you mind separating the libvpx code import itself with the build
> system and update script changes for next iteration? Will make reviewing
> much easier.

ok, will upload next version in 2 parts.

> With all the things you moved to moz.build, you probably don't need most if
> not all of those VPATHS.

yes, have removed that in my 1.3.0 branch already but can be included here.

> @@ -350,4 @@
> >  GARBAGE += asm_com_offsets.$(OBJ_SUFFIX) asm_com_offsets.asm
> >  
> >  ifdef MOZ_VP8_ENCODER
> > -CSRCS += asm_enc_offsets.c
> 
> Moving these two to SOURCES means they'll end up linked into libxul, which
> is not really expected. They're only in CSRCS so that the rules to create
> the assembly from the C source work. Doing that with moz.build is not
> supported yet.

Right, will now only be included for windows builds,
where it gets filtered out from $(OBJS):

OBJS := $(filter-out asm_enc_offsets.$(OBJ_SUFFIX),$(OBJS))


> @@ +176,5 @@
> > +idct_blk_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> > +loopfilter_block_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> > +recon_wrapper_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> > +variance_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> > +vp8_enc_stubs_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
> 
> I don't think that actually works without the full path. You should be able
> to use $(filter %_sse2.$(OBJ_SUFFIX),$(COBJS)) though.

%_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2

is the most elegant version that works here and sets the flag for the right files.
The old version also worked, so make does not require the full path.

> ::: media/libvpx/update_update.sh.py
> @@ +56,5 @@
> > +        if f.split('.')[-1] in extensions \
> > +          and '/' in f \
> > +          and f not in ignore_files \
> > +          and not filter(lambda folder: folder in f, ignore_folders):
> > +            common_files.append(f)
> 
> It would probably be better to use FileFinder on top of bug 941245

Since this is part of the experimental update_update.sh.py script
that I intend to tweak further, I would suggest to delay this
until bug 941245 is fixed and update later.

Comment 37

4 years ago
Created attachment 8335884 [details] [diff] [review]
vpx-1.2.0v8_libvpx.patch

libvpx part of the previous patch

Comment 38

4 years ago
Created attachment 8335885 [details] [diff] [review]
vpx-1.2.0v8_build.patch

Build part of patch: moz.build Makefile.in update.sh
Attachment #8335885 - Flags: review?(mh+mozilla)

Updated

4 years ago
Attachment #8335884 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Attachment #8335025 - Attachment is obsolete: true
Comment on attachment 8335884 [details] [diff] [review]
vpx-1.2.0v8_libvpx.patch

the libvpx code part is probably better rubberstamped by e.g. cpearce.
Attachment #8335884 - Flags: review?(mh+mozilla) → review?(cpearce)
Comment on attachment 8335885 [details] [diff] [review]
vpx-1.2.0v8_build.patch

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

(Ignoring update_update.sh.py)

::: media/libvpx/moz.build
@@ +301,5 @@
> +        'vp8/encoder/arm/neon/vp8_shortwalsh4x4_neon.asm',
> +    ]
> +
> +if arm_asm_files:
> +    SOURCES += sorted(arm_asm_files)

Did you try this on android? I doubt this works, because you're making the build system build the ARM RCVT files as well as generate the GNU as ones. Even if somehow we don't fail building those files, we'd end up with their symbols twice (once for the ARM RCVT copy, and once for the GNU as one)

@@ +306,5 @@
> +
> +    if CONFIG['VPX_AS_CONVERSION']:
> +        GENERATED_SOURCES += sorted([
> +            "%s.%s" % (f.split('/')[-1], CONFIG['ASM_SUFFIX']) for f in arm_asm_files
> +        ])

This should probably be VPX_ASM_SUFFIX, not ASM_SUFFIX.

You should just keep the full path here and adjust the rules to something like:

$(ASFILES): %.$(ASM_SUFFIX): % $(ASM_OFFSETS)
        $(VPX_AS_CONVERSION) < $< > $@

define dirdep
$(1): $$(call mkdir_deps,$(dir $(1)))
endef
$(foreach f,$(ASFILES),$(eval $(call dirdep,$(f))))
Attachment #8335885 - Flags: review?(mh+mozilla) → review-

Comment 41

4 years ago
Created attachment 8338397 [details] [diff] [review]
vpx-1.2.0v9_build.patch

(In reply to Mike Hommey [:glandium] from comment #40)
> ::: media/libvpx/moz.build
> @@ +301,5 @@
> > +        'vp8/encoder/arm/neon/vp8_shortwalsh4x4_neon.asm',
> > +    ]
> > +
> > +if arm_asm_files:
> > +    SOURCES += sorted(arm_asm_files)
> 
> Did you try this on android? I doubt this works, because you're making the
> build system build the ARM RCVT files as well as generate the GNU as ones.
> Even if somehow we don't fail building those files, we'd end up with their
> symbols twice (once for the ARM RCVT copy, and once for the GNU as one)

It did build but might have had the symbols twice,
just having GENERATED_SOURCES is cleaner in any case, so removed.


> @@ +306,5 @@
> > +
> > +    if CONFIG['VPX_AS_CONVERSION']:
> > +        GENERATED_SOURCES += sorted([
> > +            "%s.%s" % (f.split('/')[-1], CONFIG['ASM_SUFFIX']) for f in arm_asm_files
> > +        ])
> 
> This should probably be VPX_ASM_SUFFIX, not ASM_SUFFIX.

that's the same for the VPX_AS_CONVERSION case, but changed to VPX_ASM_SUFFIX.


> define dirdep
> $(1): $$(call mkdir_deps,$(dir $(1)))
> endef
> $(foreach f,$(ASFILES),$(eval $(call dirdep,$(f))))

GENERATED_DIRS looks like a good match, its not possible
to set from moz.config so adding this to Makefile.in:

GENERATED_DIRS += $(dir $(ASFILES))
Attachment #8335885 - Attachment is obsolete: true
Attachment #8338397 - Flags: review?(mh+mozilla)
Comment on attachment 8335884 [details] [diff] [review]
vpx-1.2.0v8_libvpx.patch

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

Rubberstamp engaged.
Attachment #8335884 - Flags: review?(cpearce) → review+

Comment 43

4 years ago
Created attachment 8340369 [details] [diff] [review]
vpx-1.2.0v10_build.patch

- Update patch to apply to current mozilla-central after Bug 874266 landed
- Don't include update_update.sh.py since it will be replaced in Bug 918550 with update.py
Attachment #8338397 - Attachment is obsolete: true
Attachment #8338397 - Flags: review?(mh+mozilla)
Attachment #8340369 - Flags: review?(mh+mozilla)
Comment on attachment 8340369 [details] [diff] [review]
vpx-1.2.0v10_build.patch

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

::: media/libvpx/moz.build
@@ +303,5 @@
> +
> +if arm_asm_files:
> +    if CONFIG['VPX_AS_CONVERSION']:
> +        GENERATED_SOURCES += sorted([
> +            "%s.%s" % (f, CONFIG['VPX_ASM_SUFFIX']) for f in arm_asm_files

GENERATED_SOURCES += [... for f in sorted(arm_asm_files)]
Attachment #8340369 - Flags: review?(mh+mozilla) → review+
Blocks: 945042
Combined patch, including style fix from comment #44
https://tbpl.mozilla.org/?tree=Try&rev=c587f77953b7
The target-specific -msse2 now confuses pymake. Trying a quick conditionalization:

https://tbpl.mozilla.org/?tree=Try&rev=ff28455fddf0
Created attachment 8341288 [details] [diff] [review]
Update libvpx to 1.2.0 v11

Collapsed build and code update portions of the patch, and applied the fix below to unbreak the windows build. This rule worked last week (but had no effect) but now it confuses pymake. Carrying forward r=glandium, r=cpearce. Green on try.

diff --git a/media/libvpx/Makefile.in b/media/libvpx/Makefile.in
index daa97bb..62c52f0 100644
--- a/media/libvpx/Makefile.in
+++ b/media/libvpx/Makefile.in
@@ -108,7 +108,9 @@ include $(topsrcdir)/config/rules.mk
 # This must be after rules.mk in order to use $(OBJ_SUFFIX) outside a
 # recursively-expanded variable.
 
+ifndef _MSC_VER
 %_sse2.$(OBJ_SUFFIX): CFLAGS += -msse2
+endif
 
 quantize_sse2.$(OBJ_SUFFIX): asm_enc_offsets.asm
 quantize_sse4.$(OBJ_SUFFIX): asm_enc_offsets.asm
Comment on attachment 8341288 [details] [diff] [review]
Update libvpx to 1.2.0 v11

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0144998bab0
Attachment #8341288 - Flags: checkin+

Comment 49

4 years ago
Are attachment 8340369 [details] [diff] [review] and attachment 8335884 [details] [diff] [review] obsolete?
Flags: needinfo?(giles)
https://hg.mozilla.org/mozilla-central/rev/f0144998bab0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Comment 51

4 years ago
Perhaps not surprisingly this breaks gcc LTO build:

/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_regular_quantize_b_sse2'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_fast_quantize_b_sse2'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_full_search_sad_c'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_full_search_sadx3'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_full_search_sadx8'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_refining_search_sadx4'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_diamond_search_sadx4'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_full_search_sadx8'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_refining_search_sad_c'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_diamond_search_sad_c'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_fast_quantize_b_ssse3'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/rtcd.o:rtcd.c:function setup_rtcd_internal: error: undefined reference to 'vp8_regular_quantize_b_sse4'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/temporal_filter.o:temporal_filter.c:function vp8_temporal_filter_iterate_c: error: undefined reference to 'vp8_tempo
ral_filter_apply_sse2'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/temporal_filter.o:temporal_filter.c:function vp8_temporal_filter_iterate_c: error: undefined reference to 'vp8_tempo
ral_filter_apply_sse2'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/temporal_filter.o:temporal_filter.c:function vp8_temporal_filter_iterate_c: error: undefined reference to 'vp8_tempo
ral_filter_apply_sse2'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/temporal_filter.o:temporal_filter.c:function vp8_temporal_filter_iterate_c: error: undefined reference to 'vp8_hex_s
earch'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/temporal_filter.o:temporal_filter.c:function vp8_temporal_filter_prepare_c: error: undefined reference to 'vp8_looka
head_depth'
/var/tmp/moz-build-dir/toolkit/library/../../media/libvpx/temporal_filter.o:temporal_filter.c:function vp8_temporal_filter_prepare_c: error: undefined reference to 'vp8_looka
head_peek'
collect2: error: ld returned 1 exit status
/var/tmp/mozilla-central/config/rules.mk:910: recipe for target 'libxul.so' failed

Comment 52

4 years ago
could it be that you have webrtc (In reply to Octoploid from comment #51)
> Perhaps not surprisingly this breaks gcc LTO build:

whats the exact configuration for LTO builds?
Could it be that you have WebRTC disabled or more specifically: MOZ_VP8_ENCODER

Comment 53

4 years ago
(In reply to Jan Gerber from comment #52)
> (In reply to Octoploid from comment #51)
> > Perhaps not surprisingly this breaks gcc LTO build:
> 
> whats the exact configuration for LTO builds?

I just set "export CXXFLAGS="-flto=4 -march=native -pipe"" in .mozconfig.
Binutils (ar, nm, ranlib) use the linker plugin by default on my machine.

> Could it be that you have WebRTC disabled or more specifically:
> MOZ_VP8_ENCODER

Yes I use --disable-webrtc.
(In reply to Jan Gerber from comment #52)
> could it be that you have webrtc (In reply to Octoploid from comment #51)
> > Perhaps not surprisingly this breaks gcc LTO build:
> 
> whats the exact configuration for LTO builds?
> Could it be that you have WebRTC disabled or more specifically:
> MOZ_VP8_ENCODER

Disabling WebRTC is sufficient to break the build; there's no need to do LTO.
(In reply to Florian Bender from comment #49)
> Are attachment 8340369 [details] [diff] [review] and attachment 8335884 [details] [diff] [review]
> [details] [diff] [review] obsolete?

They are.
Flags: needinfo?(giles)
(Assignee)

Updated

4 years ago
Attachment #8335884 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8340369 - Attachment is obsolete: true

Updated

4 years ago
Depends on: 945917
Depends on: 945859
status-firefox28: --- → fixed
Whiteboard: [qa-]
Depends on: 693057
You need to log in before you can comment on or make changes to this bug.