[libcamera-devel] [RFC 0/2] libcamera: formats: Remove ImageFormats

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jul 02, 2020 at 10:42:54AM +0200, Jacopo Mondi wrote:
> On Wed, Jul 01, 2020 at 11:16:48PM +0200, Niklas Söderlund wrote:
> > Hello,
> >
> > This is in no way an attempt to conflict with the pending patches to
> > ImageFormats on the ML from Jacopo. Following his patches there was a
> 
> Oh it should, this is much better :)
> 
> > discussion if we could instead of making ImageFormats a template make
> > use of the new utils::map_keys() helper. I liked the idea and gave it a
> > try and the result was not so bad so I thought I send it out as an RFC.
> >
> > As the new helper is not yet merged this series depends on [1].
> >
> > 1. [PATCH v2 0/2] libcamera: utils: Add map_keys() function
> 
> I'm still a little debated here.
> 
> One direction is to use plain std::map with type helpers. Then we'll
> have V4L2VideoDevice::Formats, V4L2VideoSubdevice::Formats and
> (hopefully) Stream::Formats as I really hope we could find a way to
> get rid of StreamFormats, as most likely ImageFormats, they're halfway
> between a custom class and a map, and I agree this is bad.
> 
> Exposing an std::map and it's API will frown-upon all attempt to expand
> the *::Formats API whenever we deem necessary. This might be bad, but
> not that nasty.
> 
> I'm toying with another ideas here. If I look at the current usage of
> V4L2*Device::Formats it seems to me that the pattern is pretty
> straightforward: we ask for keys, ask for sizes by key, and sometimes
> as for the whole map to transform keys in something else
> (V4L2PixelFormat to PixelFormat, mostly). Can all of this go through
> the video (sub)device and hide the map handling behind a few accessors
> methods ?
> 
> So we'll have
>         std::vector<PixelFormats> formats = device->pixelFormats()

You'll have to duplicate that in two classes, and it would also
duplicate device->formats() that gives the same information (and more).
If helpers are needed to access information contained in the formats for
different use cases, I think it's best to have those helpers provided by
the container and not the class that initially provides the information.

> in place of
>         V4L2VideoDevice::Formats formats = device->formats();
>         std::vector<PixelFormats> pixFormats = formats.map_keys()
> 
> This would not solve Stream, but I think it could be treated
> differently, being application API, we'll probably need something more
> than a map there.
> 
> > Niklas Söderlund (2):
> >   libcamera: v4l2_subdevice: Replace ImageFormats with a map
> >   libcamera: formats: Remove ImageFormats
> >
> >  include/libcamera/internal/camera_sensor.h  |  6 +-
> >  include/libcamera/internal/formats.h        | 14 ----
> >  include/libcamera/internal/v4l2_subdevice.h |  4 +-
> >  src/libcamera/camera_sensor.cpp             | 12 +--
> >  src/libcamera/formats.cpp                   | 88 ---------------------
> >  src/libcamera/v4l2_subdevice.cpp            | 16 ++--
> >  test/v4l2_subdevice/list_formats.cpp        | 16 ++--
> >  7 files changed, 27 insertions(+), 129 deletions(-)

-- 
Regards,

Laurent Pinchart