| Message ID | 20210131224702.8838-16-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Feb 01, 2021 at 12:46:57AM +0200, Laurent Pinchart wrote: > Store the list of converter output formats in the Configuration > structure, to be used to implement multi-stream support. As the > Configuration structure grows bigger, avoid duplicating it in the > formats_ map for each supported pixel format by storing it in a configs_ > vector instead, and storing pointers only in the map. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++---------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index c44fa9ee28ed..6a8253101a61 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -159,6 +159,7 @@ public: > uint32_t code; > PixelFormat captureFormat; > Size captureSize; > + std::vector<PixelFormat> outputFormats; > SizeRange outputSizes; > }; > > @@ -167,7 +168,8 @@ public: > std::list<Entity> entities_; > V4L2VideoDevice *video_; > > - std::map<PixelFormat, Configuration> formats_; > + std::vector<Configuration> configs_; > + std::map<PixelFormat, const Configuration *> formats_; > }; > > class SimpleCameraConfiguration : public CameraConfiguration > @@ -371,13 +373,6 @@ int SimpleCameraData::init() > }) > << " ]"; > > - /* > - * Store the configuration in the formats_ map, mapping the > - * PixelFormat to the corresponding configuration. Any > - * previously stored value is overwritten, as the pipeline > - * handler currently doesn't care about how a particular > - * PixelFormat is achieved. > - */ > for (const auto &videoFormat : videoFormats) { > PixelFormat pixelFormat = videoFormat.first.toPixelFormat(); > if (!pixelFormat) > @@ -389,23 +384,34 @@ int SimpleCameraData::init() > config.captureSize = format.size; > > if (!converter) { > + config.outputFormats = { pixelFormat }; > config.outputSizes = config.captureSize; > - formats_[pixelFormat] = config; > - continue; > + } else { > + config.outputFormats = converter->formats(pixelFormat); > + config.outputSizes = converter->sizes(format.size); > } > > - config.outputSizes = converter->sizes(format.size); > - > - for (PixelFormat fmt : converter->formats(pixelFormat)) > - formats_[fmt] = config; > + configs_.push_back(config); > } > } > > - if (formats_.empty()) { > + if (configs_.empty()) { > LOG(SimplePipeline, Error) << "No valid configuration found"; > return -EINVAL; > } > > + /* > + * Map the pixel formats to configurations. Any previously stored value > + * is overwritten, as the pipeline handler currently doesn't care about > + * how a particular PixelFormat is achieved. > + */ > + for (const Configuration &config : configs_) { > + formats_[config.captureFormat] = &config; > + > + for (PixelFormat fmt : config.outputFormats) > + formats_[fmt] = &config; > + } > + > properties_ = sensor_->properties(); > > return 0; > @@ -548,7 +554,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > status = Adjusted; > } > > - pipeConfig_ = &it->second; > + pipeConfig_ = it->second; I think this belongs in the previous patch. The rest looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > if (!pipeConfig_->outputSizes.contains(cfg.size)) { > LOG(SimplePipeline, Debug) > << "Adjusting size from " << cfg.size.toString() > @@ -614,7 +620,7 @@ CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera > std::inserter(formats, formats.end()), > [](const auto &format) -> decltype(formats)::value_type { > const PixelFormat &pixelFormat = format.first; > - const Size &size = format.second.captureSize; > + const Size &size = format.second->captureSize; > return { pixelFormat, { size } }; > });
Hi Paul, On Tue, Mar 02, 2021 at 02:45:26PM +0900, paul.elder@ideasonboard.com wrote: > On Mon, Feb 01, 2021 at 12:46:57AM +0200, Laurent Pinchart wrote: > > Store the list of converter output formats in the Configuration > > structure, to be used to implement multi-stream support. As the > > Configuration structure grows bigger, avoid duplicating it in the > > formats_ map for each supported pixel format by storing it in a configs_ > > vector instead, and storing pointers only in the map. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++---------- > > 1 file changed, 23 insertions(+), 17 deletions(-) > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index c44fa9ee28ed..6a8253101a61 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -159,6 +159,7 @@ public: > > uint32_t code; > > PixelFormat captureFormat; > > Size captureSize; > > + std::vector<PixelFormat> outputFormats; > > SizeRange outputSizes; > > }; > > > > @@ -167,7 +168,8 @@ public: > > std::list<Entity> entities_; > > V4L2VideoDevice *video_; > > > > - std::map<PixelFormat, Configuration> formats_; > > + std::vector<Configuration> configs_; > > + std::map<PixelFormat, const Configuration *> formats_; > > }; > > > > class SimpleCameraConfiguration : public CameraConfiguration > > @@ -371,13 +373,6 @@ int SimpleCameraData::init() > > }) > > << " ]"; > > > > - /* > > - * Store the configuration in the formats_ map, mapping the > > - * PixelFormat to the corresponding configuration. Any > > - * previously stored value is overwritten, as the pipeline > > - * handler currently doesn't care about how a particular > > - * PixelFormat is achieved. > > - */ > > for (const auto &videoFormat : videoFormats) { > > PixelFormat pixelFormat = videoFormat.first.toPixelFormat(); > > if (!pixelFormat) > > @@ -389,23 +384,34 @@ int SimpleCameraData::init() > > config.captureSize = format.size; > > > > if (!converter) { > > + config.outputFormats = { pixelFormat }; > > config.outputSizes = config.captureSize; > > - formats_[pixelFormat] = config; > > - continue; > > + } else { > > + config.outputFormats = converter->formats(pixelFormat); > > + config.outputSizes = converter->sizes(format.size); > > } > > > > - config.outputSizes = converter->sizes(format.size); > > - > > - for (PixelFormat fmt : converter->formats(pixelFormat)) > > - formats_[fmt] = config; > > + configs_.push_back(config); > > } > > } > > > > - if (formats_.empty()) { > > + if (configs_.empty()) { > > LOG(SimplePipeline, Error) << "No valid configuration found"; > > return -EINVAL; > > } > > > > + /* > > + * Map the pixel formats to configurations. Any previously stored value > > + * is overwritten, as the pipeline handler currently doesn't care about > > + * how a particular PixelFormat is achieved. > > + */ > > + for (const Configuration &config : configs_) { > > + formats_[config.captureFormat] = &config; > > + > > + for (PixelFormat fmt : config.outputFormats) > > + formats_[fmt] = &config; > > + } > > + > > properties_ = sensor_->properties(); > > > > return 0; > > @@ -548,7 +554,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > status = Adjusted; > > } > > > > - pipeConfig_ = &it->second; > > + pipeConfig_ = it->second; > > I think this belongs in the previous patch. I don't think so. it iterates over data_->formats_, whose type has changed in this patch. > The rest looks good to me. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > if (!pipeConfig_->outputSizes.contains(cfg.size)) { > > LOG(SimplePipeline, Debug) > > << "Adjusting size from " << cfg.size.toString() > > @@ -614,7 +620,7 @@ CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera > > std::inserter(formats, formats.end()), > > [](const auto &format) -> decltype(formats)::value_type { > > const PixelFormat &pixelFormat = format.first; > > - const Size &size = format.second.captureSize; > > + const Size &size = format.second->captureSize; > > return { pixelFormat, { size } }; > > }); >
On 31/01/2021 22:46, Laurent Pinchart wrote: > Store the list of converter output formats in the Configuration > structure, to be used to implement multi-stream support. As the > Configuration structure grows bigger, avoid duplicating it in the > formats_ map for each supported pixel format by storing it in a configs_ > vector instead, and storing pointers only in the map. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++---------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index c44fa9ee28ed..6a8253101a61 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -159,6 +159,7 @@ public: > uint32_t code; > PixelFormat captureFormat; > Size captureSize; > + std::vector<PixelFormat> outputFormats; > SizeRange outputSizes; > }; > > @@ -167,7 +168,8 @@ public: > std::list<Entity> entities_; > V4L2VideoDevice *video_; > > - std::map<PixelFormat, Configuration> formats_; > + std::vector<Configuration> configs_; > + std::map<PixelFormat, const Configuration *> formats_; Eeep, this scares me ... Are they kept in sync? Only used having pre-reserved, to ensure the pointers stay valid etc... > }; > > class SimpleCameraConfiguration : public CameraConfiguration > @@ -371,13 +373,6 @@ int SimpleCameraData::init() > }) > << " ]"; > > - /* > - * Store the configuration in the formats_ map, mapping the > - * PixelFormat to the corresponding configuration. Any > - * previously stored value is overwritten, as the pipeline > - * handler currently doesn't care about how a particular > - * PixelFormat is achieved. > - */ > for (const auto &videoFormat : videoFormats) { > PixelFormat pixelFormat = videoFormat.first.toPixelFormat(); > if (!pixelFormat) > @@ -389,23 +384,34 @@ int SimpleCameraData::init() > config.captureSize = format.size; > > if (!converter) { > + config.outputFormats = { pixelFormat }; > config.outputSizes = config.captureSize; > - formats_[pixelFormat] = config; > - continue; > + } else { > + config.outputFormats = converter->formats(pixelFormat); > + config.outputSizes = converter->sizes(format.size); > } > > - config.outputSizes = converter->sizes(format.size); > - > - for (PixelFormat fmt : converter->formats(pixelFormat)) > - formats_[fmt] = config; > + configs_.push_back(config); > } > } > > - if (formats_.empty()) { > + if (configs_.empty()) { > LOG(SimplePipeline, Error) << "No valid configuration found"; > return -EINVAL; > } > > + /* > + * Map the pixel formats to configurations. Any previously stored value > + * is overwritten, as the pipeline handler currently doesn't care about > + * how a particular PixelFormat is achieved. > + */ > + for (const Configuration &config : configs_) { > + formats_[config.captureFormat] = &config; > + > + for (PixelFormat fmt : config.outputFormats) > + formats_[fmt] = &config; > + } Phew, ok, so configs_ never grows or changes after init, and the formats_ is generated after the configs_ are prepared. So I think we're good. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > properties_ = sensor_->properties(); > > return 0; > @@ -548,7 +554,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > status = Adjusted; > } > > - pipeConfig_ = &it->second; > + pipeConfig_ = it->second; > if (!pipeConfig_->outputSizes.contains(cfg.size)) { > LOG(SimplePipeline, Debug) > << "Adjusting size from " << cfg.size.toString() > @@ -614,7 +620,7 @@ CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera > std::inserter(formats, formats.end()), > [](const auto &format) -> decltype(formats)::value_type { > const PixelFormat &pixelFormat = format.first; > - const Size &size = format.second.captureSize; > + const Size &size = format.second->captureSize; > return { pixelFormat, { size } }; > }); > >
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index c44fa9ee28ed..6a8253101a61 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -159,6 +159,7 @@ public: uint32_t code; PixelFormat captureFormat; Size captureSize; + std::vector<PixelFormat> outputFormats; SizeRange outputSizes; }; @@ -167,7 +168,8 @@ public: std::list<Entity> entities_; V4L2VideoDevice *video_; - std::map<PixelFormat, Configuration> formats_; + std::vector<Configuration> configs_; + std::map<PixelFormat, const Configuration *> formats_; }; class SimpleCameraConfiguration : public CameraConfiguration @@ -371,13 +373,6 @@ int SimpleCameraData::init() }) << " ]"; - /* - * Store the configuration in the formats_ map, mapping the - * PixelFormat to the corresponding configuration. Any - * previously stored value is overwritten, as the pipeline - * handler currently doesn't care about how a particular - * PixelFormat is achieved. - */ for (const auto &videoFormat : videoFormats) { PixelFormat pixelFormat = videoFormat.first.toPixelFormat(); if (!pixelFormat) @@ -389,23 +384,34 @@ int SimpleCameraData::init() config.captureSize = format.size; if (!converter) { + config.outputFormats = { pixelFormat }; config.outputSizes = config.captureSize; - formats_[pixelFormat] = config; - continue; + } else { + config.outputFormats = converter->formats(pixelFormat); + config.outputSizes = converter->sizes(format.size); } - config.outputSizes = converter->sizes(format.size); - - for (PixelFormat fmt : converter->formats(pixelFormat)) - formats_[fmt] = config; + configs_.push_back(config); } } - if (formats_.empty()) { + if (configs_.empty()) { LOG(SimplePipeline, Error) << "No valid configuration found"; return -EINVAL; } + /* + * Map the pixel formats to configurations. Any previously stored value + * is overwritten, as the pipeline handler currently doesn't care about + * how a particular PixelFormat is achieved. + */ + for (const Configuration &config : configs_) { + formats_[config.captureFormat] = &config; + + for (PixelFormat fmt : config.outputFormats) + formats_[fmt] = &config; + } + properties_ = sensor_->properties(); return 0; @@ -548,7 +554,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() status = Adjusted; } - pipeConfig_ = &it->second; + pipeConfig_ = it->second; if (!pipeConfig_->outputSizes.contains(cfg.size)) { LOG(SimplePipeline, Debug) << "Adjusting size from " << cfg.size.toString() @@ -614,7 +620,7 @@ CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera std::inserter(formats, formats.end()), [](const auto &format) -> decltype(formats)::value_type { const PixelFormat &pixelFormat = format.first; - const Size &size = format.second.captureSize; + const Size &size = format.second->captureSize; return { pixelFormat, { size } }; });
Store the list of converter output formats in the Configuration structure, to be used to implement multi-stream support. As the Configuration structure grows bigger, avoid duplicating it in the formats_ map for each supported pixel format by storing it in a configs_ vector instead, and storing pointers only in the map. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++---------- 1 file changed, 23 insertions(+), 17 deletions(-)