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

[Jacopo Mondi]

Hi Niklas,

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

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(-)
>
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel