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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Feb 01, 2019 at 11:07:12AM +0100, Jacopo Mondi wrote:
> On Wed, Jan 30, 2019 at 12:39:41PM +0200, Laurent Pinchart wrote:
> > On Mon, Jan 28, 2019 at 04:11:36PM +0100, 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>
> >> ---
> >>  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
> >
> > Should this be named V4L2Format ?
> 
> As I expect a V4L2SubdeviceFormat, I would keep this as
> V4L2DeviceFormat.

The kernel API has v4l2_format and v4l2_subdev_format, but that's also
because subdevs were introduced later. V4L2DeviceFormat makes sense too.

> >> +{
> >> +public:
> >> +	uint32_t width;
> >> +	uint32_t height;
> >
> > I would have gone for unsigned int instead of uint32_t as the exact
> > integer type isn't really relevant, but uint32_t is fine too. You should
> 
> I went for the lenght-specific type because the v4l2 structures has
> length specified. If not that relevant I can change back to unsigned
> int maybe?

I'm fine either way.

> > however include stdint.h to ensure that v4l2_device.h stays
> > self-contained.
> 
> I recall I had to add it, then it's not here anymore...
> 
> >> +	uint32_t fourcc;
> >> +
> >> +	struct {
> >> +		uint32_t size;
> >> +		uint32_t bpl;
> >> +	} planesFmt[3];
> >> +	unsigned int planes;
> >
> > I would have named the first field planes and the second numPlanes or
> > planesCount.
> 
> or planesNum?

Possibly, although "number of planes" sounds better to me than "planes
number".

> >> +};
> >> +
> >>  class MediaEntity;
> >
> > Should we keep all forward declarations at the top of the file ?
> > Otherwise we'll likely end up with duplicated forward declarations at
> > some point.
> 
> I will change this library-wide.
> 
> >>  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
> >
> > "This class describes ...3
> >
> > It would be fine for the \brief text, but the documentation body should
> > be made of full sentences.
> >
> >> + * video device. The image format is defined by fourcc code as defined by
> >
> > s/fourcc code/a fourcc code/
> >
> > Or maybe "a fourcc" (or "a 4CC" or "a FOURCC" ?), as the last C in
> > fourcc stands for code already.
> >
> >> + * the V4L2 APIs with the V4L2_PIX_FMT_ macros, a visible width and height
> >
> > s/V4L2_PIX_FMT_/V4L2_PIX_FMT_*/
> >
> >> + * and a variable number of planes (1 to 3) with variable sizes and line
> >> + * strides.
> >
> > I wouldn't say variable as they don't really vary. Maybe "and one to
> > three planes with configurable line stride and total size for each
> > plane" ?
> 
> ack
> 
> >> + *
> >> + * 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.
> >
> > I would add one paragraph here to explain that line and plane padding
> > may not be separately configurable, depending on the device (devices
> > that don't support the MPLANE API have a fixed relationship between
> > padding for different planes), and how this works in that case (do we
> > ignore padding information for planes > 0 ? do we require them to be
> > specified an in sync, and return an error otherwise ?).
> 
> I'm not sure I got you here.
> Why devices that do not support MPLANE should care about planes > 1 ?

Because the non-MPLANE API supports multi-planar formats (such as NV12
for instance). The different between the two APIs is that only the
MPLANE API allows multi-planar formats to be handled with separate
memory regions for each plane.

> >> + * 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.
> >
> > Shouldn't we define the class as a way to pass format information to
> > V4L2Device, without caring about what's on the other side (and thus
> > without mentioning it's an exchange format) ?
> >
> >> + */
> >> +
> >> +/**
> >> + * \var V4L2DeviceFormat::width
> >> + * \brief The image width
> >
> > The image width in pixels
> >
> >> + */
> >> +
> >> +/**
> >> + * \var V4L2DeviceFormat::height
> >> + * \brief The image height
> >
> > In pixels here too.
> >
> >> + */
> >> +
> >> +/**
> >> + * \var V4L2DeviceFormat::fourcc
> >> + * \brief The pixel encoding scheme
> >> + *
> >> + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros,
> >
> > s/APIs/API/
> > s/V4L2_PIX_FMT_/V4L2_PIX_FMT_*/
> >
> >> + * that identifies the image format pixel encoding scheme.
> >> + */
> >> +
> >> +/**
> >> + * \var V4L2DeviceFormat::planesFmt
> >> + * \brief The per-plane size information
> >
> > I'd mention "memory size" instead of just "size" to make it clear this
> > doesn't refer to the size in pixels.
> >
> >> + *
> >> + * 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
> >
> > s/size/memory size/ here too.
> >
> >> + * visible sizes to accommodate line or plane padding data.
> >> + *
> >> + * Only the first V4L2DeviceFormat::planes entries are considered valid.
> >
> > You could also write "\ref planes".
> >
> >> + *
> >
> > Extra blank line.
> 
> Thanks, please clarify the question I didn't get and I'll send a
> fixups series on top of this.

Done :-) Thank you.

> >> + */
> >> +
> >> +/**
> >> + * \var V4L2DeviceFormat::planes
> >> + * \brief The number of valid data planes
> >> + */
> >> +
> >>  /**
> >>   * \class V4L2Device
> >>   * \brief V4L2Device object and API

-- 
Regards,

Laurent Pinchart