[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

Simon Farnsworth <simon> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simon at farnz.org.uk

--- Comment #10 from Simon Farnsworth <simon at farnz.org.uk> 2014-10-31 09:35:18 UTC ---
(In reply to comment #9)
> Review of attachment 289687 [details]:
> 
> 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.
> 
I'm basing my interpretation of ANY on
http://linuxtv.org/downloads/v4l-dvb-apis/field-order.html Table 3.9 -
"Applications request this field order when any one of the V4L2_FIELD_NONE,
V4L2_FIELD_TOP, V4L2_FIELD_BOTTOM, or V4L2_FIELD_INTERLACED is acceptable".
This matches mixed mode interlace.

> ::: 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.
> 
I disagree, as above. We need Hans to weigh in here.

> @@ +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.
> 
Agreed. However, I'm not sure when I'll have time to do that properly; having
the patch out here at least lets you validate my ideas.

> @@ +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.
> 
The only reason to test both would be because that's what the old code did.

> @@ +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.

I can do that, once we've got Hans's interpretation of ANY supplied. Would you
mind asking him to comment on this bug? If my interpretation is wrong, the spec
needs a clarification.

-- 
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.