Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: linux-media@vger.kernel.org, helen.koike@collabora.com,
	ezequiel@collabora.com, hverkuil@xs4all.nl, kernel@collabora.com,
	dafna3@gmail.com, laurent.pinchart@ideasonboard.com,
	linux-rockchip@lists.infradead.org, sakari.ailus@linux.intel.com,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Hans Verkuil <HansVerkuil@cisco.com>
Subject: Re: [RFC v3 1/2] v4l2: add support for colorspace conversion for video capture
Date: Wed, 10 Jun 2020 13:34:11 +0000
Message-ID: <20200610133411.GA192932@chromium.org> (raw)
In-Reply-To: <dba58521-a619-91fe-2b60-ea5ce85a9fa2@collabora.com>

On Fri, Jun 05, 2020 at 12:11:33PM +0200, Dafna Hirschfeld wrote:
> Hi,
> 
> On 04.06.20 19:39, Tomasz Figa wrote:
> > Hi Dafna,
> > 
> > On Thu, Apr 16, 2020 at 04:56:04PM +0200, Dafna Hirschfeld wrote:
> > > From: Philipp Zabel <p.zabel@pengutronix.de>
> > > 
> > > For video capture it is the driver that reports the colorspace,
> > > Y'CbCr/HSV encoding, quantization range and transfer function
> > > used by the video, and there is no way to request something
> > > different, even though many HDTV receivers have some sort of
> > > colorspace conversion capabilities.
> > > 
> > 
> > Thanks for working on this! Please see my comments inline.
> > 
> > > For output video this feature already exists since the application
> > > specifies this information for the video format it will send out, and
> > > the transmitter will enable any available CSC if a format conversion has
> > > to be performed in order to match the capabilities of the sink.
> > > 
> > > For video capture we propose adding new pix_format flag:
> > > V4L2_PIX_FMT_FLAG_HAS_CSC. The flag is set by the application,
> > > the driver will interpret the ycbcr_enc/hsv_enc, and quantization fields
> > > as the requested colorspace information and will attempt to
> > > do the conversion it supports.
> > > 
> > > Drivers set the flags
> > > V4L2_FMT_FLAG_CSC_YCBCR_ENC,
> > > V4L2_FMT_FLAG_CSC_HSV_ENC,
> > > V4L2_FMT_FLAG_CSC_HSV_QUANTIZATION,
> > 
> > Do we need this level of granularity? The drivers would ignore
> > unsupported encoding/quantization settings and reset them to supported
> > ones anyway, so if one doesn't support changing given parameter, it
> > would just return back the original stream parameter.
> 
> I think this granularity allows userspace to know ahead what is supported
> and what is not so it doesn't have to guess.
>

The userspace needs to guess anyway, because there is no way to
enumerate the supported target parameters. For example, even if the
CSC_YCBCR_ENC bit is set, only only DEFAULT, 601 and BT2020 could be
supported. If the userspace wants to get the 709 encoding, it needs to
explicitly try setting it and see if the TRY_FMT/S_FMT operation accepts
the setting.

[snip]
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index a376b351135f..51e009936aad 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -477,6 +477,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > >   	case VIDIOC_SUBDEV_S_FMT: {
> > >   		struct v4l2_subdev_format *format = arg;
> > > +		if (!(format->format.flags & V4L2_MBUS_FRAMEFMT_HAS_CSC)) {
> > > +			format->format.colorspace = V4L2_COLORSPACE_DEFAULT;
> > > +			format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > > +			format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > > +			format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
> > > +		}
> > 
> > Wouldn't this break setting the colorspaces on the sink pads, for which
> > the flag isn't required? Actually there is some unfortunate statement in
> > the documentation related to this:
> > 
> > "This information supplements the colorspace and must be set by the
> > driver for capture streams and by the application for output streams,
> > see Colorspaces."
> > 
> > However, I don't think there is any notion of "capture" and "output" for
> > subdevices, right? Instead, the pad direction would have to be checked,
> > but AFAICT there is no access to this kind of information from this
> > wrapper.
> 
> Actually in coming v4 there is no new code added accept of the new fields and
> macros of the API. I think there is no need to change any code.
> 
> 

Agreed.

> > 
> > > +
> > >   		memset(format->reserved, 0, sizeof(format->reserved));
> > >   		memset(format->format.reserved, 0, sizeof(format->format.reserved));
> > >   		return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
> > > diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
> > > index 123a231001a8..89ff0ad6a631 100644
> > > --- a/include/uapi/linux/v4l2-mediabus.h
> > > +++ b/include/uapi/linux/v4l2-mediabus.h
> > > @@ -16,6 +16,8 @@
> > >   #include <linux/types.h>
> > >   #include <linux/videodev2.h>
> > > +#define V4L2_MBUS_FRAMEFMT_HAS_CSC	0x0001
> > > +
> > >   /**
> > >    * struct v4l2_mbus_framefmt - frame format on the media bus
> > >    * @width:	image width
> > > @@ -36,7 +38,8 @@ struct v4l2_mbus_framefmt {
> > >   	__u16			ycbcr_enc;
> > >   	__u16			quantization;
> > >   	__u16			xfer_func;
> > > -	__u16			reserved[11];
> > > +	__u16			flags;
> > 
> > Are we okay with a u16 for flags?
> 
> I am fine with it, though don't mind changing it if there are objections.
> 

I'd suggest making it a u32 to be a bit more future-proof.

Best regards,
Tomasz

  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-16 14:56 Dafna Hirschfeld
2020-04-16 14:56 ` [RFC v3 2/2] media: staging: rkisp1: allow quantization conversion from userspace for isp source pad Dafna Hirschfeld
2020-06-04 17:54   ` Tomasz Figa
2020-06-04 19:11     ` Dafna Hirschfeld
2020-06-10 12:08       ` Tomasz Figa
2020-06-18 17:27         ` Tomasz Figa
2020-06-18 18:26           ` Dafna Hirschfeld
2020-06-18 18:49             ` Tomasz Figa
2020-06-18 18:54               ` Dafna Hirschfeld
2020-06-18 19:25                 ` Tomasz Figa
2020-06-26  0:15                   ` Helen Koike
2020-06-28 13:10                     ` Tomasz Figa
2020-06-28 13:15                       ` Laurent Pinchart
2020-06-28 14:59                         ` Tomasz Figa
2020-06-28 19:34                           ` Dafna Hirschfeld
2020-06-29 15:50                             ` Helen Koike
2020-06-29 23:08                               ` Laurent Pinchart
2020-06-30  9:36                                 ` Dafna Hirschfeld
2020-07-01 12:52                                   ` Laurent Pinchart
2020-07-01 14:41                                     ` Dafna Hirschfeld
2020-07-02 18:43                                       ` Laurent Pinchart
2020-06-28 20:27                           ` Laurent Pinchart
2020-06-28 21:33                             ` Tomasz Figa
2020-06-28 21:37                               ` Laurent Pinchart
2020-06-29 12:08                                 ` Dafna Hirschfeld
2020-06-29 12:21                                   ` Laurent Pinchart
2020-06-29 12:29                                     ` Dafna Hirschfeld
2020-06-29 12:33                                       ` Laurent Pinchart
2020-06-26  0:01             ` Helen Koike
2020-05-07 13:00 ` [RFC v3 1/2] v4l2: add support for colorspace conversion for video capture Hans Verkuil
2020-05-25 13:56   ` Dafna Hirschfeld
2020-05-25 14:25     ` Hans Verkuil
2020-05-26 12:36       ` Dafna Hirschfeld
2020-05-26 13:47         ` Hans Verkuil
2020-06-04 17:39 ` Tomasz Figa
2020-06-05 10:11   ` Dafna Hirschfeld
2020-06-10 13:34     ` Tomasz Figa [this message]
2020-06-30 16:11       ` Dafna Hirschfeld
2020-06-30 16:31         ` Tomasz Figa
2020-06-30 17:43           ` Laurent Pinchart
2020-06-30 18:02             ` Tomasz Figa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200610133411.GA192932@chromium.org \
    --to=tfiga@chromium.org \
    --cc=HansVerkuil@cisco.com \
    --cc=dafna.hirschfeld@collabora.com \
    --cc=dafna3@gmail.com \
    --cc=ezequiel@collabora.com \
    --cc=helen.koike@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kernel@collabora.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=p.zabel@pengutronix.de \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git