[PATCH v3 3/4] libcamera: converter: Add DW100 converter class

[Laurent Pinchart]

Hi Umang,

On Wed, Jun 26, 2024 at 09:34:09AM +0530, Umang Jain wrote:
> On 26/06/24 2:26 am, Kieran Bingham wrote:
> > Quoting Umang Jain (2024-06-25 07:23:26)
> >> DW100 Dewarp engine is present on i.MX8MP SoC. This patch
> >> provides a converter class (inherited from V4L2M2MConverter).
> >>
> >> The DW100 converter will be used for scaling capabilites hence, has
> >> a helper to set scaler crop rectangles. Plumbing in the RkISP1
> >> pipeline-handler is done in the subsequent patch.
> >>
> >> The ConverterDW100 class can be extended later (as additional
> >> developement) to support the dewarping by setting a dewarp vertex map.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >>   .../internal/converter/converter_dw100.h      | 26 +++++++++
> >>   .../libcamera/internal/converter/meson.build  |  1 +
> >>   src/libcamera/converter/converter_dw100.cpp   | 56 +++++++++++++++++++
> >>   src/libcamera/converter/meson.build           |  1 +
> >>   4 files changed, 84 insertions(+)
> >>   create mode 100644 include/libcamera/internal/converter/converter_dw100.h
> >>   create mode 100644 src/libcamera/converter/converter_dw100.cpp
> >>
> >> diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h
> >> new file mode 100644
> >> index 00000000..ae6d0732
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/converter/converter_dw100.h
> >> @@ -0,0 +1,26 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2024, Ideas On Board Oy
> >> + *
> >> + *i.MX8MP Dewarp Engine integration
> >
> > s/*i.MX8MP/* i.MX8MP/
> >
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include "libcamera/internal/converter/converter_v4l2_m2m.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +class MediaDevice;
> >> +class Rectangle;
> >> +class Stream;
> >> +
> >> +class ConverterDW100 : public V4L2M2MConverter
> >> +{
> >> +public:
> >> +       ConverterDW100(std::shared_ptr<MediaDevice> media);
> >> +
> >> +       int setScalerCrop(const Stream *stream, Rectangle rect);
> >> +};
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build
> >> index 891e79e7..85007a4b 100644
> >> --- a/include/libcamera/internal/converter/meson.build
> >> +++ b/include/libcamera/internal/converter/meson.build
> >> @@ -1,5 +1,6 @@
> >>   # SPDX-License-Identifier: CC0-1.0
> >>   
> >>   libcamera_internal_headers += files([
> >> +    'converter_dw100.h',
> >>       'converter_v4l2_m2m.h',
> >>   ])
> >> diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp
> >> new file mode 100644
> >> index 00000000..04e6942f
> >> --- /dev/null
> >> +++ b/src/libcamera/converter/converter_dw100.cpp
> >> @@ -0,0 +1,56 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2024, Ideas On Board Oy
> >> + *
> >> + * i.MX8MP Dewarp Engine integration
> >> + */
> >> +
> >> +#include "libcamera/internal/converter/converter_dw100.h"
> >> +
> >> +#include <libcamera/base/log.h>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +
> >> +#include "libcamera/internal/media_device.h"
> >> +#include "libcamera/internal/v4l2_videodevice.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DECLARE_CATEGORY(Converter)
> >> +
> >> +/**
> >> + * \class libcamera::ConverterDW100
> >> + * \brief The i.MX8MP dewarp converter implements the converter interface based
> >> + * on V4L2 M2M device.
> >> +*/
> >> +
> >> +/**
> >> + * \fn ConverterDW100::ConverterDW100
> >> + * \brief Construct a ConverterDW100 instance
> >> + * \param[in] media The media device implementing the converter
> >> + */
> >> +ConverterDW100::ConverterDW100(std::shared_ptr<MediaDevice> media)
> >> +       : V4L2M2MConverter(media.get())
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Set the scaler crop rectangle \a rect on \a stream
> >> + * \param[in] stream Pointer to output stream
> >> + * \param[inout] rect The selection rectangle to be applied
> >> + *
> >> + * \return 0 on success or a negative error code otherwise
> >> + */
> >> +int ConverterDW100::setScalerCrop(const Stream *stream, Rectangle rect)
> >> +{
> >> +       int ret;
> >> +
> >> +       ret = setSelection(stream, V4L2_SEL_TGT_CROP, &rect);
> >> +       if (ret < 0)
> >> +               LOG(Converter, Error) << "Failed to set scaler crop on dewarper "
> >> +                                     << strerror(-ret);
> >> +
> >
> > Does calling setSelection modify the rect? Do we need to check that at
> > all or is it up to the caller to verify?
> 
> I am not sure here to be honest, If the selection rectangles get 
> modified while applying, it's probably the caller's job to verify it.

You're passing the rectangle by value to setScalerCrop(), so the caller
can't check that. Unless we mandate the converter to support arbitrary
cropping with 1-pixel granularity (and I think that's an option we can
consider), this is problematic.

When it comes to crop bounds (and thus scaling ratio bounds), I think
they need to be reported explicitly by the converter.

> Looking up the V4L2_SEL_FLAG_* flags, it seems there are ways to control 
> the modification / constraints of the rectangles that get applied. So 
> yes, a verification mechanism of what gets applied vs what was 
> requested, needs to be present. If the rectangle is modified, we need to 
> also look at  what are the  acceptable % of margins which we should 
> allow - or print a error/warning otherwise?

I don't think just printing a message will solve the problem.

> >> +       return ret;
> >> +}
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build
> >> index 2aa72fe4..175581b8 100644
> >> --- a/src/libcamera/converter/meson.build
> >> +++ b/src/libcamera/converter/meson.build
> >> @@ -1,5 +1,6 @@
> >>   # SPDX-License-Identifier: CC0-1.0
> >>   
> >>   libcamera_sources += files([
> >> +        'converter_dw100.cpp',
> >>           'converter_v4l2_m2m.cpp'
> >>   ])
> >
> > Oh - wow, that's a small implementation! I guess that's the benefit of
> > inheriting from M2MConvertor.
> >
> > That makes me wonder if ScalerCrop should be handled in the
> > V4L2M2MConvertor generically instead ... but ... as we expect this class
> > to be expanded for the dewarp, I think it's fine to be specialised here.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

-- 
Regards,

Laurent Pinchart