Closed Bug 1257870 Opened 8 years ago Closed 8 years ago

Configure's version check for system libvpx needs a bump

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed
firefox-esr45 --- fixed

People

(Reporter: glandium, Assigned: rillian)

Details

Attachments

(3 files, 2 obsolete files)

Since version 45, webrtc code uses VP9E_SET_NOISE_SENSITIVITY, which was added in libvpx 1.4, but we currently require 1.3 for system libvpx. I do know building 45 with libvpx 1.4.0 works. I haven't checked trunk yet (in case some new requirements have emerged).
Hi Ralph -- Can you handle this?
Rank: 25
Flags: needinfo?(giles)
Priority: -- → P2
Attached patch Require 1.4 --with-system-libvpx (obsolete) — Splinter Review
Sure. I don't think a feature test is worthwhile here since we're rewriting the configure script anyway, so I just bumped the pkg-config version check.
Assignee: nobody → giles
Flags: needinfo?(giles)
Attachment #8733536 - Flags: review?(mh+mozilla)
Comment on attachment 8733536 [details] [diff] [review]
Require 1.4 --with-system-libvpx

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

::: old-configure.in
@@ +4222,4 @@
>      dnl === libvpx Version check ===
>      dnl ============================
>      dnl Check to see if we have a system libvpx package.
> +    PKG_CHECK_MODULES(MOZ_LIBVPX, vpx >= 1.4.0)

Do you know if we need even more now? I haven't tried building anything newer than 45 with 1.4.
Attachment #8733536 - Flags: review?(mh+mozilla) → review+
Attached patch Require 1.5 --with-system-libvpx (obsolete) — Splinter Review
You're right, we also depend on some post-1.4 SVC changes.

This patch builds for me on fedora 23 against a libvpx built from the v1.5.0 git tag, after removing libvpx.a from the install. I guess preferring static libraries when linking libxul is another bug?
Attachment #8733536 - Attachment is obsolete: true
Attachment #8734032 - Flags: review?(mh+mozilla)
The WebRTC/SVC change may require a more complicated check:
https://bugs.chromium.org/p/chromium/issues/detail?id=575651

If it's the same as the chromium issue then the system libvpx needs to be built with --enable-experimental and --enable-spatial-svc, which Pawel contends that most system builds will not likely contain.
Comment on attachment 8734032 [details] [diff] [review]
Require 1.5 --with-system-libvpx

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

The spatial svc thing mentioned in comment 5 was dealt with in bug 1248335.

It would be worth mentioning bug 1237023 in the commit message, which added the use of VPX_MAX_LAYERS in 46.

So, to summarize, we'd need a bump to 1.4 for 45esr and 1.5 for 46+. Can you do the necessary uplift requests?
Attachment #8734032 - Flags: review?(mh+mozilla) → review+
Mention relevant bugs in the commit message per review comments. Carrying r=glandium.
Attachment #8734032 - Attachment is obsolete: true
Attachment #8735632 - Flags: review+
Comment on attachment 8735632 [details] [diff] [review]
Require 1.5 --with-system-libvpx

Approval Request Comment
[Feature/regressing bug #]: --enable-system-libvpx

[User impact if declined]: Downstream packagers may see confusing build failures.

[Describe test coverage new/current, TreeHerder]: Afaik we don't test this configuration. This is a convenience for our downstream Linux packagers.

[Risks and why]: This bumps a version check for users building against a system copy of libvpx, when they don't want to use the in-tree version. It doesn't affect any of our official builds. If the check is somehow accepts the system libvpx incorrectly, one gets the same error message as if the patch is not accepted. If it rejects system libvpx unnecessarily, it might spuriously require packagers to upgrade to the lasted libvpx release.

[String/UUID change made/needed]: None.
Attachment #8735632 - Flags: approval-mozilla-aurora?
Backport to beta, carrying r=glandium.

Approval Request Comment
[Feature/regressing bug #]: --enable-system-libvpx

[User impact if declined]: Downstream packagers may see confusing build failures.

[Describe test coverage new/current, TreeHerder]: Afaik we don't test this configuration. This is a convenience for our downstream Linux packagers.

[Risks and why]: This bumps a version check for users building against a system copy of libvpx, when they don't want to use the in-tree version. It doesn't affect any of our official builds. If the check is somehow accepts the system libvpx incorrectly, one gets the same error message as if the patch is not accepted. If it rejects system libvpx unnecessarily, it might spuriously require packagers to upgrade to the lasted libvpx release.

[String/UUID change made/needed]: None.
Attachment #8735639 - Flags: review+
Attachment #8735639 - Flags: approval-mozilla-beta?
Variant patch for the the esr45 branch, requiring just libvpx 1.4.0. Carrying r=glandium per comment #6.

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This gives better build requirement feedback for downstream users setting --enable-system-libvpx.

User impact if declined: Downstream packagers may see harder-to-debug build failures.

Fix Landed on Version: 48 (inbound as of this writing)

Risk to taking this patch (and alternatives if risky):
Afaik we don't test this configuration. This is a convenience for our downstream Linux packagers.

This bumps a version check for users building against a system copy of libvpx, when they don't want to use the in-tree version. It doesn't affect any of our official builds. If the check is somehow accepts the system libvpx incorrectly, one gets the same error message as if the patch is not accepted. If it rejects system libvpx unnecessarily, it might spuriously require packagers to upgrade to the lasted libvpx release.

String or UUID changes made by this patch: None.
Attachment #8735645 - Flags: review+
Attachment #8735645 - Flags: approval-mozilla-esr45?
https://hg.mozilla.org/mozilla-central/rev/ca78a5af2576
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8735632 [details] [diff] [review]
Require 1.5 --with-system-libvpx

NPOTB, helps downstream linux packagers, Aurora47+
Attachment #8735632 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8735639 [details] [diff] [review]
Require 1.5 --with-system-libvpx (firefox 46)

Build requirements version check, ok to uplift to beta.
Attachment #8735639 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8735645 [details] [diff] [review]
Require 1.4 --with-system-libvpx (firefox 45)

Simplify the life of Linux packagers, taking it.
Should be in 45.1.0
Attachment #8735645 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.