[libcamera-devel] [PATCH 09/14] libcamera: converter: Introduce dw100 converter

[Laurent Pinchart]

Hi Xavier,

On Fri, Oct 07, 2022 at 02:52:19PM +0200, Xavier Roumegue (OSS) wrote:
> On 10/4/22 01:02, Laurent Pinchart wrote:
> > On Thu, Sep 08, 2022 at 08:48:45PM +0200, Xavier Roumegue via libcamera-devel wrote:
> >> Add converter support for the Vivante DW100 Dewarp Processor IP core
> >> found on i.MX8MP SoC.
> >>
> >> The processor core applies a programmable geometrical transformation on
> >> input images to correct distorsion introduced by lenses.
> >> The transformation function is exposed as a grid map with 16x16 pixel
> >> macroblocks indexed using X, Y vertex coordinates.
> >>
> >> A set of input/output vertices mapping can be loaded at runtime through
> >> a configuration file. If no mapping is loaded, the vertices mapping
> >> fallbacks to an identity transformation. Scaling and pixel format
> >> conversion can be used independently of the vertices remapping.
> >>
> >> Signed-off-by: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
> >> ---
> >>   include/libcamera/internal/converter_dw100.h | 25 +++++++++++++++
> >>   include/libcamera/internal/meson.build       |  1 +
> >>   src/libcamera/converter_dw100.cpp            | 32 ++++++++++++++++++++
> >>   src/libcamera/meson.build                    |  1 +
> >>   4 files changed, 59 insertions(+)
> >>   create mode 100644 include/libcamera/internal/converter_dw100.h
> >>   create mode 100644 src/libcamera/converter_dw100.cpp
> >>
> >> diff --git a/include/libcamera/internal/converter_dw100.h b/include/libcamera/internal/converter_dw100.h
> >> new file mode 100644
> >> index 00000000..1972d6a2
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/converter_dw100.h
> >> @@ -0,0 +1,25 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright 2022 NXP
> >> + *
> >> + * converter_dw100.h - V4l2 M2M dw100 format converter interface
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include <linux/dw100.h>
> >> +
> >> +#include "libcamera/internal/converter_v4l2_m2m.h"
> >> +#include "libcamera/internal/media_device.h"
> > 
> > You can drop this header, it's guaranteed to be included by
> > libcamera/internal/converter_v4l2_m2m.h as the MediaDevice is passed to
> > the V4L2M2MConverter constructor.
> > 
> >> +
> >> +namespace libcamera {
> >> +
> >> +class DW100Converter : public V4L2M2MConverter
> >> +{
> >> +public:
> >> +	DW100Converter(MediaDevice *media)
> >> +		: V4L2M2MConverter(media){};
> > 
> > 	DW100Converter(MediaDevice *media)
> > 		: V4L2M2MConverter(media)
> > 	{
> > 	};
> > 
> 
> That's not aligned with what requests utils/checkstype.py
> 
> @@ -39,9 +37,7 @@
> 
>   public:
>   	DW100Converter(MediaDevice *media)
> -		: V4L2M2MConverter(media)
> -	{
> -	};
> +		: V4L2M2MConverter(media){};
> 
> Either way is fine for me... So ?

I need to find out how to get clang-format to behave the way we want
here. Please ignore the recommendation from checkstyle.py here, and also
drop the ; after } as that's not needed.

> >> +	virtual int applyMapping(Stream *stream, Mapping &mapping) override;
> > 
> > s/virtual //
> > 
> >> +};
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> >> index 132de5ef..c2dd094f 100644
> >> --- a/include/libcamera/internal/meson.build
> >> +++ b/include/libcamera/internal/meson.build
> >> @@ -20,6 +20,7 @@ libcamera_internal_headers = files([
> >>       'control_serializer.h',
> >>       'control_validator.h',
> >>       'converter.h',
> >> +    'converter_dw100.h',
> >>       'converter_v4l2_m2m.h',
> >>       'delayed_controls.h',
> >>       'device_enumerator.h',
> >> diff --git a/src/libcamera/converter_dw100.cpp b/src/libcamera/converter_dw100.cpp
> >> new file mode 100644
> >> index 00000000..b079fb37
> >> --- /dev/null
> >> +++ b/src/libcamera/converter_dw100.cpp
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright 2022 NXP
> >> + *
> >> + * converter_dw100.cpp - V4L2 M2M dw100 format converter
> >> + */
> >> +
> >> +#include "libcamera/internal/converter_dw100.h"
> >> +
> >> +#include <libcamera/base/log.h>
> >> +
> >> +#include <libcamera/controls.h>
> >> +
> >> +#include "libcamera/internal/v4l2_videodevice.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DECLARE_CATEGORY(Converter)
> > 
> > Not used.
> > 
> >> +
> >> +int DW100Converter::applyMapping(Stream *stream, Mapping &mapping)
> >> +{
> >> +	ControlList ctrls;
> >> +	auto value = Span<const int32_t>(reinterpret_cast<const int32_t *>(mapping.getMapping()), mapping.getLength());
> >> +	ControlValue c(value);
> >> +	ctrls.set(V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP, c);
> >> +	stream->m2m_->capture()->setControls(&ctrls);
> >> +	return 0;
> >> +}
> > 
> > As we have a single converter that can perform dewarping, I'd prefer
> > moving the Mapping support from V4L2M2MConverter to this class. It's
> > hard to predict what other dewarpers will need, and designing an API
> > based on a single example usually doesn't produce the best design. We
> > can always refactor the code later when we'll get a second device.
> > 
> >> +
> >> +REGISTER_CONVERTER("dw100", DW100Converter)
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index b12c8401..83da3e5f 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -14,6 +14,7 @@ libcamera_sources = files([
> >>       'control_serializer.cpp',
> >>       'control_validator.cpp',
> >>       'converter.cpp',
> >> +    'converter_dw100.cpp',
> >>       'converter_v4l2_m2m.cpp',
> >>       'delayed_controls.cpp',
> >>       'device_enumerator.cpp',

-- 
Regards,

Laurent Pinchart