| Message ID | 20210423040738.1227220-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Fri, Apr 23, 2021 at 01:07:38PM +0900, Hirokazu Honda wrote: > This implements flush(). It is mostly the same as close() > though the canceled events are reported with error messages. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------ > src/android/camera_device.h | 1 + > src/android/camera_ops.cpp | 6 ++++- > 3 files changed, 40 insertions(+), 8 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index c74dc0e7..22a2e13c 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -753,6 +753,15 @@ void CameraDevice::close() > stop(/*releaseCamera=*/true); > } > > +int CameraDevice::flush() > +{ > + std::scoped_lock<std::mutex> lock(mutex_); > + > + stop(); > + > + return 0; > +} > + > void CameraDevice::stop(bool releaseCamera) > { > streams_.clear(); > @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera) > worker_.stop(); > camera_->stop(); > > + for (auto &[cookie, descriptor] : descriptors_) { > + LOG(HAL, Debug) << "request is canceled: " << cookie; LOG(HAL, Debug) << "request " << cookie << " is canceled"; > + > + camera3_capture_result_t captureResult = {}; > + captureResult.frame_number = descriptor.frameNumber_; > + captureResult.num_output_buffers = descriptor.buffers_.size(); > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > + buffer.acquire_fence = -1; > + buffer.release_fence = -1; > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > + } > + captureResult.output_buffers = descriptor.buffers_.data(); > + > + notifyError(descriptor.frameNumber_, > + descriptor.buffers_[0].stream); > + callbacks_->process_capture_result(callbacks_, &captureResult); This looks very similar to the code in CameraDevice::requestComplete(), would it make sense to move it to a separate function (including the call to notifyError) ? > + } > descriptors_.clear(); > > running_ = false; > @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request) > } > > if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) { > + /* > + * \todo Report and identify the stream number or configuration > + * to clarify the stream that failed. > + */ > + LOG(HAL, Error) > + << "Error occurred on frame " << descriptor.frameNumber_ << " (" > + << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")"; > + > /* \todo Improve error handling. In case we notify an error > * because the metadata generation fails, a shutter event has > * already been notified for this frame number before the error > @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) > { > camera3_notify_msg_t notify = {}; > > - /* > - * \todo Report and identify the stream number or configuration to > - * clarify the stream that failed. > - */ > - LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " (" > - << toPixelFormat(stream->format).toString() << ")"; > - > notify.type = CAMERA3_MSG_ERROR; > notify.message.error.error_stream = stream; > notify.message.error.frame_number = frameNumber; > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 08553584..7004c89a 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -41,6 +41,7 @@ public: > > int open(const hw_module_t *hardwareModule); > void close(); > + int flush(); > > unsigned int id() const { return id_; } > camera3_device_t *camera3Device() { return &camera3Device_; } > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp > index 696e8043..981a3d30 100644 > --- a/src/android/camera_ops.cpp > +++ b/src/android/camera_ops.cpp > @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, > > static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) You can drop [[maybe_unused]]. It would be nice to have an [[unused]] that causes a warning when the variable is used. > { > - return 0; > + if (!dev) > + return -EINVAL; > + > + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv); > + return camera->flush(); > } > > int hal_dev_close(hw_device_t *hw_device)
Hi Laurent, On Mon, Apr 26, 2021 at 9:37 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Fri, Apr 23, 2021 at 01:07:38PM +0900, Hirokazu Honda wrote: > > This implements flush(). It is mostly the same as close() > > though the canceled events are reported with error messages. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------ > > src/android/camera_device.h | 1 + > > src/android/camera_ops.cpp | 6 ++++- > > 3 files changed, 40 insertions(+), 8 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index c74dc0e7..22a2e13c 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -753,6 +753,15 @@ void CameraDevice::close() > > stop(/*releaseCamera=*/true); > > } > > > > +int CameraDevice::flush() > > +{ > > + std::scoped_lock<std::mutex> lock(mutex_); > > + > > + stop(); > > + > > + return 0; > > +} > > + > > void CameraDevice::stop(bool releaseCamera) > > { > > streams_.clear(); > > @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera) > > worker_.stop(); > > camera_->stop(); > > > > + for (auto &[cookie, descriptor] : descriptors_) { > > + LOG(HAL, Debug) << "request is canceled: " << cookie; > > LOG(HAL, Debug) << "request " << cookie << " is canceled"; > > > + > > + camera3_capture_result_t captureResult = {}; > > + captureResult.frame_number = descriptor.frameNumber_; > > + captureResult.num_output_buffers = descriptor.buffers_.size(); > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > > + buffer.acquire_fence = -1; > > + buffer.release_fence = -1; > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > + } > > + captureResult.output_buffers = descriptor.buffers_.data(); > > + > > + notifyError(descriptor.frameNumber_, > > + descriptor.buffers_[0].stream); > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > This looks very similar to the code in CameraDevice::requestComplete(), > would it make sense to move it to a separate function (including the > call to notifyError) ? > Sure, I will factorize in the next patch. My first thought is to reduce the number of patches in the series and factorize them after the patches are merged to not complicate this patch series any more. > > + } > > descriptors_.clear(); > > > > running_ = false; > > @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request) > > } > > > > if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) { > > + /* > > + * \todo Report and identify the stream number or configuration > > + * to clarify the stream that failed. > > + */ > > + LOG(HAL, Error) > > + << "Error occurred on frame " << descriptor.frameNumber_ << " (" > > + << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")"; > > + > > /* \todo Improve error handling. In case we notify an error > > * because the metadata generation fails, a shutter event has > > * already been notified for this frame number before the error > > @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) > > { > > camera3_notify_msg_t notify = {}; > > > > - /* > > - * \todo Report and identify the stream number or configuration to > > - * clarify the stream that failed. > > - */ > > - LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " (" > > - << toPixelFormat(stream->format).toString() << ")"; > > - > > notify.type = CAMERA3_MSG_ERROR; > > notify.message.error.error_stream = stream; > > notify.message.error.frame_number = frameNumber; > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 08553584..7004c89a 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -41,6 +41,7 @@ public: > > > > int open(const hw_module_t *hardwareModule); > > void close(); > > + int flush(); > > > > unsigned int id() const { return id_; } > > camera3_device_t *camera3Device() { return &camera3Device_; } > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp > > index 696e8043..981a3d30 100644 > > --- a/src/android/camera_ops.cpp > > +++ b/src/android/camera_ops.cpp > > @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, > > > > static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) > > You can drop [[maybe_unused]]. > > It would be nice to have an [[unused]] that causes a warning when the > variable is used. > > > { > > - return 0; > > + if (!dev) > > + return -EINVAL; > > + > > + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv); > > + return camera->flush(); > > } > > > > int hal_dev_close(hw_device_t *hw_device) > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Mon, Apr 26, 2021 at 11:51:40AM +0900, Hirokazu Honda wrote: > On Mon, Apr 26, 2021 at 9:37 AM Laurent Pinchart wrote: > > On Fri, Apr 23, 2021 at 01:07:38PM +0900, Hirokazu Honda wrote: > > > This implements flush(). It is mostly the same as close() > > > though the canceled events are reported with error messages. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------ > > > src/android/camera_device.h | 1 + > > > src/android/camera_ops.cpp | 6 ++++- > > > 3 files changed, 40 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index c74dc0e7..22a2e13c 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -753,6 +753,15 @@ void CameraDevice::close() > > > stop(/*releaseCamera=*/true); > > > } > > > > > > +int CameraDevice::flush() > > > +{ > > > + std::scoped_lock<std::mutex> lock(mutex_); > > > + > > > + stop(); > > > + > > > + return 0; > > > +} > > > + > > > void CameraDevice::stop(bool releaseCamera) > > > { > > > streams_.clear(); > > > @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera) > > > worker_.stop(); > > > camera_->stop(); > > > > > > + for (auto &[cookie, descriptor] : descriptors_) { > > > + LOG(HAL, Debug) << "request is canceled: " << cookie; > > > > LOG(HAL, Debug) << "request " << cookie << " is canceled"; > > > > > + > > > + camera3_capture_result_t captureResult = {}; > > > + captureResult.frame_number = descriptor.frameNumber_; > > > + captureResult.num_output_buffers = descriptor.buffers_.size(); > > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > > > + buffer.acquire_fence = -1; > > > + buffer.release_fence = -1; > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > + } > > > + captureResult.output_buffers = descriptor.buffers_.data(); > > > + > > > + notifyError(descriptor.frameNumber_, > > > + descriptor.buffers_[0].stream); > > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > > > This looks very similar to the code in CameraDevice::requestComplete(), > > would it make sense to move it to a separate function (including the > > call to notifyError) ? > > Sure, I will factorize in the next patch. > My first thought is to reduce the number of patches in the series and > factorize them after the patches are merged to not complicate this > patch series any more. I actually makes review easier if you factor out the code in a separate function in a patch of its own, before this patch. > > > + } > > > descriptors_.clear(); > > > > > > running_ = false; > > > @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request) > > > } > > > > > > if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) { > > > + /* > > > + * \todo Report and identify the stream number or configuration > > > + * to clarify the stream that failed. > > > + */ > > > + LOG(HAL, Error) > > > + << "Error occurred on frame " << descriptor.frameNumber_ << " (" > > > + << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")"; > > > + > > > /* \todo Improve error handling. In case we notify an error > > > * because the metadata generation fails, a shutter event has > > > * already been notified for this frame number before the error > > > @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) > > > { > > > camera3_notify_msg_t notify = {}; > > > > > > - /* > > > - * \todo Report and identify the stream number or configuration to > > > - * clarify the stream that failed. > > > - */ > > > - LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " (" > > > - << toPixelFormat(stream->format).toString() << ")"; > > > - > > > notify.type = CAMERA3_MSG_ERROR; > > > notify.message.error.error_stream = stream; > > > notify.message.error.frame_number = frameNumber; > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > index 08553584..7004c89a 100644 > > > --- a/src/android/camera_device.h > > > +++ b/src/android/camera_device.h > > > @@ -41,6 +41,7 @@ public: > > > > > > int open(const hw_module_t *hardwareModule); > > > void close(); > > > + int flush(); > > > > > > unsigned int id() const { return id_; } > > > camera3_device_t *camera3Device() { return &camera3Device_; } > > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp > > > index 696e8043..981a3d30 100644 > > > --- a/src/android/camera_ops.cpp > > > +++ b/src/android/camera_ops.cpp > > > @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, > > > > > > static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) > > > > You can drop [[maybe_unused]]. > > > > It would be nice to have an [[unused]] that causes a warning when the > > variable is used. > > > > > { > > > - return 0; > > > + if (!dev) > > > + return -EINVAL; > > > + > > > + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv); > > > + return camera->flush(); > > > } > > > > > > int hal_dev_close(hw_device_t *hw_device)
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index c74dc0e7..22a2e13c 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -753,6 +753,15 @@ void CameraDevice::close() stop(/*releaseCamera=*/true); } +int CameraDevice::flush() +{ + std::scoped_lock<std::mutex> lock(mutex_); + + stop(); + + return 0; +} + void CameraDevice::stop(bool releaseCamera) { streams_.clear(); @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera) worker_.stop(); camera_->stop(); + for (auto &[cookie, descriptor] : descriptors_) { + LOG(HAL, Debug) << "request is canceled: " << cookie; + + camera3_capture_result_t captureResult = {}; + captureResult.frame_number = descriptor.frameNumber_; + captureResult.num_output_buffers = descriptor.buffers_.size(); + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { + buffer.acquire_fence = -1; + buffer.release_fence = -1; + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; + } + captureResult.output_buffers = descriptor.buffers_.data(); + + notifyError(descriptor.frameNumber_, + descriptor.buffers_[0].stream); + callbacks_->process_capture_result(callbacks_, &captureResult); + } descriptors_.clear(); running_ = false; @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request) } if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) { + /* + * \todo Report and identify the stream number or configuration + * to clarify the stream that failed. + */ + LOG(HAL, Error) + << "Error occurred on frame " << descriptor.frameNumber_ << " (" + << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")"; + /* \todo Improve error handling. In case we notify an error * because the metadata generation fails, a shutter event has * already been notified for this frame number before the error @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) { camera3_notify_msg_t notify = {}; - /* - * \todo Report and identify the stream number or configuration to - * clarify the stream that failed. - */ - LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " (" - << toPixelFormat(stream->format).toString() << ")"; - notify.type = CAMERA3_MSG_ERROR; notify.message.error.error_stream = stream; notify.message.error.frame_number = frameNumber; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 08553584..7004c89a 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -41,6 +41,7 @@ public: int open(const hw_module_t *hardwareModule); void close(); + int flush(); unsigned int id() const { return id_; } camera3_device_t *camera3Device() { return &camera3Device_; } diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp index 696e8043..981a3d30 100644 --- a/src/android/camera_ops.cpp +++ b/src/android/camera_ops.cpp @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) { - return 0; + if (!dev) + return -EINVAL; + + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv); + return camera->flush(); } int hal_dev_close(hw_device_t *hw_device)
This implements flush(). It is mostly the same as close() though the canceled events are reported with error messages. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------ src/android/camera_device.h | 1 + src/android/camera_ops.cpp | 6 ++++- 3 files changed, 40 insertions(+), 8 deletions(-)