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

[Laurent Pinchart]

Hi Hiro,

On Mon, Jun 28, 2021 at 01:22:46PM +0900, Hirokazu Honda wrote:
> On Mon, Jun 28, 2021 at 1:01 PM Laurent Pinchart wrote:
> > 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.
> 
> No, if configureStreams() is successful, streams_ will be used in
> other places (e.g. processCaptureRequest()) as
> camera3_stream_buffer_t.stream->priv.
> We couldn't make it a local variable.

Of course. My bad, sorry.

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

-- 
Regards,

Laurent Pinchart