| Message ID | 20210906140152.636883-1-jacopo@jmondi.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, thank you for the patch. On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > The CameraDevice::streams_ class member is populated at > configureStreams() time with one item per each stream requested to the > Camera device. > > When a new configuration is applied to the Camera HAL the list of > requested streams has to be re-initialized and the streams_ vector needs > to be manually cleared in order to be populated from scratch. > > As configureStreams() class CameraDevice::stop() at the very beginning, > centralize clearing the streams_ vector there even in the case the > camera has been stopped by a previous call to flush(). > > Suggested-by: Hirokazu Honda <hiroh@chromium.org> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index fda77db4540c..30c173a69720 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -448,8 +448,18 @@ void CameraDevice::flush() > void CameraDevice::stop() > { > MutexLocker stateLock(stateMutex_); > - if (state_ == State::Stopped) > + if (state_ == State::Stopped) { > + /* > + * We get here if the list of streams requested by the camera > + * service has to be updated but the camera has been stopped > + * already, which happens if configureStreams() gets called > + * after a flush(). Clear the list of stream configurations, > + * no need to clear descriptors as stopping the camera completes > + * all the pending requests. > + */ > + streams_.clear(); > return; > + } > > worker_.stop(); > camera_->stop(); > @@ -544,6 +554,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > LOG(HAL, Error) << "No streams in configuration"; > return -EINVAL; > } > + streams_.reserve(stream_list->num_streams); > > #if defined(OS_CHROMEOS) > if (!validateCropRotate(*stream_list)) > @@ -560,14 +571,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > return -EINVAL; > } > > - /* > - * Clear and remove any existing configuration from previous calls, and > - * ensure the required entries are available without further > - * reallocation. > - */ > - streams_.clear(); > - streams_.reserve(stream_list->num_streams); > - > std::vector<Camera3StreamConfig> streamConfigs; > streamConfigs.reserve(stream_list->num_streams); > > -- > 2.32.0 >
Hi Jacopo, thank you for the patch. On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > The CameraDevice::abortRequest() function should operate on const > pointers. > > Fix that. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 2 +- > src/android/camera_device.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 30c173a69720..a0ea138d9499 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -845,7 +845,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > return 0; > } > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > +void CameraDevice::abortRequest(const camera3_capture_request_t *request) > { > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); Is it possible to make this const reference? -Hiro > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index a55769272651..54c4cb9ab499 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -96,7 +96,7 @@ private: > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer, > libcamera::PixelFormat pixelFormat, > const libcamera::Size &size); > - void abortRequest(camera3_capture_request_t *request); > + void abortRequest(const camera3_capture_request_t *request); > bool isValidRequest(camera3_capture_request_t *request) const; > void notifyShutter(uint32_t frameNumber, uint64_t timestamp); > void notifyError(uint32_t frameNumber, camera3_stream_t *stream, > -- > 2.32.0 >
Hi Jacopo, thank you for the patch. On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > The CameraDevice::descriptors_ class member holds one item per each > in-flight request. A new descriptor is added to the map each time a new > request is queued to the camera and it gets then popped out when the > request completes or when an error happens before queueing it. > > As stopping the camera guarantees that all in-flight requests are > completed synchronously to the Camera::stop() call, there is no need to > manually clear the descriptors_ map, on the contrary, it might hides > issues in the handling of in-flight requests. > > Remove it and replace it with an assertion to make sure the underlying > camera behaves as intended and all Request are completed when the camera > is stopped. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 0ce9b699bfae..1591ad98c7d5 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -442,6 +442,9 @@ void CameraDevice::flush() > worker_.stop(); > camera_->stop(); > > + /* Make sure all in-flight requests have been completed. */ Shall we say descriptors are flushed in worker.stop()? Reviewed-by: Hirokazu Honda <hiroh@chromium.org> -Hiro > + ASSERT(descriptors_.empty()); > + > MutexLocker stateLock(stateMutex_); > state_ = State::Stopped; > } > @@ -454,9 +457,7 @@ void CameraDevice::stop() > * We get here if the list of streams requested by the camera > * service has to be updated but the camera has been stopped > * already, which happens if configureStreams() gets called > - * after a flush(). Clear the list of stream configurations, > - * no need to clear descriptors as stopping the camera completes > - * all the pending requests. > + * after a flush(). > */ > streams_.clear(); > return; > @@ -465,7 +466,9 @@ void CameraDevice::stop() > worker_.stop(); > camera_->stop(); > > - descriptors_.clear(); > + /* Make sure all in-flight requests have been completed. */ > + ASSERT(descriptors_.empty()); > + > streams_.clear(); > > state_ = State::Stopped; > -- > 2.32.0 >
Hi Hiro, On Mon, Sep 06, 2021 at 11:27:58PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for the patch. > > On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > The CameraDevice::abortRequest() function should operate on const > > pointers. > > > > Fix that. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 2 +- > > src/android/camera_device.h | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 30c173a69720..a0ea138d9499 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -845,7 +845,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > > return 0; > > } > > > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > > +void CameraDevice::abortRequest(const camera3_capture_request_t *request) > > { > > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > Is it possible to make this const reference? All the HAL entry points receive parameters by pointer from the framework. If we want to use references we would have to dereference them in the HAL code when using them as parameter. I don't think we would gain much to be honest. Thanks j > > -Hiro > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index a55769272651..54c4cb9ab499 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -96,7 +96,7 @@ private: > > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer, > > libcamera::PixelFormat pixelFormat, > > const libcamera::Size &size); > > - void abortRequest(camera3_capture_request_t *request); > > + void abortRequest(const camera3_capture_request_t *request); > > bool isValidRequest(camera3_capture_request_t *request) const; > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp); > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream, > > -- > > 2.32.0 > >
Hi Hiro On Mon, Sep 06, 2021 at 11:56:50PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for the patch. > > On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > The CameraDevice::descriptors_ class member holds one item per each > > in-flight request. A new descriptor is added to the map each time a new > > request is queued to the camera and it gets then popped out when the > > request completes or when an error happens before queueing it. > > > > As stopping the camera guarantees that all in-flight requests are > > completed synchronously to the Camera::stop() call, there is no need to > > manually clear the descriptors_ map, on the contrary, it might hides > > issues in the handling of in-flight requests. > > > > Remove it and replace it with an assertion to make sure the underlying > > camera behaves as intended and all Request are completed when the camera > > is stopped. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 0ce9b699bfae..1591ad98c7d5 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -442,6 +442,9 @@ void CameraDevice::flush() > > worker_.stop(); > > camera_->stop(); > > > > + /* Make sure all in-flight requests have been completed. */ > > Shall we say descriptors are flushed in worker.stop()? descriptors, in my understanding, as removed from the descriptors_ map one by one as requests get completed, as a consequence of the regular flow of operations or because of a force completion caused by Camera::stop(). Have I missed your comment ? Thanks j > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > -Hiro > > + ASSERT(descriptors_.empty()); > > + > > MutexLocker stateLock(stateMutex_); > > state_ = State::Stopped; > > } > > @@ -454,9 +457,7 @@ void CameraDevice::stop() > > * We get here if the list of streams requested by the camera > > * service has to be updated but the camera has been stopped > > * already, which happens if configureStreams() gets called > > - * after a flush(). Clear the list of stream configurations, > > - * no need to clear descriptors as stopping the camera completes > > - * all the pending requests. > > + * after a flush(). > > */ > > streams_.clear(); > > return; > > @@ -465,7 +466,9 @@ void CameraDevice::stop() > > worker_.stop(); > > camera_->stop(); > > > > - descriptors_.clear(); > > + /* Make sure all in-flight requests have been completed. */ > > + ASSERT(descriptors_.empty()); > > + > > streams_.clear(); > > > > state_ = State::Stopped; > > -- > > 2.32.0 > >
Hi Jacopo, On 9/6/21 7:31 PM, Jacopo Mondi wrote: > The CameraDevice::streams_ class member is populated at > configureStreams() time with one item per each stream requested to the > Camera device. > > When a new configuration is applied to the Camera HAL the list of > requested streams has to be re-initialized and the streams_ vector needs > to be manually cleared in order to be populated from scratch. > > As configureStreams() class CameraDevice::stop() at the very beginning, > centralize clearing the streams_ vector there even in the case the > camera has been stopped by a previous call to flush(). > > Suggested-by: Hirokazu Honda <hiroh@chromium.org> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index fda77db4540c..30c173a69720 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -448,8 +448,18 @@ void CameraDevice::flush() > void CameraDevice::stop() > { > MutexLocker stateLock(stateMutex_); > - if (state_ == State::Stopped) > + if (state_ == State::Stopped) { > + /* > + * We get here if the list of streams requested by the camera > + * service has to be updated but the camera has been stopped > + * already, which happens if configureStreams() gets called > + * after a flush(). Clear the list of stream configurations, > + * no need to clear descriptors as stopping the camera completes > + * all the pending requests. > + */ > + streams_.clear(); > return; > + } This makes sense to me, but I facing a more fundamental question that why does flush() tries to stop the libcamera::Camera instance on it's own and setting state_ = State::Stopped; manually. Ideally it should happen by calling CameraDevice::stop() inside flush(). Seems I am missing something.. need to look at flush() documentation in depth. As far as I have looked the current configureStreams() behaviour, the change and reasons makes sense to me, so Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > worker_.stop(); > camera_->stop(); > @@ -544,6 +554,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > LOG(HAL, Error) << "No streams in configuration"; > return -EINVAL; > } > + streams_.reserve(stream_list->num_streams); > > #if defined(OS_CHROMEOS) > if (!validateCropRotate(*stream_list)) > @@ -560,14 +571,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > return -EINVAL; > } > > - /* > - * Clear and remove any existing configuration from previous calls, and > - * ensure the required entries are available without further > - * reallocation. > - */ > - streams_.clear(); > - streams_.reserve(stream_list->num_streams); > - > std::vector<Camera3StreamConfig> streamConfigs; > streamConfigs.reserve(stream_list->num_streams); >
Hi Jacopo, On 9/6/21 7:31 PM, Jacopo Mondi wrote: > The CameraDevice::abortRequest() function should operate on const > pointers. > > Fix that. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 2 +- > src/android/camera_device.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 30c173a69720..a0ea138d9499 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -845,7 +845,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > return 0; > } > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > +void CameraDevice::abortRequest(const camera3_capture_request_t *request) > { > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index a55769272651..54c4cb9ab499 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -96,7 +96,7 @@ private: > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer, > libcamera::PixelFormat pixelFormat, > const libcamera::Size &size); > - void abortRequest(camera3_capture_request_t *request); > + void abortRequest(const camera3_capture_request_t *request); > bool isValidRequest(camera3_capture_request_t *request) const; > void notifyShutter(uint32_t frameNumber, uint64_t timestamp); > void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
Hi Jacopo, On 9/6/21 7:31 PM, Jacopo Mondi wrote: > The CameraDevice::descriptors_ class member holds one item per each > in-flight request. A new descriptor is added to the map each time a new > request is queued to the camera and it gets then popped out when the > request completes or when an error happens before queueing it. > > As stopping the camera guarantees that all in-flight requests are > completed synchronously to the Camera::stop() call, there is no need to > manually clear the descriptors_ map, on the contrary, it might hides > issues in the handling of in-flight requests. > > Remove it and replace it with an assertion to make sure the underlying > camera behaves as intended and all Request are completed when the camera > is stopped. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 0ce9b699bfae..1591ad98c7d5 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -442,6 +442,9 @@ void CameraDevice::flush() > worker_.stop(); > camera_->stop(); > > + /* Make sure all in-flight requests have been completed. */ > + ASSERT(descriptors_.empty()); > + > MutexLocker stateLock(stateMutex_); > state_ = State::Stopped; > } > @@ -454,9 +457,7 @@ void CameraDevice::stop() > * We get here if the list of streams requested by the camera > * service has to be updated but the camera has been stopped > * already, which happens if configureStreams() gets called > - * after a flush(). Clear the list of stream configurations, > - * no need to clear descriptors as stopping the camera completes > - * all the pending requests. > + * after a flush(). > */ > streams_.clear(); > return; > @@ -465,7 +466,9 @@ void CameraDevice::stop() > worker_.stop(); > camera_->stop(); > > - descriptors_.clear(); > + /* Make sure all in-flight requests have been completed. */ > + ASSERT(descriptors_.empty()); > + Well, the descriptors_ mapped might seem empty but some descriptors can be queued somewhere else for async stuff in future ;-) anyways looks good to me as per master so, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > streams_.clear(); > > state_ = State::Stopped;
Hi Jacopo, On Fri, Sep 10, 2021 at 9:34 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi Jacopo, > > On 9/6/21 7:31 PM, Jacopo Mondi wrote: > > The CameraDevice::abortRequest() function should operate on const > > pointers. > > > > Fix that. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/android/camera_device.cpp | 2 +- > > src/android/camera_device.h | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 30c173a69720..a0ea138d9499 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -845,7 +845,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > > return 0; > > } > > > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > > +void CameraDevice::abortRequest(const camera3_capture_request_t *request) > > { > > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index a55769272651..54c4cb9ab499 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -96,7 +96,7 @@ private: > > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer, > > libcamera::PixelFormat pixelFormat, > > const libcamera::Size &size); > > - void abortRequest(camera3_capture_request_t *request); > > + void abortRequest(const camera3_capture_request_t *request); > > bool isValidRequest(camera3_capture_request_t *request) const; > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp); > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
Hello, On Fri, Sep 10, 2021 at 06:04:15PM +0530, Umang Jain wrote: > On 9/6/21 7:31 PM, Jacopo Mondi wrote: > > The CameraDevice::streams_ class member is populated at > > configureStreams() time with one item per each stream requested to the > > Camera device. > > > > When a new configuration is applied to the Camera HAL the list of > > requested streams has to be re-initialized and the streams_ vector needs > > to be manually cleared in order to be populated from scratch. > > > > As configureStreams() class CameraDevice::stop() at the very beginning, s/class/calls/ > > centralize clearing the streams_ vector there even in the case the > > camera has been stopped by a previous call to flush(). > > > > Suggested-by: Hirokazu Honda <hiroh@chromium.org> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index fda77db4540c..30c173a69720 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -448,8 +448,18 @@ void CameraDevice::flush() > > void CameraDevice::stop() > > { > > MutexLocker stateLock(stateMutex_); > > - if (state_ == State::Stopped) > > + if (state_ == State::Stopped) { > > + /* > > + * We get here if the list of streams requested by the camera > > + * service has to be updated but the camera has been stopped > > + * already, which happens if configureStreams() gets called > > + * after a flush(). Clear the list of stream configurations, > > + * no need to clear descriptors as stopping the camera completes > > + * all the pending requests. > > + */ > > + streams_.clear(); stop() is called in configureStreams() and in close(). close() already has a streams_.clear() call, which can't be removed (yet ?) as there's no guarantee close() will be called with state_ == State::Stopped. I'm thus not sure what this patch brings, given that the streams_.clear() call here is not "centralized", it's only meant for the flush() case. Maybe I'll have a better understanding when reading the next patches. > > return; > > + } > > This makes sense to me, but I facing a more fundamental question that > why does flush() tries to stop the libcamera::Camera instance on it's > own and setting state_ = State::Stopped; manually. Ideally it should > happen by calling CameraDevice::stop() inside flush(). Seems I am > missing something.. need to look at flush() documentation in depth. > > As far as I have looked the current configureStreams() behaviour, the > change and reasons makes sense to me, so > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > worker_.stop(); > > camera_->stop(); > > @@ -544,6 +554,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > LOG(HAL, Error) << "No streams in configuration"; > > return -EINVAL; > > } Blank line. > > + streams_.reserve(stream_list->num_streams); > > > > #if defined(OS_CHROMEOS) > > if (!validateCropRotate(*stream_list)) > > @@ -560,14 +571,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > return -EINVAL; > > } > > > > - /* > > - * Clear and remove any existing configuration from previous calls, and > > - * ensure the required entries are available without further > > - * reallocation. > > - */ > > - streams_.clear(); > > - streams_.reserve(stream_list->num_streams); > > - > > std::vector<Camera3StreamConfig> streamConfigs; > > streamConfigs.reserve(stream_list->num_streams); > >
Hi Jacopo, Thank you for the patch. On Mon, Sep 06, 2021 at 04:01:50PM +0200, Jacopo Mondi wrote: > The CameraDevice::abortRequest() function should operate on const > pointers. > > Fix that. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_device.cpp | 2 +- > src/android/camera_device.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 30c173a69720..a0ea138d9499 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -845,7 +845,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > return 0; > } > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > +void CameraDevice::abortRequest(const camera3_capture_request_t *request) > { > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index a55769272651..54c4cb9ab499 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -96,7 +96,7 @@ private: > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer, > libcamera::PixelFormat pixelFormat, > const libcamera::Size &size); > - void abortRequest(camera3_capture_request_t *request); > + void abortRequest(const camera3_capture_request_t *request); > bool isValidRequest(camera3_capture_request_t *request) const; > void notifyShutter(uint32_t frameNumber, uint64_t timestamp); > void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
Hi Jacopo, can you merge this? On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > While testing Hiro's patch, which I'm re-sending here, I tried as well to > centralize the streams_ and descriptors_ cleanup. > > This lead to the discovery of a potentially missed frame error in CameraWorker. > > On top of that, I have added two assertions to enforce the correct Camera > behavior after a stop() call. > > Thanks > j > > Hirokazu Honda (1): > android: camera_device: Fix crash in calling CameraDevice::close() > > Jacopo Mondi (4): > android: camera_device: Clear streams_ in stop() > android: camera_device: Make abortRequest() take a const > android: camera_worker: Notify fence wait failures > android: camera_device: Assert descriptors_ is empty > > src/android/camera_device.cpp | 54 ++++++++++++++++++++++++++--------- > src/android/camera_device.h | 3 +- > src/android/camera_worker.cpp | 10 ++++--- > src/android/camera_worker.h | 9 ++++-- > 4 files changed, 55 insertions(+), 21 deletions(-) > > -- > 2.32.0 >