After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 725860 - v4l2src: Fix using v4l2src with Hauppauge HDPVR video capture device
v4l2src: Fix using v4l2src with Hauppauge HDPVR video capture device
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.2.3
Other Linux
: Normal normal
: 1.2.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-03-06 22:55 UTC by Will Manley
Modified: 2014-04-02 21:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
v4l2src: Fix support for mpegts streams (1.49 KB, patch)
2014-03-06 22:56 UTC, Will Manley
none Details | Review
v4l2src: Only care about getting correct width/height if in caps (3.54 KB, patch)
2014-03-06 22:57 UTC, Will Manley
needs-work Details | Review
strace output from gst-launch with patches applied (15.23 KB, text/plain)
2014-03-06 23:40 UTC, Will Manley
  Details
v4l2src: Fix support for mpegts streams (1.89 KB, patch)
2014-03-07 01:13 UTC, Will Manley
committed Details | Review
Fix support for caps without width, height, framerate and format (4.00 KB, patch)
2014-03-29 23:15 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Will Manley 2014-03-06 22:55:39 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.
Comment 1 Will Manley 2014-03-06 22:56:16 UTC
Created attachment 271150 [details] [review]
v4l2src: Fix support for mpegts streams
Comment 2 Will Manley 2014-03-06 22:57:05 UTC
Created attachment 271151 [details] [review]
v4l2src: Only care about getting correct width/height if in caps
Comment 3 Will Manley 2014-03-06 22:58:49 UTC
Also: would these be considered for inclusion in 1.2.4 stable release?
Comment 4 Tim-Philipp Müller 2014-03-06 23:21:26 UTC
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)
Comment 5 Will Manley 2014-03-06 23:40:12 UTC
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/
Comment 6 Will Manley 2014-03-07 01:06:36 UTC
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.
Comment 7 Will Manley 2014-03-07 01:13:10 UTC
Created attachment 271165 [details] [review]
v4l2src: Fix support for mpegts streams

New patch attached.
Comment 8 Nicolas Dufresne (ndufresne) 2014-03-07 12:56:56 UTC
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.
Comment 9 Nicolas Dufresne (ndufresne) 2014-03-07 12:59:09 UTC
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 ?
Comment 10 Nicolas Dufresne (ndufresne) 2014-03-07 13:00:46 UTC
(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.
Comment 11 Will Manley 2014-03-07 14:54:42 UTC
(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
Comment 12 Will Manley 2014-03-07 15:14:43 UTC
(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?
Comment 13 Nicolas Dufresne (ndufresne) 2014-03-07 16:48:54 UTC
(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.
Comment 14 Will Manley 2014-03-07 17:42:12 UTC
(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
Comment 15 Nicolas Dufresne (ndufresne) 2014-03-07 18:03:00 UTC
(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.
Comment 16 Will Manley 2014-03-12 11:37:10 UTC
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/
Comment 17 Will Manley 2014-03-25 12:37:21 UTC
Bump?
Comment 18 Nicolas Dufresne (ndufresne) 2014-03-25 13:35:24 UTC
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 19 Nicolas Dufresne (ndufresne) 2014-03-29 21:25:43 UTC
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+.
Comment 20 Nicolas Dufresne (ndufresne) 2014-03-29 21:51:51 UTC
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.
Comment 21 Nicolas Dufresne (ndufresne) 2014-03-29 21:51:52 UTC
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.
Comment 22 Nicolas Dufresne (ndufresne) 2014-03-29 23:15:15 UTC
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.
Comment 23 Nicolas Dufresne (ndufresne) 2014-04-01 13:25:34 UTC
Ping
Comment 24 Will Manley 2014-04-01 15:32:07 UTC
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.
Comment 25 Will Manley 2014-04-01 15:45:44 UTC
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);
Comment 26 Will Manley 2014-04-01 17:01:25 UTC
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.
Comment 27 Nicolas Dufresne (ndufresne) 2014-04-01 17:05:26 UTC
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.
Comment 28 Will Manley 2014-04-01 17:06:21 UTC
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.
Comment 29 Will Manley 2014-04-01 17:19:17 UTC
(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
Comment 30 Nicolas Dufresne (ndufresne) 2014-04-01 17:33:34 UTC
(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.
Comment 31 Olivier Crête 2014-04-01 17:44:24 UTC
I think the use-case for Youness was the C920 producing H.264, not an elusive mpeg-ts camera.
Comment 32 Nicolas Dufresne (ndufresne) 2014-04-01 20:00:38 UTC
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+.