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

RESOLVED FIXED in Firefox 53

Status

()

Core
WebRTC: Audio/Video
P2
normal
Rank:
25
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jan Beich, Assigned: jesup)

Tracking

({regression})

Trunk
mozilla54
regression
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox53 fixed, firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
$ 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.
(Reporter)

Comment 1

a year ago
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!/
(Reporter)

Comment 2

a year ago
James, is spatial_svc API still considered an experiment?
Flags: needinfo?(jzern)
(Reporter)

Comment 3

a year ago
Mike, would Debian be willing to build libvpx package with --enable-experimental --enable-spatial-svc just for Firefox?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Updated

a year ago
status-firefox53: affected → fix-optional
(Reporter)

Comment 5

a year ago
(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
(Assignee)

Comment 6

a year ago
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?
Flags: needinfo?(johannkoenig)
(Assignee)

Comment 7

a year ago
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
Rank: 25
status-firefox53: fix-optional → affected
Priority: -- → P2
(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.
Flags: needinfo?(mh+mozilla) → needinfo?(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.
(Assignee)

Comment 10

a year ago
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).
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.
(Assignee)

Comment 12

a year ago
Created attachment 8829485 [details] [diff] [review]
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
Attachment #8829485 - Flags: review?(ted)
(Assignee)

Updated

a year ago
Assignee: nobody → rjesup
Status: NEW → ASSIGNED

Comment 13

a year ago
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
Flags: needinfo?(jzern)
Flags: needinfo?(johannkoenig)

Comment 14

a year ago
(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

a year ago
(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

a year ago
(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

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

Comment 18

a year ago
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

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

Comment 20

a year ago
(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] ?
Flags: needinfo?(rjesup)
(Assignee)

Comment 21

a year ago
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
Flags: needinfo?(rjesup)
Attachment #8829485 - Flags: review?(gps)
Attachment #8829485 - Flags: review?(giles)

Comment 22

a year ago
I think jbeich@ wants https://codereview.webrtc.org/2654633002
Flags: needinfo?(rjesup)
(Reporter)

Comment 23

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

Correct.
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?
Attachment #8829485 - Flags: review?(giles) → review-
(Assignee)

Comment 25

a year ago
(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)
Flags: needinfo?(rjesup)
(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.
(Assignee)

Comment 27

a year ago
Created attachment 8831291 [details] [diff] [review]
Cherry-pick upstream webrtc.org patch to not depend on 'experimental' includes for libvpx
Attachment #8831291 - Flags: review?(giles)
(Assignee)

Updated

a year ago
Attachment #8829485 - Attachment is obsolete: true
Attachment #8829485 - Flags: review?(ted)
Attachment #8829485 - Flags: review?(gps)
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
Attachment #8831291 - Flags: review?(giles) → review+

Comment 29

a year ago
Pushed by rjesup@wgate.com:
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
(Assignee)

Comment 30

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

Jan, care to give this a try?
Flags: needinfo?(slomo) → needinfo?(jbeich)
(Reporter)

Comment 31

a year ago
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.
Flags: needinfo?(jbeich)
Attachment #8831291 - Flags: feedback+

Comment 32

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b4627afc5f2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora approval on this when you get a chance.
status-firefox52: --- → unaffected
Flags: needinfo?(rjesup)
(Assignee)

Comment 34

a year ago
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
Flags: needinfo?(rjesup)
Attachment #8831291 - Flags: approval-mozilla-aurora?
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+
Attachment #8831291 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 36

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/900c746565fd
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.