[libcamera-devel] [PATCH] android; camera_device: Reset config_ if Camera::configure() fails

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Tue, Jun 15, 2021 at 9:21 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> The config_ pointer is reset in all error paths of the
> CameraDevice::configureStreams() function, except when
> Camera::configure() fails. Fix it by using a local unique pointer to
> store the configuration until the end of the function, to avoid similar
> issues in the future.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f3fd38690e63..f0b5f87bc38f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1716,8 +1716,8 @@ int
> CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>          * Generate an empty configuration, and construct a
> StreamConfiguration
>          * for each camera3_stream to add to it.
>          */
> -       config_ = camera_->generateConfiguration();
> -       if (!config_) {
> +       std::unique_ptr<CameraConfiguration> config =
> camera_->generateConfiguration();
> +       if (!config) {
>                 LOG(HAL, Error) << "Failed to generate camera
> configuration";
>                 return -EINVAL;
>         }
> @@ -1855,29 +1855,27 @@ int
> CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>
>         sortCamera3StreamConfigs(streamConfigs, jpegStream);
>         for (const auto &streamConfig : streamConfigs) {
> -               config_->addConfiguration(streamConfig.config);
> +               config->addConfiguration(streamConfig.config);
>
>                 for (auto &stream : streamConfig.streams) {
>                         streams_.emplace_back(this, stream.type,
> stream.stream,
> -                                             config_->size() - 1);
> +                                             config->size() - 1);

                        stream.stream->priv = static_cast<void
> *>(&streams_.back());

                }
>         }
>
> -       switch (config_->validate()) {
> +       switch (config->validate()) {
>         case CameraConfiguration::Valid:
>                 break;
>         case CameraConfiguration::Adjusted:
>                 LOG(HAL, Info) << "Camera configuration adjusted";
>
> -               for (const StreamConfiguration &cfg : *config_)
> +               for (const StreamConfiguration &cfg : *config)
>                         LOG(HAL, Info) << " - " << cfg.toString();
>
> -               config_.reset();
>                 return -EINVAL;
>         case CameraConfiguration::Invalid:
>                 LOG(HAL, Info) << "Camera configuration invalid";
> -               config_.reset();
>                 return -EINVAL;
>         }
>
> @@ -1885,7 +1883,7 @@ int
> CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>          * Once the CameraConfiguration has been adjusted/validated
>          * it can be applied to the camera.
>          */
> -       int ret = camera_->configure(config_.get());
> +       int ret = camera_->configure(config.get());
>         if (ret) {
>                 LOG(HAL, Error) << "Failed to configure camera '"
>                                 << camera_->id() << "'";
> @@ -1905,6 +1903,7 @@ int
> CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>                 }
>         }
>
> +       config_ = std::move(config);
>         return 0;
>  }
>
>
Shall we also clear streams_ on failure?

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>


> --
> Regards,
>
> Laurent Pinchart
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210615/b0432b9a/attachment.htm>