Created attachment 294894 [details] [review]
v4l2object: set colorspace in caps for capture devices

Proposal patch to retrieve the colorspace information and to set it in caps.

Review of attachment 294894 [details] [review]:

Few comments, the last one is mostly a reflection, but I'm not 100% sure it matter.

::: sys/v4l2/gstv4l2object.c
@@ +1675,3 @@
+      break;
+    default:
+      GST_ERROR ("Unknown enum v4l2_colorspace %d", colorspace);

I think some driver don't support it, hence will set 0. I think we should handle that case without spamming the logs. I already have two spammer to fix  now ;-P Also, see the guessing code we do in the opposite direction.

@@ +1791,3 @@
+    fmt.fmt.pix.height = height;
+    fmt.fmt.pix.pixelformat = pixelformat;
+  }

The if case is not needed, this part of the structure is that same for mplane.

@@ +2308,3 @@
         pixelformat);
     gst_v4l2_object_add_aspect_ratio (v4l2object, tmp);
+    gst_v4l2_object_add_colorspace (v4l2object, tmp, max_w, max_h, pixelformat);

I think this is a case where just checking the max may really lead to wrong result. We probably want to check min and max, if if different, put both in the caps.

(In reply to comment #2)
> Review of attachment 294894 [details] [review]:
> 
> Few comments, the last one is mostly a reflection, but I'm not 100% sure it
> matter.
> 
> ::: sys/v4l2/gstv4l2object.c
> @@ +1675,3 @@
> +      break;
> +    default:
> +      GST_ERROR ("Unknown enum v4l2_colorspace %d", colorspace);
> 
> I think some driver don't support it, hence will set 0. I think we should
> handle that case without spamming the logs. I already have two spammer to fix 
> now ;-P Also, see the guessing code we do in the opposite direction.

If the driver doesn't set the colorspace, I think we can just leave the caps with no colorimetry since the guess could be really wrong and we could live without colorimetry. 
In the opposite direction, we guess the colorspace as a last resort because the application must set it according to the spec.

However, I agree with the fact that I'm maybe too aggressive with an error :) for a thing we can live without.
So what do you think of a warning or debug message instead (warning will also spam)

> 
> @@ +1791,3 @@
> +    fmt.fmt.pix.height = height;
> +    fmt.fmt.pix.pixelformat = pixelformat;
> +  }
> 
> The if case is not needed, this part of the structure is that same for mplane.
Agreed, I will merge it.

> 
> @@ +2308,3 @@
>          pixelformat);
>      gst_v4l2_object_add_aspect_ratio (v4l2object, tmp);
> +    gst_v4l2_object_add_colorspace (v4l2object, tmp, max_w, max_h,
> pixelformat);
> 
> I think this is a case where just checking the max may really lead to wrong
> result. We probably want to check min and max, if if different, put both in the
> caps.

Not a bad reflection, I don't know if the size matter or not, it could depend of the driver I think. But in this case we would know for sure the colorspace when the format is negotiatied, ie after final S_FMT.
I think if some drivers do this, checking min and max could not be enough. For instance, I think of a device which can output SD with associated colorspace smtpe170m, HD in rec709 and UHD in bt2020.

(In reply to comment #3)
> (In reply to comment #2)
> However, I agree with the fact that I'm maybe too aggressive with an error :)
> for a thing we can live without.
> So what do you think of a warning or debug message instead (warning will also
> spam)

I would opt of debug message and no colorimetry field in the caps seems fine to me.


> > 
> > @@ +2308,3 @@
> >          pixelformat);
> >      gst_v4l2_object_add_aspect_ratio (v4l2object, tmp);
> > +    gst_v4l2_object_add_colorspace (v4l2object, tmp, max_w, max_h,
> > pixelformat);
> > 
> > I think this is a case where just checking the max may really lead to wrong
> > result. We probably want to check min and max, if if different, put both in the
> > caps.
> 
> Not a bad reflection, I don't know if the size matter or not, it could depend
> of the driver I think. But in this case we would know for sure the colorspace
> when the format is negotiatied, ie after final S_FMT.
> I think if some drivers do this, checking min and max could not be enough. For
> instance, I think of a device which can output SD with associated colorspace
> smtpe170m, HD in rec709 and UHD in bt2020.

Fair, maybe we can leave it this way for now and add a comment. There is an evident solution in the driver, which is to implement enumeration.

Created attachment 294959 [details] [review]
v4l2object: set colorspace in caps for capture devices

- demote GST_ERROR to GST_DEBUG to avoid errors spamming.
- add a comment about getting the fact that getting colorspace for maximum frame size could be wrong in case device colorspace depends on size (if possible).

Review of attachment 294959 [details] [review]:

commit 7a1613b9e15ab3e437c56b4de7bc15a30ea4f619
Author: Aurélien Zanelli <aurelien.zanelli@parrot.com>
Date:   Mon Jan 19 15:29:24 2015 +0100

    v4l2object: set colorspace in caps for capture devices
    
    This information is set by the driver for a capture device, and so could
    be forwarded to pipeline by setting the colorimetry in caps.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=743186