Closed Bug 1248335 Opened 4 years ago Closed 4 years ago

Configure's version check of libvpx is not enough

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
Blocking Flags:

People

(Reporter: glandium, Assigned: jesup)

References

Details

Attachments

(1 file)

When building with --with-system-libvpx, we need to test something like CONFIG_SPATIAL_SVC somehow, because we are using svc_context.h, and the header is not necessarily provided by libvpx-dev packages, as it is optional in libvpx itself.
BTW, it's funny that the vpx_config_*.h files in the tree say #define CONFIG_SPATIAL_SVC 0 when svc_context.h is not shipped by libvpx without that option...

Reading further, the problem might just be that media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp9/vp9_impl.h is including svc_context.h unconditionally, while it has #ifdef LIBVPX_SVC for every use of vpx_svc_* functions...
huh, webrtc is unconditionally using svc_context.h/SvcInternal_t without svc_encodeframe.c being compiled. wtf?
Component: Audio/Video → WebRTC: Audio/Video
Flags: needinfo?(rjesup)
Assignee: nobody → rjesup
backlog: --- → webrtc/webaudio+
Rank: 15
Flags: needinfo?(rjesup)
Priority: -- → P1
(In reply to Mike Hommey [:glandium] from comment #2)
> huh, webrtc is unconditionally using svc_context.h/SvcInternal_t without
> svc_encodeframe.c being compiled. wtf?

We cherry-picked vp9 support from upstream, which had extensive use of svc.  Out current libvpx doesn't support all of the svc stuff used by upstream, so we ifdeffed some of it off, but other parts that referenced the svc_internal struct are still there.  We're not actually doing SVC currently.  Looking to expand the ifdefs, though this might be painful.  Can we demand a minimum version/configuration of libvpx for --with-system-libvpx?  (perhaps not...)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8720941 [details]
MozReview Request: Bug 1248335: avoid using SvcInternal structure entirely, as system-vpx may not have it r?glandium,pkerr

https://reviewboard.mozilla.org/r/35511/#review32199

Looks good to me.
Attachment #8720941 - Flags: review?(pkerr) → review+
> Can we demand a minimum version/configuration of libvpx for --with-system-libvpx?  (perhaps not...)

As per comment 0, no, since svc_context.h is not version dependent, but how-system-libvpx-was-built dependent.
The patch works.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8720941 [details]
MozReview Request: Bug 1248335: avoid using SvcInternal structure entirely, as system-vpx may not have it r?glandium,pkerr

https://reviewboard.mozilla.org/r/35511/#review32365

It's not my place to review this, but see comment 7
Attachment #8720941 - Flags: review?(mh+mozilla)
https://hg.mozilla.org/mozilla-central/rev/a80acb104b38
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Randell, could you request an uplift?
Flags: needinfo?(rjesup)
Comment on attachment 8720941 [details]
MozReview Request: Bug 1248335: avoid using SvcInternal structure entirely, as system-vpx may not have it r?glandium,pkerr

Approval Request Comment
[Feature/regressing bug #]: VP9 update

[User impact if declined]: --with-system-libvpx will not work.  Some distros use this apparently.

[Describe test coverage new/current, TreeHerder]: alternate build

[Risks and why]: almost no risk; ifdefs out more code used only for VP9 SVC, which we don't use or support yet.

[String/UUID change made/needed]: none
Flags: needinfo?(rjesup)
Attachment #8720941 - Flags: approval-mozilla-beta?
Comment on attachment 8720941 [details]
MozReview Request: Bug 1248335: avoid using SvcInternal structure entirely, as system-vpx may not have it r?glandium,pkerr

Should help webrtc work with more linux configurations/hardware.
OK to uplift to beta
Attachment #8720941 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.