Support more video formats with WebRTC on Linux

RESOLVED WORKSFORME

Status

()

P5
normal
Rank:
45
RESOLVED WORKSFORME
6 years ago
16 days ago

People

(Reporter: jbeich, Assigned: jbeich)

Tracking

Trunk
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 698245 [details] [diff] [review]
wrap open/ioctl/etc, tested by a FreeBSD user with JPEG cam
Attachment #698245 - Flags: review?(rjesup)
(Assignee)

Comment 2

6 years ago
s/tested/requested/
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?
Attachment #698245 - Flags: review?(rjesup)
Attachment #698245 - Flags: review-
Attachment #698245 - Flags: feedback?(ted)

Updated

6 years ago
Assignee: nobody → jbeich
Priority: -- → P3
Whiteboard: [webrtc][blocking-webrtc-]
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.
Attachment #698245 - Flags: feedback?(ted)

Updated

6 years ago
OS: FreeBSD → Linux

Comment 5

6 years ago
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
Summary: Support more cameras with WebRTC on Linux → Support more video formats with WebRTC on Linux
(Assignee)

Comment 6

6 years ago
Created attachment 713375 [details] [diff] [review]
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.
Attachment #698245 - Attachment is obsolete: true
Attachment #713375 - Flags: review?(rjesup)
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
Attachment #713375 - Flags: review?(ted)
Attachment #713375 - Flags: review?(rjesup)
Attachment #713375 - Flags: review+
Depends on: 864513
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.
Attachment #713375 - Flags: review?(ted) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 795112 [details] [diff] [review]
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.
Attachment #713375 - Attachment is obsolete: true
Attachment #795112 - Flags: review?(ted)
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 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.
Attachment #795112 - Flags: review?(ted) → review-
(Assignee)

Comment 12

5 years ago
Created attachment 8362336 [details] [diff] [review]
wrap open/ioctl/etc with ifdef, v2 (as --enable-libv4l2 option)
Attachment #795112 - Attachment is obsolete: true
Attachment #8362336 - Flags: review?(ted)
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.
Attachment #8362336 - Flags: review?(ted) → review+
Blocks: 864513
No longer depends on: 864513
backlog: --- → webRTC+
Rank: 45
Priority: P3 → P4
Whiteboard: [webrtc][blocking-webrtc-]
(Assignee)

Comment 14

3 years ago
Created attachment 8750854 [details]
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/
Attachment #8750854 - Flags: review?(rjesup)
Attachment #8750854 - Flags: review?(mh+mozilla)
(Assignee)

Comment 15

3 years ago
(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) <ted@mielczarek.org> 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. chromium@freebsd.org 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.
(Assignee)

Updated

3 years ago
Attachment #8362336 - Attachment is obsolete: true
(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) <ted@mielczarek.org> 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.
Flags: needinfo?(juberti)
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
Attachment #8750854 - Flags: review?(rjesup) → review+

Comment 18

3 years ago
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.
(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.
Flags: needinfo?(jbeich)

Updated

3 years ago
Flags: needinfo?(juberti)
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...
Attachment #8750854 - Flags: review?(mh+mozilla)
(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).

Updated

3 years ago
See Also: → bug 1272275

Updated

3 years ago
Duplicate of this bug: 1272275

Updated

3 years ago
See Also: bug 1272275
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
(Assignee)

Comment 24

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

https://chromium.googlesource.com/external/webrtc/+/6b6eb44cca%5E!/
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Flags: needinfo?(jbeich)
Resolution: --- → WORKSFORME

Updated

16 days ago
No longer blocks: 864513
You need to log in before you can comment on or make changes to this bug.