| Message ID | 20200701211650.1002567-1-niklas.soderlund@ragnatech.se |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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(-)
Hi again, 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 > 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]. > Happy to see this series getting traction, but to get it merged it should address the custom maps used by V4L2VideoDevice users, otherwise the main goal of unifying the handling maps of mbus codes and v4l2 fourccs is lost. > 1. [PATCH v2 0/2] libcamera: utils: Add map_keys() function > > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel