Can you share a trace of the failing case ? Can you better explain what the coda driver accepts ? I don't have enough context to complete this review.

Created attachment 365490 [details]
V4L2 log for videotestsrc ! v4l2h264enc ! fakesink pipeline

The attached log contains the output of:

gst-launch-1.0 --gst-debug=*v4l2*:6 --gst-debug-no-color videotestsrc ! v4l2h264enc ! fakesink

Probed sink caps return:

colorimetry=(string){ bt601, smpte240m, bt709, 2:4:5:2, 2:4:5:3, 1:4:7:1, 2:4:7:1, 2:4:12:8, bt2020, 2:0:0:0 }

bt601 is negotiated with the videotestsrc, which does not necessarily match the currently set colorimetry returned by G_FMT, and gst_v4l2_object_set_format_full fails with:

ERROR: from element /GstPipeline:pipeline0/v4l2h264enc:v4l2h264enc0: Device '/dev/video0' does not support bt601 colorimetry

But then failing is the right thing to do, you have a colorimetry miss-match. The colorimetry produces by the source MUST match the one accepted by the encoder. Ignoring it like you do is wrong.

There is a bug of course, since adding videoconvert in the middle should fix it, but I guess it does not work since there is no way to know. The driver getcaps() says all these colorimetry are supported, but then TRY/S_FMT says otherwise when we do actually set the format. So we need to figure-out how to not lie to when replying to getcaps. It's pretty difficult since V4L2 API seriously lack some APIs here.

Oh, I completely missed it's the src pad (capture queue) that fails here, since we have to set the coded format first.
It fails because the coda driver incorrectly limits the capture colorspaces to the one set on the output queue in TRY_FMT.

Hmm, I need to improve the tracing, indeed we never know which GstV4l2Object is tracking, because it trace using the parent element.

(In reply to Philipp Zabel from comment #4)
> Oh, I completely missed it's the src pad (capture queue) that fails here,
> since we have to set the coded format first.
> It fails because the coda driver incorrectly limits the capture colorspaces
> to the one set on the output queue in TRY_FMT.

Ah, now I understand how it failed. I think the fact that V4L2 does not have a "unset" mode for the format does not help here.

Let's hope this will help in the future:

commit e575e4eda82d0a3a0ab3a58f62d24d7a47fcc32b (HEAD -> master, origin/master, origin/HEAD)
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Wed Dec 13 14:39:47 2017 -0500

    v4l2object: Use a debug object for tracing
    
    This way we can pass the pad name instead of the element for tracing
    which helps identifying which v4l2object is used withing M2M element
    like decoder, encoder and transform. For the reference, pads are name
    <parent-name>:<pad-name>.

Any update, was it only the driver in the end ?

I have removed all colorimetry checks from the driver to allow GStreamer to set any colorimetry on either queue in any order.
I have also removed the size dependent default colorimetry setting (bt601 for < 720p, bt709 otherwise) and always let the driver default to bt601.
It still fails if I force any non-default colorimetry:

gst-launch-1.0 --gst-debug=*v4l2*:6 videotestsrc ! video/x-raw,colorimetry=bt709 ! v4l2h264enc ! fakesink

0:00:00.487893333   371  0x147a350 DEBUG                   v4l2 gstv4l2object.c:4076:gst_v4l2_object_set_format:<v4l2h264enc0:src> Setting format to video/x-h264, stream-format=(string)byte-stream, alignment=(string)au, profile=(string)baseline, level=(string)4, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, framerate=(fraction)30/1, interlace-mode=(string)progressive, colorimetry=(string)bt709, multiview-mode=(string)mono, multiview-flags=(GstVideoMultiviewFlagsSet)0:ffffffff:/right-view-first/left-flipped/left-flopped/right-flipped/right-flopped/half-aspect/mixed-mono
0:00:00.488061333   371  0x147a350 DEBUG                   v4l2 gstv4l2object.c:3512:gst_v4l2_object_set_format_full:<v4l2h264enc0:src> progressive video
0:00:00.488115000   371  0x147a350 DEBUG                   v4l2 gstv4l2object.c:3650:gst_v4l2_object_set_format_full:<v4l2h264enc0:src> Desired format 320x240, format H264 stride: 0
0:00:00.488164667   371  0x147a350 DEBUG                   v4l2 gstv4l2object.c:3701:gst_v4l2_object_set_format_full:<v4l2h264enc0:src> Desired format is 320x240, format H264, nb planes 1
0:00:00.488210000   371  0x147a350 DEBUG                   v4l2 gstv4l2object.c:3710:gst_v4l2_object_set_format_full:<v4l2h264enc0:src>   stride 0
0:00:00.488257333   371  0x147a350 DEBUG                   v4l2 gstv4l2object.c:3744:gst_v4l2_object_set_format_full:<v4l2h264enc0:src> Got format of 320x240, format H264, nb planes 1, colorspace 1
0:00:00.488306000   371  0x147a350 DEBUG                   v4l2 gstv4l2object.c:3754:gst_v4l2_object_set_format_full:<v4l2h264enc0:src>   stride 0, sizeimage 155648
ERROR: from element /GstPipeline:pipeline0/v4l2h264enc:v4l2h264enc0: Device '/dev/video14' does not support bt709 colorimetry
Additional debug info:
gstv4l2object.c(4042): gst_v4l2_object_set_format_full (): /GstPipeline:pipeline0/v4l2h264enc:v4l2h264enc0:
Device wants bt601 colorimetry

The issue seems to be that we set CAPTURE format first in v4l2videoenc, but v4l2object doesn't even try to set our desired colorspace:

  if (V4L2_TYPE_IS_OUTPUT (v4l2object->type)) {
    if (is_mplane) {
      format.fmt.pix_mp.colorspace = colorspace;
      format.fmt.pix_mp.quantization = range;
      format.fmt.pix_mp.ycbcr_enc = matrix;
      format.fmt.pix_mp.xfer_func = transfer;
    } else {
      format.fmt.pix.colorspace = colorspace;
      format.fmt.pix.quantization = range;
      format.fmt.pix.ycbcr_enc = matrix;
      format.fmt.pix.xfer_func = transfer;
    }

    GST_DEBUG_OBJECT (v4l2object->dbg_obj, "Desired colorspace is %d:%d:%d:%d",
        colorspace, range, matrix, transfer);
  }

Note the missing "Desired colorspace" from the logs.
If I enable that (and the initialization of these local variables above) for CAPTURE instead, I get it rolling:

0:00:00.586204333   385  0x1e5d150 DEBUG                   v4l2 gstv4l2object.c:4076:gst_v4l2_object_set_format:<v4l2h264enc0:sink> Setting format to video/x-raw, format=(string)I420, width=(int)320, height=(int)240, framerate=(fraction)30/1, multiview-mode=(string)mono, colorimetry=(string)bt709, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
0:00:00.586345000   385  0x1e5d150 DEBUG                   v4l2 gstv4l2object.c:3512:gst_v4l2_object_set_format_full:<v4l2h264enc0:sink> progressive video
0:00:00.586394000   385  0x1e5d150 DEBUG                   v4l2 gstv4l2object.c:3650:gst_v4l2_object_set_format_full:<v4l2h264enc0:sink> Desired format 320x240, format YU12 stride: 320
0:00:00.586453000   385  0x1e5d150 DEBUG                   v4l2 gstv4l2object.c:3701:gst_v4l2_object_set_format_full:<v4l2h264enc0:sink> Desired format is 320x240, format YU12, nb planes 1
0:00:00.586508666   385  0x1e5d150 DEBUG                   v4l2 gstv4l2object.c:3710:gst_v4l2_object_set_format_full:<v4l2h264enc0:sink>   stride 320
0:00:00.586550666   385  0x1e5d150 DEBUG                   v4l2 gstv4l2object.c:3728:gst_v4l2_object_set_format_full:<v4l2h264enc0:sink> Desired colorspace is 3:2:2:1
0:00:00.586619333   385  0x1e5d150 DEBUG                   v4l2 gstv4l2object.c:3744:gst_v4l2_object_set_format_full:<v4l2h264enc0:sink> Got format of 320x240, format YU12, nb planes 1, colorspace 3
0:00:00.586686666   385  0x1e5d150 DEBUG                   v4l2 gstv4l2object.c:3754:gst_v4l2_object_set_format_full:<v4l2h264enc0:sink>   stride 320, sizeimage 115200

It seems to me that for v4l2videoenc, we should probe both the src and sink colorimetry caps from the CAPTURE queue in case the driver limits OUTPUT colorimetry to the currently set CAPTURE value?
And set_format_full for the encoder needs to set colorimetry on the CAPTURE queue instead of the OUTPUT queue.

Yes, sounds like it's the opposite of decoding. So basically if we ignore output colorimetry and read capture colorimetry in v4lobject and then copy colorimetry field into the sinkpad getcaps.

 - add a way to enable/disable colorimetry in v4l2object
 - update other element, to make it explicit
  + Src: already disabled
  + Decoder: output only
  + Encoder: capture only
  + Transform: both
  + Sink: enabled
- copy colorimetry from capture to output getcaps in encoder

We should still read back colorimetry in the decoder since we allow color conversion. Does that make sense ?

I went through this thread again, and indeed, the partial enumeration of colorimetry is problematic. Maybe I could change the fixed list into a "preferred" list. So at least those verified colorimetry will be tried first, otherwise what remains will be tried. Something like:

  video/x-raw,format=X,...,colorimetry=verififed;video/x-raw,format=X,...,

Do you think that could work ? It will limit the risk of failure, but allow full colorimetried to be used.

Any feedback ?

What you wrote in December makes sense to me. But I'm not sure I understand the April comment. What is verified colorimetry?

A verified colorimetry, is a set of colorimetry parameter that as been TRY_FMT. As only a small subset are tested for performance reason, the default ones, then it means a very small subset can be used. This is problematic, it a limitation of the Linux kernel. Now I'm just trying to workaround this, proposing to add a set of default, but then also accept all the other. Maybe it's not worth it, and we should just assume they are all supported for encoder and encoder, and leave it like that until someone fix the Linux kernel interface.

With the V4L2 codec API discussion flaring up again, I'm unsure as ever how to handle this, but I understand that using the verified colorimetry alone will not suffice. At the very least colorimetry that is fixed on the sink pad should be available on the source pad initially.

I wasn't aware before that currently the V4L2 API does not allow to set colorimetry on the capture side at all. Given that everybody seems to expect encoder colorimetry to be set on the unencoded output queue, and the encoder initialization scheme makes us call S_FMT on the capture queue first, I see no way to read useful capture queue colorimetry before the first source change event at all.

And that applies to interlace modes also.

Do you have a file that triggers this, I'm setup now, so I could debug this myself.

There is no file, this can be triggered with the pipelines in the initial report:

  gst-launch-1.0 videotestsrc ! v4l2h264enc ! fakesink

and

  gst-launch-1.0 videotestsrc ! video/x-raw,colorimetry=smpte240m ! v4l2h264enc ! fakesink

Ah, ok, thanks, I'll work on that.

Half of it can be fixed by not passing colorimetry to the encoder caps when calling set_format. But then the encoder must not do any colorspace transformation internally. This is probably fine for now, until we find an encoder that does such transformation. I'll try and post this patch tomorrow.

GStreamer currently only try and set colorimetry on the OUTPUT, which is what the spec says, but the probe function does not respect that. Basically, the spec as-is prevent having multiple colorspace for a specific format/size on CAPTURE devices.

Created attachment 372856 [details] [review]
v4l2videoenc: Don't set colorimetry on capture

The colorimetry will be set along with the raw format and those fields
will then be copied from sink to src caps by the gst encoder.

Comment on attachment 372856 [details] [review]
v4l2videoenc: Don't set colorimetry on capture

It's not fixing the problem, just making the CODA encoder work. I think to
really fix this we'll have to drop the colorimetry probing completly. But this
will have side effects and require some thinking/coding.

Attachment 372856 [details] pushed as 5fc67b2 - v4l2videoenc: Don't set colorimetry on capture

Thank you, I can confirm the issues in the initial report are fixed for CODA.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/423.