[libcamera-devel] [PATCH 7/7] libcamera: v4l2_device: Improve documentation

[Jacopo Mondi]

Hi Laurent,

On Sat, Feb 02, 2019 at 12:48:28AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Feb 01, 2019 at 04:42:48PM +0100, Jacopo Mondi wrote:
> > Improve the V4L2DeviceFormat documentation as suggested during patches
> > review.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/v4l2_device.cpp | 42 +++++++++++++++++++----------------
> >  1 file changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 4d1f76b..5ae208f 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -84,48 +84,52 @@ LOG_DEFINE_CATEGORY(V4L2)
> >   * \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.
> > + * This class describes the image format and image sizes to be programmed
> > + * on a V4L2 video device. The image format is defined by a fourcc code
> > + * as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, a visible
> > + * width and height and one to three planes with configurable line stride
> > + * and a total per-plane size in bytes.
> >   *
> > - * Formats defined as 'single planar' by the V4L2 APIs are represented with
> > - * V4L2DeviceFormat instances with a single plane
> > - * (V4L2DeviceFormat::planesCount = 1). Semi-planar and multiplanar formats use
> > - * 2 and 3 planes respectively.
> > + * The V4L2 APIs defines packed, semi-planar and planar image formats.
> > + * Packed formats are stored as a single, contiguous memory area, while
> > + * semi-planar and multi-planar ones use two or three (possibly not contiguous)
> > + * memory regions to store the image components as separate planes.
> >   *
> > - * 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.
> > + * Packed formats are represented by V4L2DeviceFormat instances with a
> > + * single valid \ref planes array entry (\ref planesCount = 1).
> > + * Non-packed formats allows configuring per plane size and line stride length,
>
> s/allows/allow/
> s/stride length/stride/
>
> > + * but only when applied to devices that support the V4L2 multi-planar APIs.
>
> I don't think this is correct. bytesperline can be set for all formats
> (packed and planar), both using the MPLANE and non-MPLANE APIs.
>

When using single plane APIs, the bytesperlane cannot be set -per
plane- I think  the following sentece clarifies that.

> > + * When a non-packed image format gets applied to a device that only supports
>
> I'd write "planar or semi-planar" instead of "non-packed".
>
> > + * the V4L2 single-planar APIs, it is not possible to configure per-plane sizes
>
> Did you mean strides instead of sizes ?

I meant "sizes" as both stride and global plane size. Is this
confusing?

>
> > + * as the only valid informations are contained in the first entry of the \ref
>
> s/informations/information/
>
> > + * planes array, and apply to the image as a whole.
>
> I'd write "[...] per-plane sizes. Only the first entry of the \ref
> planes array is taken into account in that case, and apply to all
> planes, with appropriate subsampling."
>
> >   */
> >
> >  /**
> >   * \var V4L2DeviceFormat::width
> > - * \brief The image width
> > + * \brief The image width in pixels
> >   */
> >
> >  /**
> >   * \var V4L2DeviceFormat::height
> > - * \brief The image height
> > + * \brief The image height in pixels
> >   */
> >
> >  /**
> >   * \var V4L2DeviceFormat::fourcc
> >   * \brief The pixel encoding scheme
> >   *
> > - * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros,
> > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,
> >   * that identifies the image format pixel encoding scheme.
> >   */
> >
> >  /**
> >   * \var V4L2DeviceFormat::planes
> > - * \brief The per-plane size information
> > + * \brief The per-plane memory 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.
> > + * has a specific memory size and line length, which could differ from the
>
> s/memory size and line length/line stride and memory size/
>
> > + * image visible sizes to accommodate line or plane padding data.
>
> s/line or plane padding data/padding at the end of lines and end of the
> plane/ ?
>

Thanks, I'll push the other 6 patches, and re-post this one for
another review cycle.

Thanks
  j

> >   *
> >   * Only the first \ref planesCount entries are considered valid.
> >   */
>
> --
> 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/20190203/fcd446c3/attachment-0001.sig>