After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 791471 - v4l2object: non-default colorimetry broken for v4l2h264enc
v4l2object: non-default colorimetry broken for v4l2h264enc
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.14.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-12-11 11:32 UTC by Philipp Zabel
Modified: 2018-11-03 15:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
v4l2object: Validate colorimetry in S_FMT only if skip_try_fmt_probes is set (2.50 KB, patch)
2017-12-11 11:32 UTC, Philipp Zabel
none Details | Review
V4L2 log for videotestsrc ! v4l2h264enc ! fakesink pipeline (40.50 KB, text/x-log)
2017-12-13 13:55 UTC, Philipp Zabel
  Details
v4l2videoenc: Don't set colorimetry on capture (1.79 KB, patch)
2018-06-27 21:06 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Philipp Zabel 2017-12-11 11:32:38 UTC
Created attachment 365355 [details] [review]
v4l2object: Validate colorimetry in S_FMT only if skip_try_fmt_probes is set

Since commit 558e9f4e574d ("v4l2object: Validate colorimetry in S/TRY_FMT"), trying to set a supported but non-default colorimetry on the sink pad of a v4l2h264enc element fails with an error message:

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

This currently causes a pipeline like:

  videotestsrc ! v4l2h264enc ! fakesink

to fail when using the coda v4l2 driver, because bt601 is negotiated whereas the output queue defaults to bt709.
(probed caps: colorimetry=(string){ bt709, bt601, smpte240m, ... })

The same happens when forcing any non-default colorimetry:

  videotestsrc ! video/x-raw,colorimetry=smpte240m ! v4l2h264enc ! fakesink
Comment 1 Nicolas Dufresne (ndufresne) 2017-12-11 21:18:14 UTC
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.
Comment 2 Philipp Zabel 2017-12-13 13:55:17 UTC
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
Comment 3 Nicolas Dufresne (ndufresne) 2017-12-13 16:14:56 UTC
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.
Comment 4 Philipp Zabel 2017-12-13 17:03:29 UTC
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.
Comment 5 Nicolas Dufresne (ndufresne) 2017-12-13 18:49:17 UTC
Hmm, I need to improve the tracing, indeed we never know which GstV4l2Object is tracking, because it trace using the parent element.
Comment 6 Nicolas Dufresne (ndufresne) 2017-12-13 18:53:52 UTC
(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.
Comment 7 Nicolas Dufresne (ndufresne) 2017-12-13 19:43:10 UTC
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>.
Comment 8 Nicolas Dufresne (ndufresne) 2017-12-19 01:53:48 UTC
Any update, was it only the driver in the end ?
Comment 9 Philipp Zabel 2017-12-19 10:48:57 UTC
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.
Comment 10 Nicolas Dufresne (ndufresne) 2017-12-19 14:41:51 UTC
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 ?
Comment 11 Nicolas Dufresne (ndufresne) 2018-04-09 00:29:43 UTC
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.
Comment 12 Nicolas Dufresne (ndufresne) 2018-05-19 01:03:13 UTC
Any feedback ?
Comment 13 Philipp Zabel 2018-05-22 14:44:28 UTC
What you wrote in December makes sense to me. But I'm not sure I understand the April comment. What is verified colorimetry?
Comment 14 Nicolas Dufresne (ndufresne) 2018-05-23 02:06:11 UTC
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.
Comment 15 Philipp Zabel 2018-06-11 08:26:20 UTC
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.
Comment 16 Nicolas Dufresne (ndufresne) 2018-06-21 01:56:40 UTC
And that applies to interlace modes also.
Comment 17 Nicolas Dufresne (ndufresne) 2018-06-21 02:15:57 UTC
Do you have a file that triggers this, I'm setup now, so I could debug this myself.
Comment 18 Philipp Zabel 2018-06-21 09:11:01 UTC
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
Comment 19 Nicolas Dufresne (ndufresne) 2018-06-21 11:15:18 UTC
Ah, ok, thanks, I'll work on that.
Comment 20 Nicolas Dufresne (ndufresne) 2018-06-26 21:15:51 UTC
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.
Comment 21 Nicolas Dufresne (ndufresne) 2018-06-27 21:06:34 UTC
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 22 Nicolas Dufresne (ndufresne) 2018-06-27 21:12:03 UTC
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
Comment 23 Philipp Zabel 2018-06-28 10:18:55 UTC
Thank you, I can confirm the issues in the initial report are fixed for CODA.
Comment 24 GStreamer system administrator 2018-11-03 15:24:33 UTC
-- 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.