Re: [PATCH v7 1/9] media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more)

[Hsia-Jun Li]

On 6/29/23 00:27, Nicolas Dufresne wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Hi,
>
> please avoid HTML replies on the mailing list.
 I need to blame outlook.
> Le mardi 27 juin 2023 à 14:42 +0800, Hsia-Jun Li a écrit :
> > > +/**
> > > + * struct v4l2_ext_pix_format - extended multiplanar format definition
> > > + * @type:          enum v4l2_buf_type; type of the data stream
> > > + * @width:         image width in pixels
> > > + * @height:                image height in pixels
> > > + * @pixelformat:   little endian four character code (fourcc)
> > > + * @modifier:              modifier applied to the format (used for tiled formats
> > > + *                 and other kind of HW-specific formats, like compressed
> > > + *                 formats) as defined in drm_fourcc.h
> > > + * @field:         enum v4l2_field; field order (for interlaced video)
> > > + * @colorspace:            enum v4l2_colorspace; supplemental to pixelformat
> > > + * @plane_fmt:             per-plane information
> > > + * @flags:         format flags (V4L2_PIX_FMT_FLAG_*)
> > > + * @ycbcr_enc:             enum v4l2_ycbcr_encoding, Y'CbCr encoding
> > > + * @hsv_enc:               enum v4l2_hsv_encoding, HSV encoding
> > > + * @quantization:  enum v4l2_quantization, colorspace quantization
> > > + * @xfer_func:             enum v4l2_xfer_func, colorspace transfer function
> > > + * @reserved:              drivers and applications must zero this array
> > > + */
> > > +struct v4l2_ext_pix_format {
> > > +   __u32                           type;
> > > +   __u32                           width;
> > > +   __u32                           height;
> > > +   __u32                           pixelformat;
> > > +   __u64                           modifier;
> > > +   __u32                           field;
> > > +   __u32                           colorspace;
> > > +
> > > +   struct v4l2_plane_pix_format    plane_fmt[VIDEO_MAX_PLANES];
> > > +   __u8                            flags;
> > > +    union {
> > > +           __u8                            ycbcr_enc;
> > > +           __u8                            hsv_enc;
> > > +   };
> > > +   __u8                            quantization;
> > > +   __u8                            xfer_func;
> >      I heard that a suggestion that we could remove colorimetry fields
> >        here.
> >      Although those are useless for codec M2M drivers if no pixel
> >        formats translation invoked.
> >      Even HDMI(DRM) cares about colorspace.
> >      For example if a downsink(TV) shows RGB formats,  with an YUV
> >        input frame buffer, colorimetry would be important or the wrong
> >        EOTF would be used. If YUV is MPEG range(linear EOTF) while a
> >        non-linear EOFT (full range) is used, you would found the black is
> >        not black enough while the white looks a gray. Also color bias
> >        would happen.
> >      This problem may not happen to a ultra high resolution TV while
> >        only YUV type color formats are supported due to HDMI bandwidth
> >        limitation.
> >      The problem I want to raise is the time cost for enumeration.
> >        Each pixel format with a colorimetry setting would invoke a
> >        ioctl(). For the application likes Gstreamer would enum all the
> >        possible colorimetries.
> >      It would be better we could have something like DRM blob id that
> >        application could copy the data from a non-DMA buffer from the
> >        kernel.
> This is a good topic. Colorimetry could indeed be moved away from the format,
> considering they cannot be enumerated. It remains that this information needs to
> be passed around, and the format of a blob in media space is not has restricted
> as with display HW. I think keeping an "exploded version" of the colorimetry
> remains needed.
>
> Remember though that for stateful decoder, were the information could be stored
> in the bitstream, the decoder is responsible for returning that information.
> Currently its passed through the G_FMT call, it would need to be replaced with a
> control, similar to the HDR10 static metadata. If the colorimetry is no longer
> static in the future, and may change while streaming, one option would be RO
> request. This was foreseen for HDR10+ and Dolby Vision metadata notably, though
> other options exists.

That is why I want to promote importing a whole buffer(framebuffer id) 
from drm.

We can't pass such DMA metadate with the main data(graphics here) in a 
queue.

> There exist known decoders that can do YUV to RGB conversion using an inline
> post procesor (VC8000D and newer is an example), and for these to return correct
> colors, the colorimetry information needs to be passed. So its not strictly
> useless.
>
> In short, if we drop colorimetry from format, we also need to go ahead and

I am worrying when people setup up a UVC device(usb camera), they may 
ignore this step.

Camera could be in a BT.601 colorspace than BT.709 which has a slight 
difference. And BT.601 has a full range variant.
> design a replacement for it, that allow for the application to detect changes.
>
> regards,
> Nicolas
--
Hsia-Jun(Randy) Li