1248335 - Configure's version check of libvpx is not enough

Status: RESOLVED FIXED

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

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

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...

Comment 2

[Mike Hommey [:glandium]]

huh, webrtc is unconditionally using svc_context.h/SvcInternal_t without svc_encodeframe.c being compiled. wtf?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

(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...)

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MozReview Request: Bug 1248335: avoid using SvcInternal structure entirely, as system-vpx may not have it r?glandium,pkerr

Review commit: https://reviewboard.mozilla.org/r/35511/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35511/

Comment 5

[Paul Kerr [:pkerr]]

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.

Comment 6

[Mike Hommey [:glandium]]

> 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.

Comment 7

[Mike Hommey [:glandium]]

The patch works.

Comment 8

[Mike Hommey [:glandium]]

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a80acb104b38

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a80acb104b38

Comment 11

[Mike Hommey [:glandium]]

Randell, could you request an uplift?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8080e98a0141

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8080e98a0141