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

[Laurent Pinchart]

Hi Hiro,

On Tue, Jun 15, 2021 at 03:10:01PM +0900, Hirokazu Honda wrote:
> On Tue, Jun 15, 2021 at 9:21 AM Laurent Pinchart 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?

Possibly, but we clear it at the beginning of
CameraDevice::configureStreams(), so it should be fine.

Looking at it, it seems that streams_ is only used in this function.
Should it be turned into a local variable ? That's a candidate for a
separate patch.

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

-- 
Regards,

Laurent Pinchart