1332664 - media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:20: fatal error: 'vpx/svc_context.h' file not found

Status: RESOLVED FIXED

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

Description

[Jan Beich]

$ pkg info -x vpx
libvpx-1.6.1

$ echo ac_add_options --with-system-libvpx >>.mozconfig

$ ./mach build
[...]
In file included from objdir/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp9/vp9_webrtc_vp9/Unified_cpp_codecs_vp90.cpp:2:
In file included from media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:11:
In file included from media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:13:
In file included from media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:20:
objdir/dist/system_wrappers/vpx/svc_context.h:3:15: fatal error: 'vpx/svc_context.h' file not found
#include_next <vpx/svc_context.h>
              ^~~~~~~~~~~~~~~~~~~
1 error generated.

Comment 1

[Jan Beich]

vpx/svc_context.h is only installed if libvpx is built with an option that doesn't even show up in "./configure --help". Probably have to restore LIBVPX_SVC conditionals but define for bundled libvpx or if vpx/svc_context.h exists system-wide.
https://chromium.googlesource.com/webm/libvpx/+/f12ebfc939b6%5E!/

Comment 2

[Jan Beich]

James, is spatial_svc API still considered an experiment?

Comment 3

[Jan Beich]

Mike, would Debian be willing to build libvpx package with --enable-experimental --enable-spatial-svc just for Firefox?

Comment 4

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

jan - at least on my Fedora 22 system, with system libvpx of 1.3.0 (current is 1.6.1), I have a globally installed svc_context.h.

Comment 5

[Jan Beich]

(In reply to Randell Jesup [:jesup] from comment #4)
> jan - at least on my Fedora 22 system, with system libvpx of 1.3.0 (current
> is 1.6.1), I have a globally installed svc_context.h.

vpx/svc_context.h is hidden behind an option since 1.4.0. Besides, Fedora doesn't pass --with-system-libvpx unlike Arch, Debian, Gentoo, Mageia, FreeBSD, NetBSD.

http://pkgs.fedoraproject.org/cgit/rpms/firefox.git/tree/firefox-mozconfig

Comment 6

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

Hmmm.  well that's a problem, since the webrtc code wants to use those structs.

I could bring back support for vpx-without-svc_context.h.... but that means loss of functionality, especially as we start to leverage this for simulcast and for shared encoders.  and likely it will start either breaking, or be extra maintenance to fix.   Perhaps file a bug upstream to expose the file in 1.6.2 by default, so we can eventually remove the hack?

Johann?

Comment 7

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

Moving back to affected for now to keep it on radar, since per comment 5 it affects most downstream linux/bsd projects other than Fedora

Comment 8

[Mike Hommey [:glandium]]

(In reply to Jan Beich from comment #3)
> Mike, would Debian be willing to build libvpx package with
> --enable-experimental --enable-spatial-svc just for Firefox?

That's a question for Sebastian.

Comment 9

[Sebastian Dröge (:slomo)]

This was shortly discussed here: https://lists.alioth.debian.org/pipermail/pkg-gstreamer-maintainers/2016-December/013660.html

tldr is that I'm not too happy to activate these settings as they are marked as experimental and users could generate streams that are not decodeable anymore in the future if they use this experimental features now.

Comment 10

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

My plan is to back out the main patch (bring back LIBVPX_SVC), and control that in old-configure.in via HAVE_SVC_CONTEXT_H.  However, eventually this may become too much of a maintenance burden - we're planning on features, like shared encoding, which requires SVC settings.

I'll also note that webrtc.org and Chrome require this feature, so I'm not certain I'd call it "experimental" anymore (perhaps a comment for the upstream maintainers).

Comment 11

[Sebastian Dröge (:slomo)]

It's hidden behind a "--enable-experimental" configure switch. If they want to commit to stability of that feature, they should remove that :) My guess is that they don't care in Chromium about the stability of that feature if it's only used for WebRTC anyway, which allows negotiation of these things to some degree and is "experimental" by itself, or if it's used by YouTube by being able to do decisions in Javascript code before requesting such streams.

For having this in a general purpose situation like a Linux distribution I'd prefer having commitment from the libvpx maintainers that this feature is considered bitstream stable before enabling it in general. It'd be rather annoying if someone used SVC for archiving purposes, and then libvpx changes and everything becomes undecodeable. A scenario that does not really matter for the above mentioned use cases.

Comment 12

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

Attachment: Test for svc_context.h to control code that depends on it in webrtc

re-uploading to the right bug; will be combined with backout of the code changes from Bug 1332139

Comment 13

[johannkoenig]

We have discussed this several times with Chromium[0] because it had become somewhat routine for people working on WebRTC to add encoder flags (outside the SVC area), breaking the 'use system libvpx' flag until a new release rolled out.

I had been holding off on moving SVC out of experimental not because of stability issues but because the API was still in flux. It seems to have stabilized at this point so we (upstream libvpx) should probably at least move it out from behind the --enable-experimental flag and probably enable it by default, as WebRTC seems to be the primary use for vp8 these days.

The best place for such a discussion is probably WebRTC though - they are the best ones to decide if the feature is stable or if they still have plans to tweak it.

Per the other points: the SVC feature does not create incompatible bitstreams. It adds encoder features but the resulting bitstream is still vp8.

[0] https://bugs.chromium.org/p/chromium/issues/detail?id=575651

Comment 14

[James Zern]

(In reply to johannkoenig from comment #13)
> We have discussed this several times with Chromium[0] because it had become
> somewhat routine for people working on WebRTC to add encoder flags (outside
> the SVC area), breaking the 'use system libvpx' flag until a new release
> rolled out.
> 
> I had been holding off on moving SVC out of experimental not because of
> stability issues but because the API was still in flux. It seems to have
> stabilized at this point so we (upstream libvpx) should probably at least
> move it out from behind the --enable-experimental flag and probably enable
> it by default, as WebRTC seems to be the primary use for vp8 these days.
> 

There are multiple versions of svc in libvpx code. The spatial svc experiment
was never finished (or shown to be useful). As far as I can tell they don't
really need the svc_context.h include.

> The best place for such a discussion is probably WebRTC though - they are
> the best ones to decide if the feature is stable or if they still have plans
> to tweak it.
> 
> Per the other points: the SVC feature does not create incompatible
> bitstreams. It adds encoder features but the resulting bitstream is still
> vp8.
> 

The SPATIAL_SVC experiment was for vp9.

> [0] https://bugs.chromium.org/p/chromium/issues/detail?id=575651

Comment 15

[James Zern]

(In reply to Jan Beich from comment #2)
> James, is spatial_svc API still considered an experiment?

Yes. It isn't maintained and wasn't shown to be beneficial. I'd still like to remove it. There are multiple versions of svc in libvpx, webrtc relies on the others.

Comment 16

[James Zern]

(In reply to Randell Jesup [:jesup] from comment #10)
> My plan is to back out the main patch (bring back LIBVPX_SVC), and control
> that in old-configure.in via HAVE_SVC_CONTEXT_H.  However, eventually this
> may become too much of a maintenance burden - we're planning on features,
> like shared encoding, which requires SVC settings.
> 
> I'll also note that webrtc.org and Chrome require this feature, so I'm not
> certain I'd call it "experimental" anymore (perhaps a comment for the
> upstream maintainers).

This particular experiment is disabled in chrome [1]. The problem is the term is overloaded in the code base.

[1] https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx/source/config/linux/x64/vpx_config.h#88

Comment 17

[johannkoenig]

Well, that's my mistake then. Hopefully this cleans up this issue:
https://codereview.webrtc.org/2654633002

Comment 18

[johannkoenig]

Turns out there is some confusion about svc_context.h

Opened a WebRTC bug to decide what to do:
https://bugs.chromium.org/p/webrtc/issues/detail?id=7038

Comment 19

[johannkoenig]

This was a bug in WebRTC. Your next roll from there should fix it:
https://codereview.webrtc.org/2654633002

Comment 20

[Jan Beich]

(In reply to johannkoenig from comment #19)
> This was a bug in WebRTC. Your next roll from there should fix it:
> https://codereview.webrtc.org/2654633002

I confirm, the fix is enough for comment 0. Randell, can you backport it on Nightly54/Aurora53 and obsolete attachment 8829485 [details] [diff] [review] ?

Comment 21

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

Comment on attachment 8829485 [details] [diff] [review]
Test for svc_context.h to control code that depends on it in webrtc

Will take any review... :-) Need to get this in and uplift to 53

Comment 22

[johannkoenig]

I think jbeich@ wants https://codereview.webrtc.org/2654633002

Comment 23

[Jan Beich]

(In reply to johannkoenig from comment #22)
> I think jbeich@ wants https://codereview.webrtc.org/2654633002

Correct.

Comment 24

[Ralph Giles (:rillian)]

Comment on attachment 8829485 [details] [diff] [review]
Test for svc_context.h to control code that depends on it in webrtc

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

Doesn't look like this is a clean patch?

::: media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc
@@ -713,2 @@
>    }
> -#endif

Which branch is this patch against? The line numbers and #endif don't match for my m-c or aurora branches. Should there be more to this diff?

::: old-configure.in
@@ +3002,5 @@
>          AC_DEFINE(MOZ_VPX_NO_MEM_REPORTING)
>      fi
> +
> +    MOZ_CHECK_HEADER([vpx/svc_context.h], [AC_DEFINE(HAVE_SVC_CONTEXT_H)],
> +      [])

You shouldn't need the empty 'else' clause here?

Comment 25

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

(In reply to Ralph Giles (:rillian) | needinfo me from comment #24)
> Comment on attachment 8829485 [details] [diff] [review]
> Test for svc_context.h to control code that depends on it in webrtc
> 
> Review of attachment 8829485 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Doesn't look like this is a clean patch?
> 
> ::: media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc
> @@ -713,2 @@
> >    }
> > -#endif
> 
> Which branch is this patch against? The line numbers and #endif don't match
> for my m-c or aurora branches. Should there be more to this diff?

This would land on top of the backout of the previously-landed patch, which I have not uploaded.

> 
> ::: old-configure.in
> @@ +3002,5 @@
> >          AC_DEFINE(MOZ_VPX_NO_MEM_REPORTING)
> >      fi
> > +
> > +    MOZ_CHECK_HEADER([vpx/svc_context.h], [AC_DEFINE(HAVE_SVC_CONTEXT_H)],
> > +      [])
> 
> You shouldn't need the empty 'else' clause here?

Probably not.

However: 
> I think jbeich@ wants https://codereview.webrtc.org/2654633002

Since we're only in Aurora with this bug, and that's a pretty straightforward patch, we could import that upstream fix instead.  (Just landed on upstream webrtc.org yesterday)

Comment 26

[Ralph Giles (:rillian)]

(In reply to Randell Jesup [:jesup] from comment #25)

> Since we're only in Aurora with this bug, and that's a pretty
> straightforward patch, we could import that upstream fix instead.  (Just
> landed on upstream webrtc.org yesterday)

I think that's better, but either fix is fine.

Comment 27

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

Attachment: Cherry-pick upstream webrtc.org patch to not depend on 'experimental' includes for libvpx

Comment 28

[Ralph Giles (:rillian)]

Comment on attachment 8831291 [details] [diff] [review]
Cherry-pick upstream webrtc.org patch to not depend on 'experimental' includes for libvpx

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

lgtm

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b4627afc5f2
Cherry-pick upstream webrtc.org patch to not depend on 'experimental' includes for libvpx r=rillian

Comment 30

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

Green Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94b4028dc133c24d5a179a47efc5c81364ed6a30

Jan, care to give this a try?

Comment 31

[Jan Beich]

Comment on attachment 8831291 [details] [diff] [review]
Cherry-pick upstream webrtc.org patch to not depend on 'experimental' includes for libvpx

I confirm, comment 0 no longer happens on mozilla-inbound.

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3b4627afc5f2

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 34

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

Comment on attachment 8831291 [details] [diff] [review]
Cherry-pick upstream webrtc.org patch to not depend on 'experimental' includes for libvpx

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1332139 

[User impact if declined]: Blocks downstream linux/bsd builds

[Is this code covered by automated tests?]: no - requires --with-system-vpx

[Has the fix been verified in Nightly?]: yes

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: No

[Why is the change risky/not risky?]: Switches include files and which-structure details; landed upstream.

[String changes made/needed]: none

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8831291 [details] [diff] [review]
Cherry-pick upstream webrtc.org patch to not depend on 'experimental' includes for libvpx

NPOTB, needed for BDS/linux distro builds. Aurora53+

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/900c746565fd