826985 - Support more video formats with WebRTC on Linux

Status: RESOLVED WORKSFORME

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

Description

[Jan Beich]

Why not support more video formats on Linux? There are cameras that don't support

  V4L2_PIX_FMT_MJPEG
  V4L2_PIX_FMT_YUV420
  V4L2_PIX_FMT_YUYV

while libv4l2 can be plugged to do the conversion.

Comment 1

[Jan Beich]

Attachment: wrap open/ioctl/etc, tested by a FreeBSD user with JPEG cam

Comment 2

[Jan Beich]

s/tested/requested/

Comment 3

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

Comment on attachment 698245 [details] [diff] [review]
wrap open/ioctl/etc, tested by a FreeBSD user with JPEG cam

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

r- until the questions are answered.  Also, build peer review will be needed.

::: configure.in
@@ +5286,5 @@
> +    *-linux*)
> +        MOZ_WEBRTC_LIBV4L=1
> +        PKG_CHECK_MODULES(MOZ_LIBV4L2, libv4l2, ,
> +            [echo "$MOZ_LIBV4L2_PKG_ERRORS"
> +              AC_MSG_ERROR([WebRTC on Linux needs libv4l2 for video format conversion.])])

If there are Linux/BSD versions or installs we care about without v4l2, then this would break them.

Also, this is compile-time - this will check if the builder has the lib; if the client doesn't FF won't load.  How ubiquitous is v4l2?  Do our test servers (older Fedora 12? 13?) have it?  (Probably).  Our builders (frozen RHEL5)?  (maybe, maybe not)

what about android; will this break that?  (I dunno, maybe not)

If it needs to build without v4l2, then the ioctls/etc need to be ifdefed/wrapped.  If it needs to build with, but load without, then alternates with some late-binding library load is needed (see how alsa is handled)

Are there any issues with what version of v4l2 is on the builder or the client?

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 698245 [details] [diff] [review]
wrap open/ioctl/etc, tested by a FreeBSD user with JPEG cam

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

I don't really have anything to add beyond what jesup has already said. Adding new runtime dependencies is not to be taken lightly, certainly, and we should at least be able to dynamically load the lib so we can fail gracefully when it's not present.

Comment 5

[jonathan chetwynd]

Summary changed:
Support more cameras with WebRTC on Linux
to
Support more video formats with WebRTC on Linux

seems to match description and comments better,

and filed 'support more than one camera at a time' here:

#835332

Comment 6

[Jan Beich]

Attachment: wrap open/ioctl/etc, ifdef version

libv4l2 is only used when video_capture is built on Linux. With bug 807492 BSD and Solaris may link with it, too.

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

> If it needs to build with, but load without, then alternates with
> some late-binding library load is needed (see how alsa is handled)

mozilla build currently doesn't support anything but alsa on linux.
Besides, there is no late-binding for alsa in libcubeb/libsydneyaudio.

Comment 7

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

Comment on attachment 713375 [details] [diff] [review]
wrap open/ioctl/etc, ifdef version

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

Submit an issue at webrtc.org, mark here, and submit a patch to codereview there please.  Thanks

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 713375 [details] [diff] [review]
wrap open/ioctl/etc, ifdef version

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

::: media/webrtc/trunk/webrtc/modules/video_capture/video_capture.gypi
@@ +57,5 @@
> +                  'defines': [
> +                    'HAVE_LIBV4L2',
> +                  ],
> +                  'libraries': [
> +                    '-lv4l2',

Presumably you should actually be persisting the MOZ_V4L2_LIBS value from configure to here. You also probably want MOZ_V4L2_CFLAGS.

Comment 9

[Jan Beich]

Attachment: wrap open/ioctl/etc, ifdef version

carrying over r+ by :jesup from the bitrotten version

(In reply to Randell Jesup [:jesup] from comment #7)
> Submit an issue at webrtc.org, mark here, and submit a patch to codereview
> there please.  Thanks

I'm unable to test upstream and Google wants to abuse
my privacy (CLA and no Tor) just to submit a patch.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> Presumably you should actually be persisting the MOZ_V4L2_LIBS value from
> configure to here. You also probably want MOZ_V4L2_CFLAGS.

-lv4l2 is for upstream which doesn't pass CFLAGS for other libs.
mozilla links under toolkit/library and signaling/test where
MOZ_V4L2_LIBS are already used.

Comment 10

[Eric Rescorla (:ekr)]

We shouldn't be carrying this as a diff against upstream. The right thing here is
for this to go upstream and then us to pick it up in an updated version of webrtc.org

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 795112 [details] [diff] [review]
wrap open/ioctl/etc, ifdef version

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

This mostly looks fine, I just have one issue with the configure bit that needs to be resolved.

::: configure.in
@@ +9189,5 @@
>  fi
>  
> +if test -n "$MOZ_LIBV4L2_LIBS"; then
> +   EXTRA_GYP_DEFINES="$EXTRA_GYP_DEFINES -D use_libv4l2=1"
> +fi

We're generally opposed to having functionality silently enabled or disabled based on the presence of packages on the builder. If this is intended to be optional then there should be an explicit --enable-webrtc-libv4l2 configure argument, and configure should error out if the package is not available in that case.

Comment 12

[Jan Beich]

Attachment: wrap open/ioctl/etc with ifdef, v2 (as --enable-libv4l2 option)

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8362336 [details] [diff] [review]
wrap open/ioctl/etc with ifdef, v2 (as --enable-libv4l2 option)

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

The build bits look fine here. Did you sufficiently address jesup/ekr's upstreaming concerns?

::: configure.in
@@ +5150,5 @@
> +if test -n "$MOZ_LIBV4L2"; then
> +    PKG_CHECK_MODULES(MOZ_LIBV4L2, libv4l2, ,
> +        AC_MSG_ERROR([$MOZ_LIBV4L2_PKG_ERRORS Need libv4l2 development package. (On Ubuntu, you might try installing the package libv4l-dev.)]))
> +    dnl Unused, WebRTC defines HAVE_LIBV4L2 in video_capture.gypi
> +    dnl AC_DEFINE(MOZ_LIBV4L2)

Just remove these lines instead of commenting them out.

Comment 14

[Jan Beich]

Attachment: MozReview Request: Bug 826985 - Add --enable-libv4l2 to support more video formats on Linux. r?glandium r?jesup

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

Comment 15

[Jan Beich]

(In reply to Ted Mielczarek [:ted.mielczarek] (Vacation May 5th-11th) from comment #13)
> > +    dnl Unused, WebRTC defines HAVE_LIBV4L2 in video_capture.gypi
> > +    dnl AC_DEFINE(MOZ_LIBV4L2)
> 
> Just remove these lines instead of commenting them out.

Done and rebased. Asking another build peer because MozReview says:

> commit message for 863063001239 has r=ted but they have not granted a ship-it. review will be requested on your behalf
[...]
> (error publishing review request 51669: Error publishing: Bugzilla error: Ted Mielczarek [:ted.mielczarek] (Vacation May 5th-11th) <[e-mail]> is not currently accepting 'review' requests. (HTTP 500, API Error 225))

(In reply to Eric Rescorla (:ekr) from comment #10)
> We shouldn't be carrying this as a diff against upstream. The right
> thing here is for this to go upstream and then us to pick it up in
> an updated version of webrtc.org

WebRTC project is hosted by Google. In order to submit code CLA must be signed, which can only be done with a Google Account. However, Google, like many "big brother" adverstising companies, prohibits using Tor (or rather Tor Browser) during signup. I'm a volunteer but my privacy isn't free. Is Mozilla relationship with upstream so one-sided that it cannot help its own contributors?

Upstream code currently doesn't work on FreeBSD. In order to test the patch here works correctly I need to fix it first. Worse, WebRTC is developed along with Chromium, and that doesn't work on FreeBSD as well. [e-mail] folks made some progress in upstreaming patches but it's stalled. And trying to find anything recent is hard because upstream review tool doesn't allow searching by subject e.g., anything that contains "freebsd" string.

Comment 16

[Eric Rescorla (:ekr)]

(In reply to Jan Beich from comment #15)
> (In reply to Ted Mielczarek [:ted.mielczarek] (Vacation May 5th-11th) from
> comment #13)
> > > +    dnl Unused, WebRTC defines HAVE_LIBV4L2 in video_capture.gypi
> > > +    dnl AC_DEFINE(MOZ_LIBV4L2)
> > 
> > Just remove these lines instead of commenting them out.
> 
> Done and rebased. Asking another build peer because MozReview says:
> 
> > commit message for 863063001239 has r=ted but they have not granted a ship-it. review will be requested on your behalf
> [...]
> > (error publishing review request 51669: Error publishing: Bugzilla error: Ted Mielczarek [:ted.mielczarek] (Vacation May 5th-11th) <[e-mail]> is not currently accepting 'review' requests. (HTTP 500, API Error 225))
> 
> (In reply to Eric Rescorla (:ekr) from comment #10)
> > We shouldn't be carrying this as a diff against upstream. The right
> > thing here is for this to go upstream and then us to pick it up in
> > an updated version of webrtc.org
> 
> WebRTC project is hosted by Google. In order to submit code CLA must be
> signed, which can only be done with a Google Account. However, Google, like
> many "big brother" adverstising companies, prohibits using Tor (or rather
> Tor Browser) during signup. I'm a volunteer but my privacy isn't free. Is
> Mozilla relationship with upstream so one-sided that it cannot help its own
> contributors?

I don't think it's an issue of "one-sided" but rather that the point of upstream
is that we minimize changes and this seems like something that in principle could
be upstreamed. I've needinfoed Justin Uberti. Perhaps he can help you with the
CLA issue.

Comment 17

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

Comment on attachment 8750854 [details]
MozReview Request: Bug 826985 - Add --enable-libv4l2 to support more video formats on Linux. r?glandium r?jesup

https://reviewboard.mozilla.org/r/51671/#review48509

Comment 18

[Justin Uberti]

I don't know of any way to sign the CLA as an individual other than https://cla.developers.google.com/clas.

Note that blocking Tor on the account creation page is simply an anti-abuse measure; unfortunately, one of the primary uses of proxies has been to create large numbers of spam accounts.

If this is an insurmountable issue perhaps someone can upstream the patch on your behalf.

Comment 19

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

(In reply to Justin Uberti from comment #18)
> If this is an insurmountable issue perhaps someone can upstream the patch on
> your behalf.

Jan - perhaps we can upstream the patch for you, if you're good with that.  Since the patches made here (that need uplifting, not to moz-specific files) are to files under the google-derived BSD license, I imagine there aren't any issues with that part of it.

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8750854 [details]
MozReview Request: Bug 826985 - Add --enable-libv4l2 to support more video formats on Linux. r?glandium r?jesup

https://reviewboard.mozilla.org/r/51671/#review48643

::: old-configure.in:3679
(Diff revision 1)
> +dnl ========================================================
> +dnl = Enable libv4l2 support
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(libv4l2,
> +[  --enable-libv4l2        Use libv4l2 to convert between webcam formats],
> +   MOZ_LIBV4L2=1,
> +   MOZ_LIBV4L2=)
> +
> +if test -n "$MOZ_LIBV4L2"; then
> +    PKG_CHECK_MODULES(MOZ_LIBV4L2, libv4l2, ,
> +        AC_MSG_ERROR([$MOZ_LIBV4L2_PKG_ERRORS Need libv4l2 development package. (On Ubuntu, you might try installing the package libv4l-dev.)]))
> +fi
> +
> +AC_SUBST(MOZ_LIBV4L2)

Please wait for bug 1269513 and do the changes in toolkit/moz.configure instead.

That said, it's not very useful if it's not enabled by default, but enabling it by default means adding a new runtime dependency. However, considering the number of symbols, I don't see why we shouldn't enable by default and dlopen/dlsym them...

Comment 21

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

(In reply to Mike Hommey [:glandium] from comment #20)
> That said, it's not very useful if it's not enabled by default, but enabling
> it by default means adding a new runtime dependency. However, considering
> the number of symbols, I don't see why we shouldn't enable by default and
> dlopen/dlsym them...

This may well be worth considering, though there are some questions if we're using it by default about stability/compatibility/security and if there are any perf issues (extra buffer copies/conversions, etc).  Likely it's all ok or a win, but there's certainly some added code and complexity for maybe-loading a library.

If we do this, is there a minimum version we should support (due to API changes or sec issue)?

I'd be good with landing this and filing a bug to convert it to dlsym (and review if this is good as a default).

Comment 23

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.

Comment 24

[Jan Beich]

JPEG is supported since bug 880879. Rebasing is complicated since bug 1393119. Let's drop.

https://chromium.googlesource.com/external/webrtc/+/6b6eb44cca%5E!/