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

[Laurent Pinchart]

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(-)
> 

-- 
Regards,

Laurent Pinchart