| Message ID | 20190128151137.31075-2-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 28/01/2019 15:11, 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@jmondi.org> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > 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 > +{ > +public: > + uint32_t width; > + uint32_t height; > + uint32_t fourcc; I might have called this pixelFormat, but it's internal and it can change if we later define a PixelFormat class or such. Currently we will store a fourcc so this is accurate. > + > + struct { > + uint32_t size; > + uint32_t bpl; > + } planesFmt[3]; > + unsigned int planes; > +}; > + > class MediaEntity; > 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 > + * 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. > + * > + * 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. > + * > + * 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. > + */ > + > +/** > + * \var V4L2DeviceFormat::width > + * \brief The image width > + */ > + > +/** > + * \var V4L2DeviceFormat::height > + * \brief The image height > + */ > + > +/** > + * \var V4L2DeviceFormat::fourcc> + * \brief The pixel encoding scheme > + * > + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros, > + * that identifies the image format pixel encoding scheme. > + */ > + > +/** > + * \var V4L2DeviceFormat::planesFmt > + * \brief The per-plane 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. > + * > + * Only the first V4L2DeviceFormat::planes entries are considered valid. > + * > + */ > + > +/** > + * \var V4L2DeviceFormat::planes > + * \brief The number of valid data planes > + */ > + > /** > * \class V4L2Device > * \brief V4L2Device object and API >
Hi Jacopo, One more thing... On 28/01/2019 15:42, Kieran Bingham wrote: > Hi Jacopo, > > On 28/01/2019 15:11, 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@jmondi.org> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- >> 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 >> +{ >> +public: >> + uint32_t width; >> + uint32_t height; >> + uint32_t fourcc; > > I might have called this pixelFormat, but it's internal and it can > change if we later define a PixelFormat class or such. > > Currently we will store a fourcc so this is accurate. struct v4l2_pix_format *pix stores the following extra information: field (we might need to consider interlaced somewhere?) colorspace, flags ycbcr_enc, hsv_enc quantization xfer_func Should we be handling any of that information here too? Or should we add it on an as-required basis? -- Kieran > >> + >> + struct { >> + uint32_t size; >> + uint32_t bpl; >> + } planesFmt[3]; >> + unsigned int planes; >> +}; >> + >> class MediaEntity; >> 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 >> + * 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. >> + * >> + * 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. >> + * >> + * 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. >> + */ >> + >> +/** >> + * \var V4L2DeviceFormat::width >> + * \brief The image width >> + */ >> + >> +/** >> + * \var V4L2DeviceFormat::height >> + * \brief The image height >> + */ >> + >> +/** >> + * \var V4L2DeviceFormat::fourcc> + * \brief The pixel encoding scheme >> + * >> + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros, >> + * that identifies the image format pixel encoding scheme. >> + */ >> + >> +/** >> + * \var V4L2DeviceFormat::planesFmt >> + * \brief The per-plane 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. >> + * >> + * Only the first V4L2DeviceFormat::planes entries are considered valid. >> + * >> + */ >> + >> +/** >> + * \var V4L2DeviceFormat::planes >> + * \brief The number of valid data planes >> + */ >> + >> /** >> * \class V4L2Device >> * \brief V4L2Device object and API >> >
Hi Kieran, On Mon, Jan 28, 2019 at 03:57:45PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > One more thing... > > On 28/01/2019 15:42, Kieran Bingham wrote: > > Hi Jacopo, > > > > On 28/01/2019 15:11, 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@jmondi.org> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> --- > >> 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 > >> +{ > >> +public: > >> + uint32_t width; > >> + uint32_t height; > >> + uint32_t fourcc; > > > > I might have called this pixelFormat, but it's internal and it can > > change if we later define a PixelFormat class or such. > > > > Currently we will store a fourcc so this is accurate. > > > struct v4l2_pix_format *pix stores the following extra information: Not only v4l2_pix_format, but v4l2_pix_format_mplane and v4l2_mbus_framefmt too... > > field (we might need to consider interlaced somewhere?) > colorspace, > flags > ycbcr_enc, hsv_enc > quantization > xfer_func Right now, I cannot tell. I'm sure there are use cases where those informations are relevant. > > Should we be handling any of that information here too? > > Or should we add it on an as-required basis? I think they just need to be copied back and forth from the ioctl arguments, once the V4L2DeviceFormat has been expanded to include these fields. I'll leave it for later, but if anyone has other opinions we can look into that now. Thanks j > > -- > Kieran > > > > > > >> + > >> + struct { > >> + uint32_t size; > >> + uint32_t bpl; > >> + } planesFmt[3]; > >> + unsigned int planes; > >> +}; > >> + > >> class MediaEntity; > >> 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 > >> + * 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. > >> + * > >> + * 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. > >> + * > >> + * 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. > >> + */ > >> + > >> +/** > >> + * \var V4L2DeviceFormat::width > >> + * \brief The image width > >> + */ > >> + > >> +/** > >> + * \var V4L2DeviceFormat::height > >> + * \brief The image height > >> + */ > >> + > >> +/** > >> + * \var V4L2DeviceFormat::fourcc> + * \brief The pixel encoding scheme > >> + * > >> + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros, > >> + * that identifies the image format pixel encoding scheme. > >> + */ > >> + > >> +/** > >> + * \var V4L2DeviceFormat::planesFmt > >> + * \brief The per-plane 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. > >> + * > >> + * Only the first V4L2DeviceFormat::planes entries are considered valid. > >> + * > >> + */ > >> + > >> +/** > >> + * \var V4L2DeviceFormat::planes > >> + * \brief The number of valid data planes > >> + */ > >> + > >> /** > >> * \class V4L2Device > >> * \brief V4L2Device object and API > >> > > > > -- > Regards > -- > Kieran
Hi Jacopo, On 28/01/2019 16:20, Jacopo Mondi wrote: > Hi Kieran, > > On Mon, Jan 28, 2019 at 03:57:45PM +0000, Kieran Bingham wrote: >> Hi Jacopo, >> >> One more thing... >> >> On 28/01/2019 15:42, Kieran Bingham wrote: >>> Hi Jacopo, >>> >>> On 28/01/2019 15:11, 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@jmondi.org> >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>> --- >>>> 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 >>>> +{ >>>> +public: >>>> + uint32_t width; >>>> + uint32_t height; >>>> + uint32_t fourcc; >>> >>> I might have called this pixelFormat, but it's internal and it can >>> change if we later define a PixelFormat class or such. >>> >>> Currently we will store a fourcc so this is accurate. >> >> >> struct v4l2_pix_format *pix stores the following extra information: > > Not only v4l2_pix_format, but v4l2_pix_format_mplane and > v4l2_mbus_framefmt too... > >> >> field (we might need to consider interlaced somewhere?) >> colorspace, >> flags >> ycbcr_enc, hsv_enc >> quantization >> xfer_func > > Right now, I cannot tell. I'm sure there are use cases where those > informations are relevant. > >> >> Should we be handling any of that information here too? >> >> Or should we add it on an as-required basis? > > I think they just need to be copied back and forth from the ioctl > arguments, once the V4L2DeviceFormat has been expanded to include > these fields. > > I'll leave it for later, but if anyone has other opinions we can look > into that now. Leaving it until it is needed is fine with me. -- Kieran > > Thanks > j > >> >> -- >> Kieran >> >> >> >>> >>>> + >>>> + struct { >>>> + uint32_t size; >>>> + uint32_t bpl; >>>> + } planesFmt[3]; >>>> + unsigned int planes; >>>> +}; >>>> + >>>> class MediaEntity; >>>> 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 >>>> + * 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. >>>> + * >>>> + * 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. >>>> + * >>>> + * 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. >>>> + */ >>>> + >>>> +/** >>>> + * \var V4L2DeviceFormat::width >>>> + * \brief The image width >>>> + */ >>>> + >>>> +/** >>>> + * \var V4L2DeviceFormat::height >>>> + * \brief The image height >>>> + */ >>>> + >>>> +/** >>>> + * \var V4L2DeviceFormat::fourcc> + * \brief The pixel encoding scheme >>>> + * >>>> + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros, >>>> + * that identifies the image format pixel encoding scheme. >>>> + */ >>>> + >>>> +/** >>>> + * \var V4L2DeviceFormat::planesFmt >>>> + * \brief The per-plane 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. >>>> + * >>>> + * Only the first V4L2DeviceFormat::planes entries are considered valid. >>>> + * >>>> + */ >>>> + >>>> +/** >>>> + * \var V4L2DeviceFormat::planes >>>> + * \brief The number of valid data planes >>>> + */ >>>> + >>>> /** >>>> * \class V4L2Device >>>> * \brief V4L2Device object and API >>>> >>> >> >> -- >> Regards >> -- >> Kieran
Hi Jacopo, On Mon, Jan 28, 2019 at 05:20:02PM +0100, Jacopo Mondi wrote: > On Mon, Jan 28, 2019 at 03:57:45PM +0000, Kieran Bingham wrote: > > On 28/01/2019 15:42, Kieran Bingham wrote: > >> On 28/01/2019 15:11, 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@jmondi.org> > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> --- > >>> 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 > >>> +{ > >>> +public: > >>> + uint32_t width; > >>> + uint32_t height; > >>> + uint32_t fourcc; > >> > >> I might have called this pixelFormat, but it's internal and it can > >> change if we later define a PixelFormat class or such. > >> > >> Currently we will store a fourcc so this is accurate. > > > > struct v4l2_pix_format *pix stores the following extra information: > > Not only v4l2_pix_format, but v4l2_pix_format_mplane and > v4l2_mbus_framefmt too... > > > field (we might need to consider interlaced somewhere?) > > colorspace, > > flags > > ycbcr_enc, hsv_enc > > quantization > > xfer_func > > Right now, I cannot tell. I'm sure there are use cases where those > informations are relevant. > > > Should we be handling any of that information here too? > > > > Or should we add it on an as-required basis? > > I think they just need to be copied back and forth from the ioctl > arguments, once the V4L2DeviceFormat has been expanded to include > these fields. > > I'll leave it for later, but if anyone has other opinions we can look > into that now. I'm fine with that. For the record, colorspace is a legacy field that has been replaced with encoding, quantization and xfer function. We should expose the new API through V4L2DeviceFormat when we'll add support for colorspaces. > >>> + > >>> + struct { > >>> + uint32_t size; > >>> + uint32_t bpl; > >>> + } planesFmt[3]; > >>> + unsigned int planes; > >>> +}; > >>> + > >>> class MediaEntity; > >>> 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 > >>> + * 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. > >>> + * > >>> + * 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. > >>> + * > >>> + * 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. > >>> + */ > >>> + > >>> +/** > >>> + * \var V4L2DeviceFormat::width > >>> + * \brief The image width > >>> + */ > >>> + > >>> +/** > >>> + * \var V4L2DeviceFormat::height > >>> + * \brief The image height > >>> + */ > >>> + > >>> +/** > >>> + * \var V4L2DeviceFormat::fourcc> + * \brief The pixel encoding scheme > >>> + * > >>> + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros, > >>> + * that identifies the image format pixel encoding scheme. > >>> + */ > >>> + > >>> +/** > >>> + * \var V4L2DeviceFormat::planesFmt > >>> + * \brief The per-plane 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. > >>> + * > >>> + * Only the first V4L2DeviceFormat::planes entries are considered valid. > >>> + * > >>> + */ > >>> + > >>> +/** > >>> + * \var V4L2DeviceFormat::planes > >>> + * \brief The number of valid data planes > >>> + */ > >>> + > >>> /** > >>> * \class V4L2Device > >>> * \brief V4L2Device object and API
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@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 ? > +{ > +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 however include stdint.h to ensure that v4l2_device.h stays self-contained. > + 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. > +}; > + > 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. > 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" ? > + * > + * 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 ?). > + * 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. > + */ > + > +/** > + * \var V4L2DeviceFormat::planes > + * \brief The number of valid data planes > + */ > + > /** > * \class V4L2Device > * \brief V4L2Device object and API
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@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
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@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
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 +{ +public: + uint32_t width; + uint32_t height; + uint32_t fourcc; + + struct { + uint32_t size; + uint32_t bpl; + } planesFmt[3]; + unsigned int planes; +}; + class MediaEntity; 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 + * 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. + * + * 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. + * + * 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. + */ + +/** + * \var V4L2DeviceFormat::width + * \brief The image width + */ + +/** + * \var V4L2DeviceFormat::height + * \brief The image height + */ + +/** + * \var V4L2DeviceFormat::fourcc + * \brief The pixel encoding scheme + * + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros, + * that identifies the image format pixel encoding scheme. + */ + +/** + * \var V4L2DeviceFormat::planesFmt + * \brief The per-plane 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. + * + * Only the first V4L2DeviceFormat::planes entries are considered valid. + * + */ + +/** + * \var V4L2DeviceFormat::planes + * \brief The number of valid data planes + */ + /** * \class V4L2Device * \brief V4L2Device object and API
Add a V4L2DeviceFormat class aimed to be used to provide format configuration requests to a V4L2Device. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_device.h | 14 ++++++++ src/libcamera/v4l2_device.cpp | 56 +++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+)