| Message ID | 20210615002047.25393-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 7532caa2c77b966afde643d5550dff6e404e7208 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thank you for the patch. On Tue, Jun 15, 2021 at 9:21 AM Laurent Pinchart < laurent.pinchart@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@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@chromium.org> > -- > Regards, > > Laurent Pinchart > >
Hi Laurent, On 6/15/21 5:50 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@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@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; > } >
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@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@chromium.org>
Hi Laurent, On Mon, Jun 28, 2021 at 1:01 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@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. -Hiro > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > -- > Regards, > > 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@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@chromium.org>
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; }
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@ideasonboard.com> --- src/android/camera_device.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)