[libcamera-devel] [PATCH 2/5] libcamera: v4l2_videodevice: Use ImageFormats
Jacopo Mondi
jacopo at jmondi.org
Mon Jun 8 09:59:21 CEST 2020
Hi Laurent,
On Sat, Jun 06, 2020 at 02:27:14AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, May 29, 2020 at 01:03:32PM +0200, Jacopo Mondi wrote:
> > ImageFormats was meant to be used not only for V4L2Subdevice but
> > for video devices as well. Since the introduction of of V4L2PixelFormat
> > the V4L2VideoDevice class and its users have been using a raw map to
> > enumerate and inspect the V4L2VideoDevice formats.
> >
> > Use the ImageFormats<V4L2PixelFormat> specialization in place of a raw
> > map and update its usage in pipeline handlers.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/internal/v4l2_videodevice.h | 2 +-
> > .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++----------
> > src/libcamera/pipeline/simple/simple.cpp | 3 +--
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +--
> > src/libcamera/v4l2_videodevice.cpp | 4 ++--
> > 5 files changed, 12 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index dc259523599c..de4745982e94 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -187,7 +187,7 @@ public:
> >
> > int getFormat(V4L2DeviceFormat *format);
> > int setFormat(V4L2DeviceFormat *format);
> > - std::map<V4L2PixelFormat, std::vector<SizeRange>> formats(uint32_t code = 0);
> > + ImageFormats<V4L2PixelFormat> formats(uint32_t code = 0);
> >
> > int setSelection(unsigned int target, Rectangle *rect);
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index e16a9c7f10d3..64364afb3f7e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -37,8 +37,6 @@ namespace libcamera {
> >
> > LOG_DEFINE_CATEGORY(RPI)
> >
> > -using V4L2PixFmtMap = std::map<V4L2PixelFormat, std::vector<SizeRange>>;
> > -
> > namespace {
> >
> > bool isRaw(PixelFormat &pixFmt)
> > @@ -67,7 +65,7 @@ double scoreFormat(double desired, double actual)
> > return score;
> > }
> >
> > -V4L2DeviceFormat findBestMode(V4L2PixFmtMap &formatsMap, const Size &req)
> > +V4L2DeviceFormat findBestMode(ImageFormats<V4L2PixelFormat> &formatsMap, const Size &req)
> > {
> > double bestScore = 9e9, score;
> > V4L2DeviceFormat bestMode = {};
> > @@ -79,12 +77,11 @@ V4L2DeviceFormat findBestMode(V4L2PixFmtMap &formatsMap, const Size &req)
> > #define PENALTY_UNPACKED 500.0
> >
> > /* Calculate the closest/best mode from the user requested size. */
> > - for (const auto &iter : formatsMap) {
> > - V4L2PixelFormat v4l2Format = iter.first;
> > + for (const auto &v4l2Format : formatsMap.formats()) {
>
> This will impact efficiency, as you will first create a temporary vector
> calling formats(), with an additional but smaller impact when calling
> size() below. Is this required ?
>
No, it's for stylistic reasons mostly, and considering we'll usually
have no more than 2 or 3 formats to iterate on, I considered this a
negligible performance impact.
> It's also a change that isn't described in the commit message, so I'd
> split it to a separate patch if desired.
I can drop this if it's controversial.
Thanks
j
>
> > PixelFormat pixelFormat = v4l2Format.toPixelFormat();
> > const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> >
> > - for (const SizeRange &sz : iter.second) {
> > + for (const SizeRange &sz : formatsMap.sizes(v4l2Format)) {
> > double modeWidth = sz.contains(req) ? req.width : sz.max.width;
> > double modeHeight = sz.contains(req) ? req.height : sz.max.height;
> > double reqAr = static_cast<double>(req.width) / req.height;
> > @@ -427,7 +424,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > * Calculate the best sensor mode we can use based on
> > * the user request.
> > */
> > - V4L2PixFmtMap fmts = data_->unicam_[Unicam::Image].dev()->formats();
> > + ImageFormats<V4L2PixelFormat> fmts = data_->unicam_[Unicam::Image].dev()->formats();
> > V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);
> > PixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat();
> > if (cfg.size != sensorFormat.size ||
> > @@ -481,7 +478,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > *
> > */
> > PixelFormat &cfgPixFmt = config_.at(outSize[i].first).pixelFormat;
> > - V4L2PixFmtMap fmts;
> > + ImageFormats<V4L2PixelFormat> fmts;
> >
> > if (i == maxIndex)
> > fmts = data_->isp_[Isp::Output0].dev()->formats();
> > @@ -518,7 +515,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> > RPiCameraData *data = cameraData(camera);
> > CameraConfiguration *config = new RPiCameraConfiguration(data);
> > V4L2DeviceFormat sensorFormat;
> > - V4L2PixFmtMap fmts;
> > + ImageFormats<V4L2PixelFormat> fmts;
> >
> > if (roles.empty())
> > return config;
> > @@ -605,7 +602,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > }
> >
> > /* First calculate the best sensor mode we can use based on the user request. */
> > - V4L2PixFmtMap fmts = data->unicam_[Unicam::Image].dev()->formats();
> > + ImageFormats<V4L2PixelFormat> fmts = data->unicam_[Unicam::Image].dev()->formats();
> > V4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ? sensorSize : maxSize);
> >
> > /*
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 1ec8d0f7de03..f3e03ba60196 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -275,8 +275,7 @@ int SimpleCameraData::init()
> > return ret;
> > }
> >
> > - std::map<V4L2PixelFormat, std::vector<SizeRange>> videoFormats =
> > - video_->formats(format.mbus_code);
> > + ImageFormats<V4L2PixelFormat> videoFormats = video_->formats(format.mbus_code);
> >
> > LOG(SimplePipeline, Debug)
> > << "Adding configuration for " << format.size.toString()
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index a074909499f1..dbd835f5c5ef 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -159,8 +159,7 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > if (roles.empty())
> > return config;
> >
> > - std::map<V4L2PixelFormat, std::vector<SizeRange>> v4l2Formats =
> > - data->video_->formats();
> > + ImageFormats<V4L2PixelFormat> v4l2Formats = data->video_->formats();
> > std::map<PixelFormat, std::vector<SizeRange>> deviceFormats;
> > std::transform(v4l2Formats.begin(), v4l2Formats.end(),
> > std::inserter(deviceFormats, deviceFormats.begin()),
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 3614b2ed1cbc..ea952444e0ad 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -925,9 +925,9 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> > *
> > * \return A list of the supported video device formats
> > */
> > -std::map<V4L2PixelFormat, std::vector<SizeRange>> V4L2VideoDevice::formats(uint32_t code)
> > +ImageFormats<V4L2PixelFormat> V4L2VideoDevice::formats(uint32_t code)
> > {
> > - std::map<V4L2PixelFormat, std::vector<SizeRange>> formats;
> > + ImageFormats<V4L2PixelFormat> formats;
> >
> > for (V4L2PixelFormat pixelFormat : enumPixelformats(code)) {
> > std::vector<SizeRange> sizes = enumSizes(pixelFormat);
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list