Message ID | 20240627134656.582462-5-umang.jain@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
On Thu, Jun 27, 2024 at 07:16:55PM +0530, Umang Jain wrote: > 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 > development) to support the dewarping by setting a dewarp vertex map. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > .../internal/converter/converter_dw100.h | 26 ++++++++ > .../libcamera/internal/converter/meson.build | 1 + > src/libcamera/converter/converter_dw100.cpp | 64 +++++++++++++++++++ > src/libcamera/converter/meson.build | 1 + > 4 files changed, 92 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..dbd39e08 > --- /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 > + */ > + > +#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..78802615 > --- /dev/null > +++ b/src/libcamera/converter/converter_dw100.cpp > @@ -0,0 +1,64 @@ > +/* 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(), Feature::Crop) > +{ > +} > + > +/** > + * \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 rect doesn't seem to be used as an output parameter? > + * > + * \return 0 on success or a negative error code otherwise > + */ > +int ConverterDW100::setScalerCrop(const Stream *stream, Rectangle *rect) > +{ > + Rectangle apply = *rect; This dereference scares me but I see you want to use it as an output parameter (I assume to report the rectangle that was actually set). > + int ret; > + > + ret = setCrop(stream, &apply); > + if (ret < 0) > + LOG(Converter, Error) << "Failed to set scaler crop " > + << apply.toString() << " on dewarper " > + << strerror(-ret); Can't we just return here? > + > + if (*rect != apply) { > + /* \todo This case needs proper after handling. */ I think it's ok to just return the scaler crop that was able to be set. Paul > + LOG(Converter, Warning) << "Applied rectangle " << apply.toString() > + << " differs from requested " << rect->toString(); > + } > + > + 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' > ]) > -- > 2.44.0 >
Hi Paul On 02/07/24 6:53 pm, Paul Elder wrote: > On Thu, Jun 27, 2024 at 07:16:55PM +0530, Umang Jain wrote: >> 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 >> development) to support the dewarping by setting a dewarp vertex map. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> .../internal/converter/converter_dw100.h | 26 ++++++++ >> .../libcamera/internal/converter/meson.build | 1 + >> src/libcamera/converter/converter_dw100.cpp | 64 +++++++++++++++++++ >> src/libcamera/converter/meson.build | 1 + >> 4 files changed, 92 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..dbd39e08 >> --- /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 >> + */ >> + >> +#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..78802615 >> --- /dev/null >> +++ b/src/libcamera/converter/converter_dw100.cpp >> @@ -0,0 +1,64 @@ >> +/* 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(), Feature::Crop) >> +{ >> +} >> + >> +/** >> + * \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 > rect doesn't seem to be used as an output parameter? > >> + * >> + * \return 0 on success or a negative error code otherwise >> + */ >> +int ConverterDW100::setScalerCrop(const Stream *stream, Rectangle *rect) >> +{ >> + Rectangle apply = *rect; > This dereference scares me but I see you want to use it as an output > parameter (I assume to report the rectangle that was actually set). This setScalerCrop() something can be dropped, to directly call setCrop() of the converter by the consumer. But first, I want to just check what should be the handling of the below \todo w.r.t to applied rectangles, hence I kept a additional wrapper around setCrop() for now. > >> + int ret; >> + >> + ret = setCrop(stream, &apply); >> + if (ret < 0) >> + LOG(Converter, Error) << "Failed to set scaler crop " >> + << apply.toString() << " on dewarper " >> + << strerror(-ret); > Can't we just return here? > >> + >> + if (*rect != apply) { >> + /* \todo This case needs proper after handling. */ > I think it's ok to just return the scaler crop that was able to be set. I think that too and report that as metadata. > > > Paul > >> + LOG(Converter, Warning) << "Applied rectangle " << apply.toString() >> + << " differs from requested " << rect->toString(); >> + } >> + >> + 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' >> ]) >> -- >> 2.44.0 >>
On Wed, Jul 03, 2024 at 09:02:56AM +0530, Umang Jain wrote: > Hi Paul > > On 02/07/24 6:53 pm, Paul Elder wrote: > > On Thu, Jun 27, 2024 at 07:16:55PM +0530, Umang Jain wrote: > > > 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 > > > development) to support the dewarping by setting a dewarp vertex map. > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > .../internal/converter/converter_dw100.h | 26 ++++++++ > > > .../libcamera/internal/converter/meson.build | 1 + > > > src/libcamera/converter/converter_dw100.cpp | 64 +++++++++++++++++++ > > > src/libcamera/converter/meson.build | 1 + > > > 4 files changed, 92 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..dbd39e08 > > > --- /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 > > > + */ > > > + > > > +#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..78802615 > > > --- /dev/null > > > +++ b/src/libcamera/converter/converter_dw100.cpp > > > @@ -0,0 +1,64 @@ > > > +/* 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(), Feature::Crop) > > > +{ > > > +} > > > + > > > +/** > > > + * \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 > > rect doesn't seem to be used as an output parameter? > > > > > + * > > > + * \return 0 on success or a negative error code otherwise > > > + */ > > > +int ConverterDW100::setScalerCrop(const Stream *stream, Rectangle *rect) > > > +{ > > > + Rectangle apply = *rect; > > This dereference scares me but I see you want to use it as an output > > parameter (I assume to report the rectangle that was actually set). > > This setScalerCrop() something can be dropped, to directly call setCrop() > of the converter by the consumer. Oh right yeah that's true. The caller would be responsible for checking the capability. Paul > > But first, I want to just check what should be the handling of the below > \todo w.r.t to applied rectangles, hence I kept a additional wrapper around > setCrop() for now. > > > > > > + int ret; > > > + > > > + ret = setCrop(stream, &apply); > > > + if (ret < 0) > > > + LOG(Converter, Error) << "Failed to set scaler crop " > > > + << apply.toString() << " on dewarper " > > > + << strerror(-ret); > > Can't we just return here? > > > > > + > > > + if (*rect != apply) { > > > + /* \todo This case needs proper after handling. */ > > I think it's ok to just return the scaler crop that was able to be set. > > I think that too and report that as metadata. > > > > > > Paul > > > > > + LOG(Converter, Warning) << "Applied rectangle " << apply.toString() > > > + << " differs from requested " << rect->toString(); > > > + } > > > + > > > + 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' > > > ]) > > > -- > > > 2.44.0 > > > >
diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h new file mode 100644 index 00000000..dbd39e08 --- /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 + */ + +#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..78802615 --- /dev/null +++ b/src/libcamera/converter/converter_dw100.cpp @@ -0,0 +1,64 @@ +/* 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(), Feature::Crop) +{ +} + +/** + * \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) +{ + Rectangle apply = *rect; + int ret; + + ret = setCrop(stream, &apply); + if (ret < 0) + LOG(Converter, Error) << "Failed to set scaler crop " + << apply.toString() << " on dewarper " + << strerror(-ret); + + if (*rect != apply) { + /* \todo This case needs proper after handling. */ + LOG(Converter, Warning) << "Applied rectangle " << apply.toString() + << " differs from requested " << rect->toString(); + } + + 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' ])
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 development) to support the dewarping by setting a dewarp vertex map. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- .../internal/converter/converter_dw100.h | 26 ++++++++ .../libcamera/internal/converter/meson.build | 1 + src/libcamera/converter/converter_dw100.cpp | 64 +++++++++++++++++++ src/libcamera/converter/meson.build | 1 + 4 files changed, 92 insertions(+) create mode 100644 include/libcamera/internal/converter/converter_dw100.h create mode 100644 src/libcamera/converter/converter_dw100.cpp