[libcamera-devel] [PATCH 1/2] libcamera: v4l2-format: Add V4L2DeviceFormat

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 28, 2019 at 05:20:02PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 28, 2019 at 03:57:45PM +0000, Kieran Bingham wrote:
> > On 28/01/2019 15:42, Kieran Bingham wrote:
> >> On 28/01/2019 15:11, Jacopo Mondi wrote:
> >>> Add a V4L2DeviceFormat class aimed to be used to provide format configuration
> >>> requests to a V4L2Device.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >>> ---
> >>>  src/libcamera/include/v4l2_device.h | 14 ++++++++
> >>>  src/libcamera/v4l2_device.cpp       | 56 +++++++++++++++++++++++++++++
> >>>  2 files changed, 70 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >>> index c67ebbf..c70959e 100644
> >>> --- a/src/libcamera/include/v4l2_device.h
> >>> +++ b/src/libcamera/include/v4l2_device.h
> >>> @@ -53,6 +53,20 @@ struct V4L2Capability final : v4l2_capability {
> >>>  	}
> >>>  };
> >>>
> >>> +class V4L2DeviceFormat
> >>> +{
> >>> +public:
> >>> +	uint32_t width;
> >>> +	uint32_t height;
> >>> +	uint32_t fourcc;
> >>
> >> I might have called this pixelFormat, but it's internal and it can
> >> change if we later define a PixelFormat class or such.
> >>
> >> Currently we will store a fourcc so this is accurate.
> >
> > struct v4l2_pix_format *pix stores the following extra information:
> 
> Not only v4l2_pix_format, but v4l2_pix_format_mplane and
> v4l2_mbus_framefmt too...
> 
> > 	field (we might need to consider interlaced somewhere?)
> > 	colorspace,
> > 	flags
> > 	ycbcr_enc, hsv_enc
> > 	quantization
> > 	xfer_func
> 
> Right now, I cannot tell. I'm sure there are use cases where those
> informations are relevant.
> 
> > Should we be handling any of that information here too?
> >
> > Or should we add it on an as-required basis?
> 
> I think they just need to be copied back and forth from the ioctl
> arguments, once the V4L2DeviceFormat has been expanded to include
> these fields.
> 
> I'll leave it for later, but if anyone has other opinions we can look
> into that now.

I'm fine with that. For the record, colorspace is a legacy field that
has been replaced with encoding, quantization and xfer function. We
should expose the new API through V4L2DeviceFormat when we'll add
support for colorspaces.

> >>> +
> >>> +	struct {
> >>> +		uint32_t size;
> >>> +		uint32_t bpl;
> >>> +	} planesFmt[3];
> >>> +	unsigned int planes;
> >>> +};
> >>> +
> >>>  class MediaEntity;
> >>>  class V4L2Device
> >>>  {
> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >>> index 408f9b9..d6143f2 100644
> >>> --- a/src/libcamera/v4l2_device.cpp
> >>> +++ b/src/libcamera/v4l2_device.cpp
> >>> @@ -80,6 +80,62 @@ LOG_DEFINE_CATEGORY(V4L2)
> >>>   * \return True if the device provides Streaming I/O IOCTLs
> >>>   */
> >>>
> >>> +/**
> >>> + * \class V4L2DeviceFormat
> >>> + * \brief The V4L2 device image format and sizes
> >>> + *
> >>> + * Describes the image format and image sizes to be programmed on a V4L2
> >>> + * video device. The image format is defined by fourcc code as defined by
> >>> + * the V4L2 APIs with the V4L2_PIX_FMT_ macros, a visible width and height
> >>> + * and a variable number of planes (1 to 3) with variable sizes and line
> >>> + * strides.
> >>> + *
> >>> + * Formats defined as 'single planar' by the V4L2 APIs are represented with
> >>> + * V4L2DeviceFormat instances with a single plane
> >>> + * (V4L2DeviceFormat::planes = 1). Semi-planar and multiplanar formats use
> >>> + * 2 and 3 planes respectively.
> >>> + *
> >>> + * V4L2DeviceFormat defines the exchange format between components that
> >>> + * receive image configuration requests from applications and a V4L2Device.
> >>> + * The V4L2Device validates and applies the requested size and format to
> >>> + * the device driver.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var V4L2DeviceFormat::width
> >>> + * \brief The image width
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var V4L2DeviceFormat::height
> >>> + * \brief The image height
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var V4L2DeviceFormat::fourcc> + * \brief The pixel encoding scheme
> >>> + *
> >>> + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros,
> >>> + * that identifies the image format pixel encoding scheme.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var V4L2DeviceFormat::planesFmt
> >>> + * \brief The per-plane size information
> >>> + *
> >>> + * Images are stored in memory in one or more data planes. Each data plane
> >>> + * has a specific size and line length, which could differ from the image
> >>> + * visible sizes to accommodate line or plane padding data.
> >>> + *
> >>> + * Only the first V4L2DeviceFormat::planes entries are considered valid.
> >>> + *
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var V4L2DeviceFormat::planes
> >>> + * \brief The number of valid data planes
> >>> + */
> >>> +
> >>>  /**
> >>>   * \class V4L2Device
> >>>   * \brief V4L2Device object and API

-- 
Regards,

Laurent Pinchart