Created attachment 271638 [details] [review]
[PATCH] v4l2: Ensure interlace-mode is mixed whenever hardware does interlaced


There exists component video capture hardware capable of supporting both
720x576i25 and 720x576p25 formats. Test interlaced mode before progressive,
so that our caps are interlace-mode=mixed whenever buffers can be
interlaced.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
 sys/v4l2/gstv4l2object.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

This is indeed broken, and negotiation may fail. The propose patch fixes one use case, but it does not solve everything as you mention.

First thing to notice, we don't (can't ?) negotiation TFF, BFF, ONEFEILD or RFF in GStreamer. For a capture device it does not matter, but for other device it mean we should reject anything that does not support everything.

I think we need to push the reflection further, and maybe extend the interlace-method field in GStreamer.

One thing that you could do in your patch, would be to correctly fill an array of modes, so we can have caps with progressive, mixed and interlaced at the same time if supported.

Ping ?

I've not got time available to do much more to this patch right now - and I have no way to work with anything other than capture devices anyway. I'm happy to leave it open as a known bug, with the patch I supplied acting as a detailed bug report.

*** Bug 731702 has been marked as a duplicate of this bug. ***

Created attachment 289687 [details] [review]
[PATCH] v4l2: Support three interlace modes, not just progressive or mixed


With my capture setup, this gets me the following sample caps:

For 1080i resolution:
video/x-raw, format=(string)YUY2, width=(int)1920, height=(int)1080, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string){ mixed, interleaved }, framerate=(fraction){ 25/1, 30/1 }

For 720p resolution:
video/x-raw, format=(string)YUY2, width=(int)1280, height=(int)720, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string){ mixed, progressive }, framerate=(fraction){ 50/1, 60/1 }

For 576i/p resolution:
video/x-raw, format=(string)YUY2, width=(int)720, height=(int)576, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string){ mixed, progressive, interleaved }, framerate=(fraction){ 25/1, 50/1 }

This, in turn, makes 576i work correctly; with the old code,
the caps would be interlace-mode=progressive for interlaced video.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
 sys/v4l2/gstv4l2bufferpool.c |  28 ++++--
 sys/v4l2/gstv4l2object.c     | 211 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 186 insertions(+), 53 deletions(-)

Note that this patch is a little iffy - my GObject-fu is minimal, at best.

However, it greatly improves interlace handling for my setup, by translating the V4L2_FIELD values I can use to interlace-mode values.

Thanks, I'll have a look, the caps you just show in comment 6 are what I wanted to see, it's a good sign ;-P

Review of attachment 289687 [details] [review]:

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.

(In reply to comment #9)
> Review of attachment 289687 [details] [review]:
> 
> 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.

I've asked Hans if drivers returning ANY from TRY_FMT are doing it wrong, answer was quite direct:

<hverkuil> 04:23:45> stormer: that's correct, drivers should never return FIELD_ANY.

The reason is that ANY only exist for requesting something, basically when the application don't care. It's not an interlaced mode. Reading attentively the quote you have brought, the specification is actually correct. Any is for a "request" when "V4L2_FIELD_NONE,
V4L2_FIELD_TOP, V4L2_FIELD_BOTTOM, or V4L2_FIELD_INTERLACED is acceptable". I'm sure next release of Hans driver validation will check this ;-P.

Note, it's just one case, everything else seems right in your patch. What trigger my concern is that "mixed" was present all over your test results. Where mixed is something that should be very rare.

If you don't have time, don't worry about the rest, I can take over from here. About checking both min and max, I'll think about it, I still think it's not really needed. Keep in mind that this code was written a long time ago, where literally none of the drivers where implementing the spec the same way. The situation is much better now and I'm pushing forward cleaning up some "workaround" code when possible.

I also forgot, but order matter in negotiation, we should have progressive first.

Created attachment 289757 [details] [review]
[PATCH] v4l2: Clean up interlace support


Rather than try and guess interlace support as part of checking supported
sizes, look for interlace support specifically in its own function.

As a cleanup, use V4L2_FIELD_ANY when probing sizes, which should result in
the driver doing the right thing.

With my capture setup, this gets me the following sample caps:

For 1080i resolution:
video/x-raw, format=(string)YUY2, width=(int)1920, height=(int)1080, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)interleaved, framerate=(fraction){ 25/1, 30/1 }

For 720p resolution:
video/x-raw, format=(string)YUY2, width=(int)1280, height=(int)720, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, framerate=(fraction){ 50/1, 60/1 }

For 576i/p resolution (both possible at the point of query):
video/x-raw, format=(string)YUY2, width=(int)720, height=(int)576, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string){ progressive, interleaved }, framerate=(fraction){ 25/1, 50/1 }

This, in turn, makes 576i work correctly; with the old code,
the caps would be interlace-mode=progressive for interlaced video.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
 sys/v4l2/gstv4l2bufferpool.c |  28 ++++--
 sys/v4l2/gstv4l2object.c     | 226 ++++++++++++++++++++++++-------------------
 sys/v4l2/gstv4l2src.c        |   3 +
 3 files changed, 151 insertions(+), 106 deletions(-)

Review of attachment 289757 [details] [review]:

Looks good to me, thanks for the quick update.

Comment on attachment 289757 [details] [review]
[PATCH] v4l2: Clean up interlace support

commit 02040d507cc4a3d4d22fab15b8bdf1d3746ea26f
Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
Date:   Thu Oct 30 17:41:19 2014 +0000

    v4l2: Clean up interlace support

Need some opinion on getting this in upcoming 1.4.4. It is conceptually a bug fix, even though this has been broken for years in fact. I'm mostly incline to include it in stable, but would like some more opinions / point of view.

(In reply to comment #16)
> Need some opinion on getting this in upcoming 1.4.4. It is conceptually a bug
> fix, even though this has been broken for years in fact. I'm mostly incline to
> include it in stable, but would like some more opinions / point of view.

FWIW, I'm running with this patch applied against 1.4.3; it's a considerable improvement from my point of view.

However, there is a slight chance of regression. Previously, the caps from v4l2src would always contain either interlace-mode=mixed or interlace-mode=progressive. Now, they will contain one of interlace-mode=progressive, interlace-mode=interleaved, or interlace-mode={ progressive, interleaved }. Pipelines that depended on being able to ask for interlace-mode=mixed and get interlaced video from v4l2src will now fail.

For master, I don't see this as an issue - we're simply reporting our caps accurately. For 1.4, however, there is the risk that a user depended on the old buggy caps and is now caught out by what should have been a safe upgrade.

As background, interlace-mode=mixed would normally be appropriate for things like MPEG-2 and H.264 3:2 pulldown signalling; when you're encoding film material at 60 fields per second, the encoded stream contains 24 frames per second of video, and uses the RFF flag to indicate the 3:2 pulldown cadence. You can then switch back to providing a true 60 fields per second stream when you're in video, and return to 24 frames per second with 3:2 cadence signalling for film sourced material. interlace-mode=mixed has semantics that precisely match this and enable it to correctly signal the move from 24 frames per second with 3:2 pulldown back to 60 fields per second then back to 24 frames per second with 3:2 pulldown.

Ok, I've chosen to take that risk. We can address H264/MPEG2 byte-stream issue if they become visible.

commit 66099