[RFC PATCH 6/6] libcamera: converter_dw100: Load and apply dewarp vertex maps

[Jacopo Mondi]

Hi Umang

On Fri, Jul 12, 2024 at 10:59:20AM GMT, Umang Jain wrote:
> Load the dewarp vertex maps for different configurations using the
> LIBCAMERA_DEWARP_CONFIG_FILE environment variable.

The env variable needs to be documented

>
> In addition, provide a applyMappings(stream) API for converter_dw100, in
> order to apply mappings for the given stream. Plumb it into rkisp1
> pipeline handler, if the dewarper is being used.
>
> \todo The parsing of dewarp configuration file is yet to be determined.

What are you unsure about here ? The configuration file format ?

> I've used the same parsing logic as made in previous attempts:
> https://patchwork.libcamera.org/patch/17348/
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  .../internal/converter/converter_dw100.h      |   5 +
>  src/libcamera/converter/converter_dw100.cpp   | 127 ++++++++++++++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   4 +
>  3 files changed, 136 insertions(+)
>
> diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h
> index dc41f365..a3062e84 100644
> --- a/include/libcamera/internal/converter/converter_dw100.h
> +++ b/include/libcamera/internal/converter/converter_dw100.h
> @@ -19,6 +19,11 @@ class ConverterDW100 : public V4L2M2MConverter
>  {
>  public:
>  	ConverterDW100(std::shared_ptr<MediaDevice> media);
> +
> +	int applyMappings(const Stream *stream);

Doesn't this 'override' the one declared in the V4L2M2MConverter base
class ?

> +
> +private:
> +	int loadDewarpMaps();
>  };
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp
> index 3061fc71..1a641779 100644
> --- a/src/libcamera/converter/converter_dw100.cpp
> +++ b/src/libcamera/converter/converter_dw100.cpp
> @@ -7,12 +7,17 @@
>
>  #include "libcamera/internal/converter/converter_dw100.h"
>
> +#include <linux/dw100.h>
> +
> +#include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/stream.h>
>  #include <libcamera/geometry.h>
>
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
> +#include "libcamera/internal/yaml_parser.h"
>
>  namespace libcamera {
>
> @@ -32,6 +37,128 @@ LOG_DECLARE_CATEGORY(Converter)
>  ConverterDW100::ConverterDW100(std::shared_ptr<MediaDevice> media)
>  	: V4L2M2MConverter(media.get(), Feature::Crop)
>  {
> +	loadDewarpMaps();
> +}
> +
> +int ConverterDW100::loadDewarpMaps()

Return value is useless if only called at construction time

> +{
> +	int ret;
> +
> +	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_DEWARP_CONFIG_FILE");
> +	if (!configFromEnv || *configFromEnv == '\0')
> +		return 0;

Is the dewarper usable without a config file ?

Should we have an example of a mapping file somewhere or it is not
needed ?

> +
> +	LOG(Converter, Info) << "Parsing dewarp configuration file " << configFromEnv;
> +
> +	std::string filename = std::string(configFromEnv);
> +	File file(filename);
> +
> +	if (!file.open(File::OpenModeFlag::ReadOnly)) {
> +		ret = file.error();
> +		LOG(Converter, Error) << "Failed to open configuration file "
> +				      << filename << ": " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	std::unique_ptr<libcamera::YamlObject> data = YamlParser::parse(file);
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (!data->contains("mappings")) {
> +		LOG(Converter, Error) << "Vertex mapping key missing";
> +		return -EINVAL;
> +	}
> +
> +	const YamlObject &mappings = (*data)["mappings"];
> +	if (!mappings.isList() || mappings.size() == 0) {
> +		LOG(Converter, Error) << "Invalid mappings entry";
> +		return -EINVAL;
> +	}
> +
> +	LOG(Converter, Debug) << "Parsing " << mappings.size() << " mappings";
> +	mappings_.clear();
> +	mappings_.reserve(mappings.size());
> +
> +	for (std::size_t i = 0; i < mappings.size(); i++) {

Does
        for (const YamlObject &mapping : mappings)

work ?

> +		const YamlObject &mapping = mappings[i];
> +		if (!mapping.isDictionary()) {
> +			LOG(Converter, Error) << "Mapping is not a dictionary";
> +			return -EINVAL;
> +		}
> +
> +		if (!mapping.contains("input-resolution")) {
> +			LOG(Converter, Error) << "Input resolution missing";
> +			return -EINVAL;
> +		}
> +
> +		if (!mapping.contains("output-resolution")) {
> +			LOG(Converter, Error) << "Output resolution missing";
> +			return -EINVAL;
> +		}
> +
> +		if (!mapping.contains("mapping")) {
> +			LOG(Converter, Error) << "Mapping table missing";
> +			return -EINVAL;
> +		}
> +
> +		const YamlObject &input_res = mapping["input-resolution"];
> +		if (!input_res.isList() || input_res.size() != 2) {
> +			LOG(Converter, Error) << "Incorrect input resolution";
> +			return -EINVAL;
> +		}
> +
> +		const YamlObject &output_res = mapping["output-resolution"];
> +		if (!output_res.isList() || output_res.size() != 2) {
> +			LOG(Converter, Error) << "Incorrect output resolution";
> +			return -EINVAL;
> +		}
> +
> +		const YamlObject &map = mapping["mapping"];
> +		if (!map.isList() || map.size() == 0) {
> +			LOG(Converter, Error) << "Incorrect mapping entries";
> +			return -EINVAL;
> +		}
> +
> +		Size input(input_res[0].get<uint32_t>(0), input_res[1].get<uint32_t>(0));
> +		Size output(output_res[0].get<uint32_t>(0), output_res[1].get<uint32_t>(0));
> +		const auto &mapVector = map.getList<uint32_t>().value_or(std::vector<uint32_t>{});
> +
> +		LOG(Converter, Debug)
> +			<< "Input/Output mapping resolution " << input << " -> " << output;
> +
> +		mappings_.emplace_back(Mapping(input, output, mapVector));
> +	}
> +
> +	return mappings.size();
> +}
> +
> +/*
> + * \brief Apply
> + * \todo this is just a wrapper, trying to test waters
> + * \param[in] media The media device implementing the converter
> + */
> +int ConverterDW100::applyMappings(const Stream *stream)
> +{
> +	const StreamConfiguration &config = stream->configuration();
> +	ControlList ctrls;
> +	int ret = 0;
> +
> +	for (const auto &map : mappings_) {
> +		/* Currently DW100's input and output configuration are same. */
> +		if (map.inputSize() == config.size &&
> +		    map.outputSize() == config.size) {
> +			auto value = Span<const int32_t>(reinterpret_cast<const int32_t *>(map.mapping()), map.size());
> +
> +			ControlValue c(value);
> +			ctrls.set(V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP, c);
> +			ret = applyControls(stream, ctrls);
> +
> +			LOG(Converter, Debug) << "Dewarp mapping applied for " << config.toString();
> +			break;
> +		}
> +	}
> +
> +       return ret;
>  }
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 881e10e1..f102b364 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1019,6 +1019,10 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>  				LOG(RkISP1, Error) << "Failed to start dewarper";
>  				return ret;
>  			}
> +
> +			ret = dewarper_->applyMappings(&data->mainPathStream_);
> +			if (ret)
> +				LOG(RkISP1, Warning) << "Dewarper mapping couldn't be applied";
>  		}
>  	}
>
> --
> 2.45.0
>