| Message ID | 20240712052920.33396-7-umang.jain@ideasonboard.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
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@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"; > > } > > } > >
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); + +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() +{ + int ret; + + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_DEWARP_CONFIG_FILE"); + if (!configFromEnv || *configFromEnv == '\0') + return 0; + + 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++) { + 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"; } }
Load the dewarp vertex maps for different configurations using the LIBCAMERA_DEWARP_CONFIG_FILE 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. 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@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(+)