[libcamera-devel] [PATCH v3 2/3] android: camera_device: Use Camera3StreamConfig in configureStreams()
Jacopo Mondi
jacopo at jmondi.org
Mon Dec 7 11:24:45 CET 2020
Hi Hiro,
On Mon, Dec 07, 2020 at 08:42:17AM +0000, Hirokazu Honda wrote:
> This uses Camera3StreamConfig in CameraDevice::configureStreams().
I would be slightly more verbose.
"Use the newly introduced Camera3StreamConfig to associate the
Android requested streams with the associated StreamConfiguration
in a vector of configurations.
This change prepares to sort the vector of configuration before using
it to configure the Camera and populate the streams_ vector.
No functional changes intended."
And the signed-off-by of course.
> ---
> src/android/camera_device.cpp | 52 +++++++++++++++++++++--------------
> 1 file changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 09269d95..b7bf3d88 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1240,6 +1240,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> streams_.clear();
> streams_.reserve(stream_list->num_streams);
>
> + std::vector<Camera3StreamConfig> streamConfigs;
> + streamConfigs.reserve(stream_list->num_streams);
> +
> /* First handle all non-MJPEG streams. */
> camera3_stream_t *jpegStream = nullptr;
> for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> @@ -1270,14 +1273,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> continue;
> }
>
> - StreamConfiguration streamConfiguration;
> - streamConfiguration.size = size;
> - streamConfiguration.pixelFormat = format;
> -
> - config_->addConfiguration(streamConfiguration);
> - streams_.emplace_back(this, CameraStream::Type::Direct,
> - stream, config_->size() - 1);
> - stream->priv = static_cast<void *>(&streams_.back());
> + Camera3StreamConfig streamConfig;
> + streamConfig.streams = { stream };
> + streamConfig.types = { CameraStream::Type::Direct };
> + streamConfig.config.size = size;
> + streamConfig.config.pixelFormat = format;
> + streamConfigs.push_back(std::move(streamConfig));
Nit: does this move help in any way with memory handling ? I mean,
I do expect the auto-generated move constructor of Camera3StreamConfig
not being able to magically move bits between the local variable and
the vector storage ? Am I wrong or what move() actually does is to
invalidate the local variable but copies data anyway in this case ?
We're talking about a tiny number of operations not in an hot-path, so
this i very minor, but I wonder if emplace() wouldn't do better (or
just a push_back() it it's equivalent to move()).
> }
>
> /* Now handle the MJPEG streams, adding a new stream if required. */
> @@ -1286,9 +1287,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> int index = -1;
>
> /* Search for a compatible stream in the non-JPEG ones. */
> - for (unsigned int i = 0; i < config_->size(); i++) {
> - StreamConfiguration &cfg = config_->at(i);
> -
> + for (size_t i = 0; i < streamConfigs.size(); ++i) {
> + const auto &cfg = streamConfigs[i].config;
> /*
> * \todo The PixelFormat must also be compatible with
> * the encoder.
> @@ -1310,28 +1310,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> * introduce a new stream to satisfy the request requirements.
> */
> if (index < 0) {
> - StreamConfiguration streamConfiguration;
> -
> /*
> * \todo The pixelFormat should be a 'best-fit' choice
> * and may require a validation cycle. This is not yet
> * handled, and should be considered as part of any
> * stream configuration reworks.
> */
> - streamConfiguration.size.width = jpegStream->width;
> - streamConfiguration.size.height = jpegStream->height;
> - streamConfiguration.pixelFormat = formats::NV12;
> + Camera3StreamConfig streamConfig;
> + streamConfig.config.size.width = jpegStream->width;
> + streamConfig.config.size.height = jpegStream->height;
> + streamConfig.config.pixelFormat = formats::NV12;
> + streamConfigs.push_back(std::move(streamConfig));
>
> - LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> + LOG(HAL, Info) << "Adding " << streamConfig.config.toString()
> << " for MJPEG support";
>
> type = CameraStream::Type::Internal;
> - config_->addConfiguration(streamConfiguration);
> - index = config_->size() - 1;
> + index = streamConfigs.size() - 1;
> }
>
> - streams_.emplace_back(this, type, jpegStream, index);
> - jpegStream->priv = static_cast<void *>(&streams_.back());
> + streamConfigs[index].streams.push_back(jpegStream);
> + streamConfigs[index].types.push_back(type);
> + }
> +
> + for (const auto &streamConfig : streamConfigs) {
> + config_->addConfiguration(streamConfig.config);
> + for (size_t i = 0; i < streamConfig.streams.size(); ++i) {
> + camera3_stream_t *stream = streamConfig.streams[i];
> + const CameraStream::Type type = streamConfig.types[i];
> + streams_.emplace_back(this, type,
> + stream, config_->size() - 1);
> + stream->priv = static_cast<void*>(&streams_.back());
> + }
I like where this is going!
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> }
>
> switch (config_->validate()) {
> --
> 2.29.2.576.ga3fc446d84-goog
More information about the libcamera-devel
mailing list