[libcamera-devel,0/5] ImageFormats' not dead

3877 mbox series

Hello, this might be controversial, as ImageFormats has been a target for
deprecation since a long time. But as of now, it's still alive and kicking, and
it has a few merits in my opinion, so this series tries to refactor it and its
users to make it nicer to work with.

First and foremost, ImageFormats was designed to be used by both video devices
and video subdevices, but since the introduction of V4L2PixelFormat the formats
map cannot be represented anymore with ImageFormats, as it indexes the image
resolutions with a numerical index. As a consequence, pipeline handlers and
V4L2VideoDevice fell back on using a raw map in place of ImageFormats.

To recover the situation, ImageFormats is turned into a templated class, capable
of indexing image resolutions with keys of variadic type.

This allows reintroducing ImageFormats in V4L2VideoDevice, with its
specialization ImageFormats<V4l2PixelFormats>.

On top of this the last three patches renames ImageFormats, provides typedef
for V4L2VideoDevice and V4L2Subdevice formats and renames the formats() method
in those classes whose name collides with ImageFormats::formats.

The new usage pattern looks like

V4L2VideoDevice::formatsMap = device_->imageFormats();
for (const V4L2PixelFormat &v4lFormat : formatsMaps().formats()) {
	for (const SizeRange &sizes : formatsMap.sizes(v4l2Format) {

	}
}

compared to

std::map<V4L2PixelFormat, std::vector<SizeRange>> formats = device_->formats();
for (const auto it: formats) {
	V4L2PixelFormat &v4l2Format = it.first;
	for (const SizeRange &sizes : it.second) {

	}
}

While for subdevice it stays similar

V4L2Subdevice::formatsMap formatsMap = subdev_->imageFormats();
for (const uint32_t code : formatsMap().formats()) {
	for (const SizeRange &sizes : formatsMap.sizes(v4l2Format) {

	}
}

Thanks
   j

Jacopo Mondi (5):
  libcamera: formats: Make ImageFormats a templated class
  libcamera: v4l2_videodevice: Use ImageFormats
  libcamera: Rename ImageFormats
  libcamera: v4l2_device: Add formatsMap typedef
  libcamera: v4l2_device: Rename formats() method

 include/libcamera/internal/camera_sensor.h    |   6 +-
 include/libcamera/internal/formats.h          |  66 ++++++++--
 include/libcamera/internal/v4l2_subdevice.h   |   4 +-
 include/libcamera/internal/v4l2_videodevice.h |   4 +-
 src/libcamera/camera_sensor.cpp               |   2 +-
 src/libcamera/formats.cpp                     | 119 +++++++++++-------
 .../pipeline/raspberrypi/raspberrypi.cpp      |  24 ++--
 src/libcamera/pipeline/simple/simple.cpp      |   3 +-
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |   3 +-
 src/libcamera/v4l2_subdevice.cpp              |  11 +-
 src/libcamera/v4l2_videodevice.cpp            |   9 +-
 test/v4l2_subdevice/list_formats.cpp          |   8 +-
 12 files changed, 175 insertions(+), 84 deletions(-)

--
2.26.2

Hi Jacopo,

Thank you for the patch.

On Fri, May 29, 2020 at 01:03:30PM +0200, Jacopo Mondi wrote:
> Hello, this might be controversial, as ImageFormats has been a target for
> deprecation since a long time. But as of now, it's still alive and kicking, and
> it has a few merits in my opinion, so this series tries to refactor it and its
> users to make it nicer to work with.

I still think (hope ?) we'll come up with something better in the long
term, but if the class has value for now, I certainly welcome a good
refactoring :-)

> First and foremost, ImageFormats was designed to be used by both video devices
> and video subdevices, but since the introduction of V4L2PixelFormat the formats
> map cannot be represented anymore with ImageFormats, as it indexes the image
> resolutions with a numerical index. As a consequence, pipeline handlers and
> V4L2VideoDevice fell back on using a raw map in place of ImageFormats.
> 
> To recover the situation, ImageFormats is turned into a templated class, capable
> of indexing image resolutions with keys of variadic type.
> 
> This allows reintroducing ImageFormats in V4L2VideoDevice, with its
> specialization ImageFormats<V4l2PixelFormats>.
> 
> On top of this the last three patches renames ImageFormats, provides typedef
> for V4L2VideoDevice and V4L2Subdevice formats and renames the formats() method
> in those classes whose name collides with ImageFormats::formats.
> 
> The new usage pattern looks like
> 
> V4L2VideoDevice::formatsMap = device_->imageFormats();
> for (const V4L2PixelFormat &v4lFormat : formatsMaps().formats()) {
> 	for (const SizeRange &sizes : formatsMap.sizes(v4l2Format) {
> 
> 	}
> }
> 
> compared to
> 
> std::map<V4L2PixelFormat, std::vector<SizeRange>> formats = device_->formats();
> for (const auto it: formats) {
> 	V4L2PixelFormat &v4l2Format = it.first;
> 	for (const SizeRange &sizes : it.second) {
> 
> 	}
> }
> 
> While for subdevice it stays similar
> 
> V4L2Subdevice::formatsMap formatsMap = subdev_->imageFormats();
> for (const uint32_t code : formatsMap().formats()) {
> 	for (const SizeRange &sizes : formatsMap.sizes(v4l2Format) {
> 
> 	}
> }
> 
> Jacopo Mondi (5):
>   libcamera: formats: Make ImageFormats a templated class
>   libcamera: v4l2_videodevice: Use ImageFormats
>   libcamera: Rename ImageFormats
>   libcamera: v4l2_device: Add formatsMap typedef
>   libcamera: v4l2_device: Rename formats() method
> 
>  include/libcamera/internal/camera_sensor.h    |   6 +-
>  include/libcamera/internal/formats.h          |  66 ++++++++--
>  include/libcamera/internal/v4l2_subdevice.h   |   4 +-
>  include/libcamera/internal/v4l2_videodevice.h |   4 +-
>  src/libcamera/camera_sensor.cpp               |   2 +-
>  src/libcamera/formats.cpp                     | 119 +++++++++++-------
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  24 ++--
>  src/libcamera/pipeline/simple/simple.cpp      |   3 +-
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |   3 +-
>  src/libcamera/v4l2_subdevice.cpp              |  11 +-
>  src/libcamera/v4l2_videodevice.cpp            |   9 +-
>  test/v4l2_subdevice/list_formats.cpp          |   8 +-
>  12 files changed, 175 insertions(+), 84 deletions(-)
>