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

[Laurent Pinchart]

Hi Jacopo,

On Sun, Feb 03, 2019 at 12:22:02PM +0100, Jacopo Mondi wrote:
> On Sat, Feb 02, 2019 at 12:48:28AM +0200, Laurent Pinchart wrote:
> > 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.

Re-reading the text I now understand what you meant. I think it's a bit
confusing.

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

With the single-planar API is used, sizeimage is set by the driver, not
the application, so it can't be configured, isn't it ?

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

Thank you.

> >>   *
> >>   * Only the first \ref planesCount entries are considered valid.
> >>   */

-- 
Regards,

Laurent Pinchart