GNOME Bugzilla – Bug 723446
v4l2src: Should detect support for mplanar formats during runtime
Last modified: 2014-03-28 19:37:13 UTC
Created attachment 267816 [details] [review] patch 1/2: Remove an obsolete section from configure.ac (Our shop supports the two most recent Ubuntu LTS releases, but uses bleeding edge gstreamer, so we still care about ubuntu 10.04 for a few more months.) Since commit http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/sys/v4l2/gstv4l2bufferpool.h?id=61ae84b50dc319a53b513e5474c1578d7127c67c (which fixed bug 712754 ), gst-plugins-good fails to build on Ubuntu 10.04 because symbol struct v4l2_plane is defined /usr/include/linux/videodev2.h from package linux-libc-dev in Ubuntu 12.04, but not in Ubuntu 10.04. Here's a two part series that addresses the build breakage by skipping v4l2 if the header is too old, and removes a related but obsolete section in configure.ac.
Created attachment 267817 [details] [review] patch 2/2: update header up-to-date check for v4l2
Please fix the first patch that removes the obsolete check so that things still build after it (it looks like the second patches fixes up the superfluous 'fi' then).
Created attachment 267861 [details] [review] patch 1/2: Remove an obsolete section from configure.ac (try 2) That *was* crap, wasn't it? This should look better.
Created attachment 267862 [details] [review] patch 2/2: update header up-to-date check for v4l2 (try 2)
Comment on attachment 267862 [details] [review] patch 2/2: update header up-to-date check for v4l2 (try 2) Nope, this breaks here on Debian/unstable: configure:29842: checking linux/videodev2.h usability configure:29842: gcc-4.8 -std=gnu99 -c -g -O2 conftest.c >&5 configure:29842: $? = 0 configure:29842: result: yes configure:29842: checking linux/videodev2.h presence configure:29842: cpp-4.8 conftest.c configure:29842: $? = 0 configure:29842: result: yes configure:29842: checking for linux/videodev2.h configure:29842: result: yes configure:29859: WARNING: video4linux2 headers were found, but they're old. configure:29861: WARNING: Please update v4l2 to compile the v4l2 plugins configure:29996: *** These plugins will not be built: v4l2src
Problem is that the test program created is broken, note the first line: | HAVE_V4L2_PLANE=yes | | int | main () | { | if (sizeof (struct v4l2_plane)) | return 0; | ; | return 0; | }
Comment on attachment 267861 [details] [review] patch 1/2: Remove an obsolete section from configure.ac (try 2) commit 99bf36b55d33de8c46fc4722540297417b6b72c6 Author: Dan Kegel <dank@kegel.com> Date: Sun Feb 2 09:57:03 2014 -0800 v4l2: Remove obsolete definition GST_V4L2_MISSING_BUFDECL The only use was removed by 9edc0c0365f79ab07ff2e65461c6696e3931a3f0 https://bugzilla.gnome.org/show_bug.cgi?id=723446
*** Bug 724045 has been marked as a duplicate of this bug. ***
Dan, any plans to update your patch?
Yes, I should get to it in the next couple days when I update to 1.2.3.
Created attachment 269942 [details] [review] 3rd try. AC_CHECK_TYPE argument order now correct. Build verified on ubuntu 10.04 against g62f5a27. Patch with AC_CHECK_TYPE usage corrected (thanks for the review!)
Comment on attachment 269942 [details] [review] 3rd try. AC_CHECK_TYPE argument order now correct. Build verified on ubuntu 10.04 against g62f5a27. So this would only allow building the v4l2 plugin if "struct v4l2_plane" is available, right? Can we maybe just simplify all these checks a bit more to just check if linux/videodev2.h works, has a non-ancient version and struct v4l2_plane is available? The current checks seem just too convoluted...
So you'd prefer not issuing the "Please update v4l2 to compile the v4l2 plugins" warning, and just saying "video4linux2 was not found" for both "v4l not found" and "v4l too old"?
I think the goal is to make the v4l2_plane stuff out-compilable, or we could just copy paste that part of the header, so we keep the code clean. Probing will tell us if mplane is actually supported or not. Note that I've seen a lot of V4L2 based project that just copy the latest header in their project, and completely rely on probing for the rest. This would cleanup the code from all the ifdef, but may require some review of the error handling.
I think I'm happy with my current patch for now; it seems correct, and I'm not sure what the right path is to improve it.
(In reply to comment #14) > I think the goal is to make the v4l2_plane stuff out-compilable, or we could > just copy paste that part of the header, so we keep the code clean. Probing > will tell us if mplane is actually supported or not. > > Note that I've seen a lot of V4L2 based project that just copy the latest > header in their project, and completely rely on probing for the rest. This > would cleanup the code from all the ifdef, but may require some review of the > error handling. Nicolas, will you put that on your list and make that work before 1.4? I'll include this patch for the time being.
Ok.
Ok, just experimenting, but I made a patch where we not longer depend on system installed v4l2 headers, let's me know what you think, I still need to cleanup the configure.ac. http://cgit.collabora.com/git/user/nicolas/gst-plugins-good.git/commit/?h=v4l2-cleanup&id=f58f1a3a9ffca07f1bc4b67e528d5eda32ff38c5
This should fix the compilation issues. Let me know if some improvement is needed on top of that. commit 418a4940a8c41411cef33a0054879cbd4c9bd181 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Sat Mar 15 11:13:05 2014 +0100 v4l2: Use a copy of videodev2.h header With years the amount of ifdef have grown up and we are not even sure if the old code path compiles. Each time we need to update the v4l2 framework to add the new feature, we break compilation on older kernel. With exception of two controls in the video orientation control, this patch get rid of all ifdef by including the latest version of videodev2.h inside GStreamer. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=723446
Ping, does it work for you now ?
I'm probably unable to try it for a few weeks (we are entering code freeze at the moment).
n FreeBSD so andI'll assume it works. The offending check is completely gone. If anything, feel free to reopen.