[RFC PATCH 6/6] libcamera: converter_dw100: Load and apply dewarp vertex maps
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Aug 3 01:01:10 CEST 2024
On Wed, Jul 24, 2024 at 03:46:48PM +0200, Jacopo Mondi wrote:
> 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
And this should not come from an environment variable :-)
> > 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 ?
No, because the function in the base class isn't volatile.
> > +
> > +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 ?
Usable, yes, but without dewarping. It can still be used as a scaler.
> 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")) {
Is that the resolution before or after the input crop is applied ?
> > + 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));
The YAML parser supports Size natively.
> > + 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) {
This significantly reduces the usability of the dewarper. I'd like to
see a proposal to do better.
> > + 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";
> > }
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list