[Bug 726194] v4l2src does not cope well when a capture card is sometimes interlaced, sometimes progressive at the same resolution

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=726194
  GStreamer | gst-plugins-good | git

Nicolas Dufresne (stormer) <nicolas.dufresne> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #289687|none                        |needs-work
             status|                            |

--- Comment #9 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> 2014-10-30 19:29:22 UTC ---
Review of attachment 289687:
 --> (https://bugzilla.gnome.org/review?bug=726194&attachment=289687)

It overall looks very good, except that for the mixed case, which I think does
not really exist in V4L2. I've poked Hans Verkuil, he should be able to confirm
this by tomorrow. By fixing the notion of ANY, we should also do our first
TRY_FMT with ANY, which removes the need for a progressive fallback. Finally,
some more cleanup is needed to get rid of all the copy paste in this patch, but
you seem to have nailed the main idea.

Some context about TRY_FMT (fmt.feild = ANY). The driver that support more then
1 interlacing mode have no choice but to return ANY, since "enum v4l2_field" is
an enum, not a bit mask. That would explain why "mixed" is always present in
your test result. By initially using ANY, we should be certain that a failure
means that the format is not supported.

::: sys/v4l2/gstv4l2object.c
@@ +1634,3 @@
+  switch (field) {
+    case V4L2_FIELD_ANY:
+      *interlace_mode = GST_VIDEO_INTERLACE_MODE_MIXED;

This ANY field isn't documented as GStreamer "mixed" mode anyway. Also from you
test case, whenever you give ANY to TRY_FMT you seem to receive ANY. I'm
thinking this mode does not exist in V4L2, and that we should only probe for
specific mode, and never ANY.

@@ +1653,3 @@
+gst_v4l2_object_add_interlace_mode (GstV4l2Object * v4l2object,
+    GstStructure * s, guint32 width, guint32 height, guint32 pixelformat)
+{

This need to be re-factored with a look, there is too much copy paste.

@@ +2269,3 @@
       gst_structure_set (tmp, "height", GST_TYPE_INT_RANGE, min_h, max_h,
NULL);

+    /* We could consider setting interlace mode from min and max. */

This is not needed, for range or step, the available settings are expected to
apply to the entire range/step. So testing one should be enough, testing both
would only be a small sanity check of the driver.

@@ +2314,1 @@
     /* try again with interlaced video */

I think there is some cleanup to do here. As we should to this first TRY_FMT
with ANY, interlace vs progressive should never be a reason to fail, hence
there should not be any fallback.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.