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

[Jacopo Mondi]

Hi Laurent,

On Wed, Jan 30, 2019 at 12:39:41PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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.

> > +{
> > +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?

> 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?

> > +};
> > +
> >  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 ?

> > + * 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.

Thanks
  j

> > + */
> > +
> > +/**
> > + * \var V4L2DeviceFormat::planes
> > + * \brief The number of valid data planes
> > + */
> > +
> >  /**
> >   * \class V4L2Device
> >   * \brief V4L2Device object and API
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190201/b9ecc512/attachment.sig>