GNOME Bugzilla – Bug 725860
v4l2src: Fix using v4l2src with Hauppauge HDPVR video capture device
Last modified: 2014-04-02 21:02:12 UTC
The attached patches fix a regression preventing using the Hauppauge HDPVR with GStreamer 1.0+. Specifically systemstream=true is added to mpegts caps allowing linking to tsdemux and GStreamer no longer gets confused if it doesn't have width/height v4l2src's src caps. On a tangent: is there any strategy for writing tests for v4l2 support? I'd quite like to come up with test cases for these issues but I don't want to go to all the effort of mocking out the whole V4L2 API.
Created attachment 271150 [details] [review] v4l2src: Fix support for mpegts streams
Created attachment 271151 [details] [review] v4l2src: Only care about getting correct width/height if in caps
Also: would these be considered for inclusion in 1.2.4 stable release?
Both look like they could be picked into 1.2.4. Out of curiosity: why doesn't the driver return the right size during probing, so that we're requesting the right size from the start? (rather than 0x0)
Created attachment 271159 [details] strace output from gst-launch with patches applied That's a good question. Looking at the strace output it does seem to return width and height information correctly. I've attached the strace output generated from running: strace -f -e ioctl gst-launch-1.0 v4l2src device=/dev/video1 ! queue \ ! tsdemux ! video/x-h264 ! decodebin ! xvimagesink With an strace with v4l2 decoding added[1]. I'll have a look through the code. [1]: http://sourceforge.net/p/strace/mailman/message/32060311/
Ah, the reason is in `gst_v4l2_object_probe_caps_for_format` we see: if (pixelformat == GST_MAKE_FOURCC ('M', 'P', 'E', 'G')) return gst_caps_new_empty_simple ("video/mpegts"); and so it never even bothers going through the finding out the size dance. I guess this sort-of makes sense, maybe the mpeg stream contains multiple video streams, or they come and go. Maybe the video stream could change resolution half way through. I'll upload a new patch with systemstream=true added to these caps too for good measure.
Created attachment 271165 [details] [review] v4l2src: Fix support for mpegts streams New patch attached.
Review of attachment 271151 [details] [review]: This is a bit obfuscated. Basically you have an encoded format, or in this case a container. What I'd prefer is to see a special case for that, rather then checking for value set to zero. In GstVideoFormatInfo you get a format type ENCODED when this is the case.
Review of attachment 271165 [details] [review]: That seems fair, though is this how MPEG support is defined in V4L2 or is this only this way for this driver ?
(In reply to comment #5) > Created an attachment (id=271159) [details] > strace output from gst-launch with patches applied > > That's a good question. Looking at the strace output it does seem to return > width and height information correctly. > > I've attached the strace output generated from running: > > strace -f -e ioctl gst-launch-1.0 v4l2src device=/dev/video1 ! queue \ > ! tsdemux ! video/x-h264 ! decodebin ! xvimagesink > > With an strace with v4l2 decoding added[1]. I'll have a look through the code. > > [1]: http://sourceforge.net/p/strace/mailman/message/32060311/ Idelly GST_DEBUG="v4l2*:6" will tell us more.
(In reply to comment #9) > Review of attachment 271165 [details] [review]: > > That seems fair, though is this how MPEG support is defined in V4L2 or is this > only this way for this driver ? The v4l2 documentation[1] says: > Identifier Code Details > > V4L2_PIX_FMT_MPEG 'MPEG' MPEG stream. The actual format is determined by > extended control V4L2_CID_MPEG_STREAM_TYPE, see > Table 1.2, “MPEG Control IDs”. Table 1.2 in turn says: > V4L2_CID_MPEG_STREAM_TYPE enum v4l2_mpeg_stream_type > The MPEG-1, -2 or -4 output stream type. One cannot assume anything here. > Each hardware MPEG encoder tends to support different subsets of the > available MPEG stream types. The currently defined stream types are: > > V4L2_MPEG_STREAM_TYPE_MPEG2_PS MPEG-2 program stream > V4L2_MPEG_STREAM_TYPE_MPEG2_TS MPEG-2 transport stream > V4L2_MPEG_STREAM_TYPE_MPEG1_SS MPEG-1 system stream > V4L2_MPEG_STREAM_TYPE_MPEG2_DVD MPEG-2 DVD-compatible stream > V4L2_MPEG_STREAM_TYPE_MPEG1_VCD MPEG-1 VCD-compatible stream > V4L2_MPEG_STREAM_TYPE_MPEG2_SVCD MPEG-2 SVCD-compatible stream So currently GStreamer doesn't check V4L2_CID_MPEG_STREAM_TYPE and just assumes V4L2_MPEG_STREAM_TYPE_MPEG2_TS. As far as I can see "video/mpegts,systemstream=true" is how mpegts is represented in terms of GStreamer caps. I do find this confusing as systemstream=true doesn't seem to confer any additional information. I would happy to be corrected by someone who knows better though. This systemstream thing has been puzzling me. IOW the v4l2 spec doesn't specify any way of having "video/mpegts,systemstream=false" because the concept only seems to exist in GStreamer AFAICS and doesn't correspond to any real video or container format. Therefore it has to be systemstream=true. [1]: http://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single/v4l2.html#id2809728
(In reply to comment #8) > Review of attachment 271151 [details] [review]: > > This is a bit obfuscated. Basically you have an encoded format, or in this case > a container. What I'd prefer is to see a special case for that, rather then > checking for value set to zero. In GstVideoFormatInfo you get a format type > ENCODED when this is the case. Agreed, it's not beautiful, but I'm not certain that checking for an encoded format would be appropriate either. For instance I have another V4L2 device - a logitech C920 webcam - which produces h264 encoded video. Unlike the HDPVR which is a video-capture device it can choose what height/width video it will produce. In that case you want to be insisting that the camera produces video with the width/height set in the caps. So in some cases we care about what resolution video will be produced and in other cases we don't. We express this via caps. With caps "video/x-h264,height=1280,width=720" we want to produce an error if the camera won't negotiate the appropriate height/width. With caps just "video/x-h264" we shouldn't error no matter what height/width we are given. This is what this patch does but I admit it does go about it in a bit of a roundabout way and a solution which removes the mpeg special casing: if (pixelformat == GST_MAKE_FOURCC ('M', 'P', 'E', 'G')) return gst_caps_new_empty_simple ("video/mpegts"); from gst_v4l2_object_probe_caps_for_format might be better. What would you suggest?
(In reply to comment #11) > (In reply to comment #9) > > Review of attachment 271165 [details] [review] [details]: > > > > That seems fair, though is this how MPEG support is defined in V4L2 or is this > > only this way for this driver ? > > The v4l2 documentation[1] says: > > > Identifier Code Details > > > > V4L2_PIX_FMT_MPEG 'MPEG' MPEG stream. The actual format is determined by > > extended control V4L2_CID_MPEG_STREAM_TYPE, see > > Table 1.2, “MPEG Control IDs”. > > Table 1.2 in turn says: > > > V4L2_CID_MPEG_STREAM_TYPE enum v4l2_mpeg_stream_type > > The MPEG-1, -2 or -4 output stream type. One cannot assume anything here. > > Each hardware MPEG encoder tends to support different subsets of the > > available MPEG stream types. The currently defined stream types are: > > > > V4L2_MPEG_STREAM_TYPE_MPEG2_PS MPEG-2 program stream > > V4L2_MPEG_STREAM_TYPE_MPEG2_TS MPEG-2 transport stream > > V4L2_MPEG_STREAM_TYPE_MPEG1_SS MPEG-1 system stream > > V4L2_MPEG_STREAM_TYPE_MPEG2_DVD MPEG-2 DVD-compatible stream > > V4L2_MPEG_STREAM_TYPE_MPEG1_VCD MPEG-1 VCD-compatible stream > > V4L2_MPEG_STREAM_TYPE_MPEG2_SVCD MPEG-2 SVCD-compatible stream > > So currently GStreamer doesn't check V4L2_CID_MPEG_STREAM_TYPE and just assumes > V4L2_MPEG_STREAM_TYPE_MPEG2_TS. > > As far as I can see "video/mpegts,systemstream=true" is how mpegts is > represented in terms of GStreamer caps. I do find this confusing as > systemstream=true doesn't seem to confer any additional information. I would > happy to be corrected by someone who knows better though. This systemstream > thing has been puzzling me. > > IOW the v4l2 spec doesn't specify any way of having > "video/mpegts,systemstream=false" because the concept only seems to exist in > GStreamer AFAICS and doesn't correspond to any real video or container format. > Therefore it has to be systemstream=true. > > [1]: > http://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single/v4l2.html#id2809728 Indeed, all agreed, thanks for taking time to explain. (In reply to comment #12) > (In reply to comment #8) > > Review of attachment 271151 [details] [review] [details]: > > > > This is a bit obfuscated. Basically you have an encoded format, or in this case > > a container. What I'd prefer is to see a special case for that, rather then > > checking for value set to zero. In GstVideoFormatInfo you get a format type > > ENCODED when this is the case. > > from gst_v4l2_object_probe_caps_for_format might be better. What would you > suggest? Sorry, I should have proposed a check if it's a container indeed. Encoded format don't indeed comes with widht/height. We have a list of formats, with a type, currently RAW and ECNODED. We should mark container as well and skip some part base on that. This all need a lot of cleanup, there use to be hacks here and there for mpegts, I think it's time to unhack this a bit. Except that for 1.2 branch, a smaller patch like you proposed might be adequate. I'll need to think about it a bit, will come back on that soon.
(In reply to comment #13) > I think it's time to unhack this a bit. Agreed, but I wouldn't have any confidence making bigger changes without some sort of test suite. Maybe something like [umockdev] would be appropriate. I'll have to have a play around with it. [umockdev]: https://github.com/martinpitt/umockdev
(In reply to comment #14) > (In reply to comment #13) > > I think it's time to unhack this a bit. > > Agreed, but I wouldn't have any confidence making bigger changes without some > sort of test suite. Maybe something like [umockdev] would be appropriate. > I'll have to have a play around with it. > > [umockdev]: https://github.com/martinpitt/umockdev The thing with v4l2 is that you really want to test against a driver using the videobuf framework. I would rather look int he direction vivi has taken. But we are far from being able to do real testing yet there. Another way around would be to implement simulation through something similar to libv4l2, which is fully supported in GStreamer. Though, this is out of scope for this bug report. It's also a lot of work, so unless someone is sponsoring it, it might not happen.
So what's the next step here? My interest in having this merged is that I'm trying to port stb-tester[1] from 0.10 to 1.0 to avoid disappointed looks from Tim at next years GStreamer conference. Many of our users use the HD-PVR so this is fairly important to me. So: what can I do to help get this merged? [1]: http://stb-tester.com/
Bump?
So, basically we have this format description table, with flags for each formats, RAW, CODEC and TRANSPORT. I think we should skip size check for transport (well probably skip everything). Though we should make sure to request a decent buffer size like we already do now with CODEC. I have been very busy with some stabilization branch lately, but I didn't forgot about the hd-pvr support (thouhg I don't own one).
Comment on attachment 271165 [details] [review] v4l2src: Fix support for mpegts streams Merge in master and 1.2, the commit in 1.2, sorry forgot to reference the bug :-S commit a403256039799c9b8b0b9b4c9fe3f011d301ad1c Author: William Manley <will@williammanley.net> Date: Thu Mar 6 19:52:36 2014 +0000 v4l2src: Fix support for mpegts streams It seems that GStreamer's mpegts elements (tsdemux, tsparse) require caps `video/mpegts,systemstream=true`. As far as I can see the significance of systemstream is to indicate that this is a container format rather than an elementary stream. As this is the case (and I can't understand how it could not be the case with mpegts) I add systemstream=true to v4l2src's caps. This allows v4l2src to be linked with tsdemux for playback from my Hauppauge HD-PVR with the pipeline: v4l2src ! queue ! tsdemux ! video/x-h264 ! decodebin ! xvimagesink In combination with the next commit this fixes using Hauppauge HD-PVR with GStreamer 1.0+.
Review of attachment 271151 [details] [review]: Here's more instruction, note that some changes has appeared upstream, so this all need updating. Do you have time to rework it ? ::: sys/v4l2/gstv4l2object.c @@ +2392,3 @@ if (format.type != v4l2object->type || + (width != 0 && format.fmt.pix.width != width) || + (height != 0 && format.fmt.pix.height != height) || Ok, I know what to do instead. So instead of doing that, if size is not set the caps then set: format.fmt.pix.width = width; format.mt.pix.height = height; This way it less obfuscated, and should work. @@ +2417,3 @@ + if ((width != 0 && format.fmt.pix.width != width) || + (height != 0 && format.fmt.pix.height != height)) Will then not be needed. ::: sys/v4l2/gstv4l2src.c @@ +284,3 @@ + G_MAXINT, 1); + if (gst_structure_has_field (structure, "format")) + gst_structure_fixate_field (structure, "format"); This though is totally needed, we should not introduce new field when fixating.
Created attachment 273261 [details] [review] Fix support for caps without width, height, framerate and format Please test this patch against git master, it should solve the problem you are having with your MPEG source. I'll wait for result, and will backport it to 1.2.
Ping
So I've been trying to reproduce this issue with git master (but without your patch) but have been unable to. With git master@b95d9cfb: $ gst-launch-1.0 v4l2src device=/dev/v4l/by-id/usb-AMBA_Hauppauge_HD_PVR_00A776F7-video-index0 ! fakesink Setting pipeline to PAUSED ... Pipeline is live and does not need PREROLL ... Setting pipeline to PLAYING ... New clock: GstSystemClock (gst-launch-1.0:26567): GStreamer-CRITICAL **: gst_structure_fixate_field_nearest_int: assertion 'gst_structure_has_field (structure, field_name)' failed (gst-launch-1.0:26567): GStreamer-CRITICAL **: gst_structure_fixate_field_nearest_int: assertion 'gst_structure_has_field (structure, field_name)' failed (gst-launch-1.0:26567): GStreamer-CRITICAL **: gst_structure_fixate_field_nearest_fraction: assertion 'gst_structure_has_field (structure, field_name)' failed And data flows. Whereas with 1.2.3 from Debian testing I get: $ /usr/bin/gst-launch-1.0 v4l2src device=/dev/v4l/by-id/usb-AMBA_Hauppauge_HD_PVR_00A776F7-video-index0 ! fakesink Setting pipeline to PAUSED ... Pipeline is live and does not need PREROLL ... Setting pipeline to PLAYING ... New clock: GstSystemClock (gst-launch-1.0:26579): GStreamer-CRITICAL **: gst_structure_fixate_field_nearest_int: assertion 'gst_structure_has_field (structure, field_name)' failed (gst-launch-1.0:26579): GStreamer-CRITICAL **: gst_structure_fixate_field_nearest_int: assertion 'gst_structure_has_field (structure, field_name)' failed (gst-launch-1.0:26579): GStreamer-CRITICAL **: gst_structure_fixate_field_nearest_fraction: assertion 'gst_structure_has_field (structure, field_name)' failed ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Device '/dev/v4l/by-id/usb-AMBA_Hauppauge_HD_PVR_00A776F7-video-index0' cannot capture at 0x0 Additional debug info: gstv4l2object.c(2539): gst_v4l2_object_set_format (): /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Tried to capture at 0x0, but device returned size 1280x720 Execution ended after 0:00:00.154631319 Setting pipeline to PAUSED ... Setting pipeline to READY ... Setting pipeline to NULL ... Freeing pipeline ... This fails. With GStreamer master + your patch: $ gst-launch-1.0 v4l2src device=/dev/v4l/by-id/usb-AMBA_Hauppauge_HD_PVR_00A776F7-video-index0 ! fakesink Setting pipeline to PAUSED ... Pipeline is live and does not need PREROLL ... Setting pipeline to PLAYING ... New clock: GstSystemClock e.g. the GStreamer-CRITICAL goes away but otherwise it behaves the same as git master. So I guess some other change has "fixed" this issue. I'll investigate.
Ah, 09dabd4 seems interesting: commit 09dabd4be60c1f0a2e3132a23abcfa97d544b800 Author: Youness Alaoui <youness.alaoui@collabora.co.uk> Date: Thu Apr 19 08:27:01 2012 +0000 v4l2src: Allow mpeg-ts cameras to negociate format This removes an ugly hack until the reason for the hack can be documented diff --git a/sys/v4l2/gstv4l2object.c b/sys/v4l2/gstv4l2object.c index 74d1e83..92df04f 100644 --- a/sys/v4l2/gstv4l2object.c +++ b/sys/v4l2/gstv4l2object.c @@ -2235,10 +2235,16 @@ gst_v4l2_object_set_format (GstV4l2Object * v4l2object, GstCaps * caps) GST_V4L2_CHECK_OPEN (v4l2object); GST_V4L2_CHECK_NOT_ACTIVE (v4l2object); + /* MPEG-TS source cameras don't get their format set for some reason. + * It looks wrong and we weren't able to track down the reason for that code + * so it is disabled until someone who has an mpeg-ts camera complains... + */ +#if 0 /* Only unconditionally accept mpegts for sources */ if ((v4l2object->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) && (pixelformat == GST_MAKE_FOURCC ('M', 'P', 'E', 'G'))) goto done; +#endif memset (&format, 0x00, sizeof (struct v4l2_format)); format.type = v4l2object->type; diff --git a/sys/v4l2/v4l2src_calls.c b/sys/v4l2/v4l2src_calls.c index fa9a55c..1104d70 100644 --- a/sys/v4l2/v4l2src_calls.c +++ b/sys/v4l2/v4l2src_calls.c @@ -215,8 +215,14 @@ gst_v4l2src_set_capture (GstV4l2Src * v4l2src, guint32 pixelformat, gint fd = v4l2src->v4l2object->video_fd; struct v4l2_streamparm stream; + /* MPEG-TS source cameras don't get their format set for some reason. + * It looks wrong and we weren't able to track down the reason for that code + * so it is disabled until someone who has an mpeg-ts camera complains... + */ +#if 0 if (pixelformat == GST_MAKE_FOURCC ('M', 'P', 'E', 'G')) return TRUE; +#endif g_signal_emit_by_name (v4l2src, "prepare-format", v4l2src->v4l2object->video_fd, pixelformat, width, height);
Right, so this makes sense in the context of Comment 4 and Comment 6: Tim in Comment 4 said: > why doesn't the driver return the right size during probing, > so that we're requesting the right size from the start? (rather than 0x0) Will in Comment 6 said: > Ah, the reason is in `gst_v4l2_object_probe_caps_for_format` we see: > > if (pixelformat == GST_MAKE_FOURCC ('M', 'P', 'E', 'G')) > return gst_caps_new_empty_simple ("video/mpegts"); > > and so it never even bothers going through the finding out the size dance. I > guess this sort-of makes sense, maybe the mpeg stream contains multiple video > streams, or they come and go. Maybe the video stream could change resolution > half way through. So the reason it's working is that in 09dabd4b Youness Alaoui removed the code that was preventing probing. I've confirmed this theory by reverting 09dabd4b locally. The failure mode is different to version 1.2.3 but this confirms that 09dabd4b fixes the issue. The patch is definitely still valuable as it fixes the GStreamer-CRITICALs associated with fixating missing parameters in the caps.
Ok. this was fixed partially in master by: commit b80169a16abb8526e5d766fa3e7cc04c6667db3e Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon Dec 2 15:26:50 2013 -0500 v4l2object: Don't validate dimension for encoded format We set the dimensions just in case but don't validate them afterwards. For some codecs the dimensions are *not* in the bitstream, IIRC VC1 in ASF mode for example. https://bugzilla.gnome.org/show_bug.cgi?id=720568 and then this latest patches fixes few criticals, and avoid reconfiguring the devices if it's already configured properly. Thanks for taking the time to test all this. I'll see if I can backport all this in stable branch.
I've just done a bisect as well. It seems it started working after b80169a1: "v4l2object: Don't validate dimension for encoded format", which is a patch which you (Nicolas) authored as part of your M2M stuff. So the fact that this is working on master depends on both 09dabd4b and b80169a being applied.
(In reply to comment #27) > Ok. this was fixed partially in master by: > > commit b80169a16abb8526e5d766fa3e7cc04c6667db3e > Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Ah, you beat me to it :). Thanks for all your efforts here, it's good to get to the bottom of this. When I find some time I'll see what I can do WRT writing some automated tests for this stuff. Once this is in I'll move on to chasing some distro package maintainers to see if they will include it. Then hopefully this stuff will "just work" for my users :). Thanks again Will
(In reply to comment #4) > Both look like they could be picked into 1.2.4. > > Out of curiosity: why doesn't the driver return the right size during probing, > so that we're requesting the right size from the start? (rather than 0x0) The 0x0 is from GStreamer, not the driver. The driver report the size correctly, but GStreamer negotiated caps has no width/height field. These field will be added by the demuxer later. Previous hack was bad, because it would ignore manually added caps filter with width/height/framerate field, hence prevent requesting precise parameters from cameras producing MPEG-TS (Youness use case at the time). The HDPVR case is a bit special and its probably not fully supported in GStreamer. E.g. the encoding can be changed live by issuing decoder stop command, setup, decoder start command, the size is driven by the settings rather then s_fmt, etc. So I'm pretty happy to hear is works, at least in default settings.
I think the use-case for Youness was the C920 producing H.264, not an elusive mpeg-ts camera.
Git master: commit e5ef2db489799d4bbf2da3ccbdf72d2ef73bf4ed Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Sat Mar 29 19:13:06 2014 -0400 v4l2: Fix support for caps without width, height, framerate or format For format like mpegts, width and height is rarely in the negotiated caps. This patch fixes failure when setting format, and prevent introducing width, height, framerate and format to the caps when fixating. https://bugzilla.gnome.org/show_bug.cgi?id=725860 For 1.2 branch (for packager, this is the full list): commit 9eb8e5976c6af57b29c3727a69fb800ac415ca4d Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon Dec 2 15:26:50 2013 -0500 v4l2object: Don't validate dimension for encoded format We set the dimensions just in case but don't validate them afterwards. For some codecs the dimensions are *not* in the bitstream, IIRC VC1 in ASF mode for example. https://bugzilla.gnome.org/show_bug.cgi?id=720568 Conflicts: sys/v4l2/gstv4l2object.c commit 03dddd6e7f171db33cfcd7a975a2da976e0fc1f0 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Sat Mar 29 19:13:06 2014 -0400 v4l2: Fix support for caps without width, height, framerate or format For format like mpegts, width and height is rarely in the negotiated caps. This patch fixes failure when setting format, and prevent introducing width, height, framerate and format to the caps when fixating. https://bugzilla.gnome.org/show_bug.cgi?id=725860 Conflicts: sys/v4l2/gstv4l2object.c commit a403256039799c9b8b0b9b4c9fe3f011d301ad1c Author: William Manley <will@williammanley.net> Date: Thu Mar 6 19:52:36 2014 +0000 v4l2src: Fix support for mpegts streams It seems that GStreamer's mpegts elements (tsdemux, tsparse) require caps `video/mpegts,systemstream=true`. As far as I can see the significance of systemstream is to indicate that this is a container format rather than an elementary stream. As this is the case (and I can't understand how it could not be the case with mpegts) I add systemstream=true to v4l2src's caps. This allows v4l2src to be linked with tsdemux for playback from my Hauppauge HD-PVR with the pipeline: v4l2src ! queue ! tsdemux ! video/x-h264 ! decodebin ! xvimagesink In combination with the next commit this fixes using Hauppauge HD-PVR with GStreamer 1.0+.