| Message ID | 20190201154248.15006-8-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 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 ? > + * 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/ ? > * > * Only the first \ref planesCount entries are considered valid. > */
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@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
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@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. > >> */
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, + * but only when applied to devices that support the V4L2 multi-planar APIs. + * When a non-packed image format gets applied to a device that only supports + * the V4L2 single-planar APIs, it is not possible to configure per-plane sizes + * as the only valid informations are contained in the first entry of the \ref + * planes array, and apply to the image as a whole. */ /** * \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 + * image visible sizes to accommodate line or plane padding data. * * Only the first \ref planesCount entries are considered valid. */
Improve the V4L2DeviceFormat documentation as suggested during patches review. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/v4l2_device.cpp | 42 +++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 19 deletions(-)