| Message ID | 20211011073505.243864-5-umang.jain@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Mon, Oct 11, 2021 at 01:05:02PM +0530, Umang Jain wrote: > CameraStream::process() is invoked by CameraDevice::requestComplete() > in case any post-processing is required for the camera stream. The > failure or success is checked via the value returned by > CameraStream::process(). > > Now that the post-processor notifies about the post-processing > completion operation, we can drop the return value of > CameraStream::process(). The status of post-processing is passed > to CameraDevice::streamProcessingComplete() by the > PostProcessor::processComplete slot. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 2 +- > src/android/camera_stream.cpp | 14 +++++++------- > src/android/camera_stream.h | 6 +++--- > src/android/jpeg/post_processor_jpeg.cpp | 12 +++++------- > src/android/jpeg/post_processor_jpeg.h | 6 +++--- > src/android/post_processor.h | 6 +++--- > src/android/yuv/post_processor_yuv.cpp | 14 ++++++-------- > src/android/yuv/post_processor_yuv.h | 6 +++--- > 8 files changed, 31 insertions(+), 35 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 9f26c36d..eba370ea 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1228,7 +1228,7 @@ void CameraDevice::requestComplete(Request *request) > if (cameraStream->type() == CameraStream::Type::Internal) > descriptor->internalBuffer_ = src; > > - int ret = cameraStream->process(*src, buffer, descriptor); > + cameraStream->process(*src, buffer, descriptor); > > return; I forgot to comment in the previous patch that you should only return early if no stream requires post-processing, not after the first one. > } > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index d91e7dee..cec07269 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -147,9 +147,9 @@ int CameraStream::waitFence(int fence) > return -errno; > } > > -int CameraStream::process(const FrameBuffer &source, > - camera3_stream_buffer_t &camera3Dest, > - Camera3RequestDescriptor *request) > +void CameraStream::process(const FrameBuffer &source, > + camera3_stream_buffer_t &camera3Dest, > + Camera3RequestDescriptor *request) > { > /* Handle waiting on fences on the destination buffer. */ > int fence = camera3Dest.acquire_fence; > @@ -160,12 +160,12 @@ int CameraStream::process(const FrameBuffer &source, > if (ret < 0) { > LOG(HAL, Error) << "Failed waiting for fence: " > << fence << ": " << strerror(-ret); > - return ret; > + return; > } > } > > if (!postProcessor_) > - return 0; > + return; Can this happen btw ? > > /* > * \todo Buffer mapping and processing should be moved to a > @@ -176,10 +176,10 @@ int CameraStream::process(const FrameBuffer &source, > PROT_READ | PROT_WRITE); > if (!dest.isValid()) { > LOG(HAL, Error) << "Failed to create destination buffer"; > - return -EINVAL; > + return; > } We have a set of failure conditions here that don't result in any notification to the CameraDevice, the request will be lost. You could call CameraDevice::streamProcessingComplete() in those locations, but it may be easier to keep returning an int from this function (not the ones below) and handle errors in the caller. > > - return postProcessor_->process(source, &dest, request); > + postProcessor_->process(source, &dest, request); > } > > FrameBuffer *CameraStream::getBuffer() > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index 04cfd111..a0c5f166 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -120,9 +120,9 @@ public: > libcamera::Stream *stream() const; > > int configure(); > - int process(const libcamera::FrameBuffer &source, > - camera3_stream_buffer_t &camera3Buffer, > - Camera3RequestDescriptor *request); > + void process(const libcamera::FrameBuffer &source, > + camera3_stream_buffer_t &camera3Buffer, > + Camera3RequestDescriptor *request); > libcamera::FrameBuffer *getBuffer(); > void putBuffer(libcamera::FrameBuffer *buffer); > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 81d1efe6..eb87931b 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -97,12 +97,12 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > } > } > > -int PostProcessorJpeg::process(const FrameBuffer &source, > - CameraBuffer *destination, > - Camera3RequestDescriptor *request) > +void PostProcessorJpeg::process(const FrameBuffer &source, > + CameraBuffer *destination, > + Camera3RequestDescriptor *request) > { > if (!encoder_) > - return 0; > + return; Same here, if this can happen, we'll lose the request, if it can't happen, it should be dropped, or replaced with an ASSERT() if you're not sure. > > ASSERT(destination->numPlanes() == 1); > > @@ -198,7 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source, > if (jpeg_size < 0) { > LOG(JPEG, Error) << "Failed to encode stream image"; > processComplete.emit(request, PostProcessor::Status::Error); > - return jpeg_size; > + return; > } > > /* Fill in the JPEG blob header. */ > @@ -212,6 +212,4 @@ int PostProcessorJpeg::process(const FrameBuffer &source, > /* Update the JPEG result Metadata. */ > resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); > processComplete.emit(request, PostProcessor::Status::Success); > - > - return 0; > } > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > index 0184d77e..5dab14e1 100644 > --- a/src/android/jpeg/post_processor_jpeg.h > +++ b/src/android/jpeg/post_processor_jpeg.h > @@ -22,9 +22,9 @@ public: > > int configure(const libcamera::StreamConfiguration &incfg, > const libcamera::StreamConfiguration &outcfg) override; > - int process(const libcamera::FrameBuffer &source, > - CameraBuffer *destination, > - Camera3RequestDescriptor *request) override; > + void process(const libcamera::FrameBuffer &source, > + CameraBuffer *destination, > + Camera3RequestDescriptor *request) override; > > private: > void generateThumbnail(const libcamera::FrameBuffer &source, > diff --git a/src/android/post_processor.h b/src/android/post_processor.h > index 6e67bcba..88a5f985 100644 > --- a/src/android/post_processor.h > +++ b/src/android/post_processor.h > @@ -30,9 +30,9 @@ public: > > virtual int configure(const libcamera::StreamConfiguration &inCfg, > const libcamera::StreamConfiguration &outCfg) = 0; > - virtual int process(const libcamera::FrameBuffer &source, > - CameraBuffer *destination, > - Camera3RequestDescriptor *request) = 0; > + virtual void process(const libcamera::FrameBuffer &source, > + CameraBuffer *destination, > + Camera3RequestDescriptor *request) = 0; > > libcamera::Signal<Camera3RequestDescriptor *, Status> processComplete; > }; > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > index b34b389d..860a1a7f 100644 > --- a/src/android/yuv/post_processor_yuv.cpp > +++ b/src/android/yuv/post_processor_yuv.cpp > @@ -51,20 +51,20 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, > return 0; > } > > -int PostProcessorYuv::process(const FrameBuffer &source, > - CameraBuffer *destination, > - Camera3RequestDescriptor *request) > +void PostProcessorYuv::process(const FrameBuffer &source, > + CameraBuffer *destination, > + Camera3RequestDescriptor *request) > { > if (!isValidBuffers(source, *destination)) { > processComplete.emit(request, PostProcessor::Status::Error); > - return -EINVAL; > + return; > } > > const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read); > if (!sourceMapped.isValid()) { > LOG(YUV, Error) << "Failed to mmap camera frame buffer"; > processComplete.emit(request, PostProcessor::Status::Error); > - return -EINVAL; > + return; > } > > int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(), > @@ -82,12 +82,10 @@ int PostProcessorYuv::process(const FrameBuffer &source, > if (ret) { > LOG(YUV, Error) << "Failed NV12 scaling: " << ret; > processComplete.emit(request, PostProcessor::Status::Error); > - return -EINVAL; > + return; > } > > processComplete.emit(request, PostProcessor::Status::Success); > - > - return 0; > } > > bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, > diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h > index a4e0ff5d..1278843b 100644 > --- a/src/android/yuv/post_processor_yuv.h > +++ b/src/android/yuv/post_processor_yuv.h > @@ -18,9 +18,9 @@ public: > > int configure(const libcamera::StreamConfiguration &incfg, > const libcamera::StreamConfiguration &outcfg) override; > - int process(const libcamera::FrameBuffer &source, > - CameraBuffer *destination, > - Camera3RequestDescriptor *request) override; > + void process(const libcamera::FrameBuffer &source, > + CameraBuffer *destination, > + Camera3RequestDescriptor *request) override; > > private: > bool isValidBuffers(const libcamera::FrameBuffer &source,
Hi Laurent, On 10/12/21 9:09 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Mon, Oct 11, 2021 at 01:05:02PM +0530, Umang Jain wrote: >> CameraStream::process() is invoked by CameraDevice::requestComplete() >> in case any post-processing is required for the camera stream. The >> failure or success is checked via the value returned by >> CameraStream::process(). >> >> Now that the post-processor notifies about the post-processing >> completion operation, we can drop the return value of >> CameraStream::process(). The status of post-processing is passed >> to CameraDevice::streamProcessingComplete() by the >> PostProcessor::processComplete slot. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 2 +- >> src/android/camera_stream.cpp | 14 +++++++------- >> src/android/camera_stream.h | 6 +++--- >> src/android/jpeg/post_processor_jpeg.cpp | 12 +++++------- >> src/android/jpeg/post_processor_jpeg.h | 6 +++--- >> src/android/post_processor.h | 6 +++--- >> src/android/yuv/post_processor_yuv.cpp | 14 ++++++-------- >> src/android/yuv/post_processor_yuv.h | 6 +++--- >> 8 files changed, 31 insertions(+), 35 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 9f26c36d..eba370ea 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -1228,7 +1228,7 @@ void CameraDevice::requestComplete(Request *request) >> if (cameraStream->type() == CameraStream::Type::Internal) >> descriptor->internalBuffer_ = src; >> >> - int ret = cameraStream->process(*src, buffer, descriptor); >> + cameraStream->process(*src, buffer, descriptor); >> >> return; > I forgot to comment in the previous patch that you should only return > early if no stream requires post-processing, not after the first one. We can only know that by iterating over the descriptor->buffers_ right? We return early here since, we don't want to trigger sendCaptureResults() below, outside the loop. Since we are now discussing multiple post-processing requests even for single requestComplete(), this loop becomes crucial as it will queue up all the post-processing requests to relevant cameraStreams I'll think how should we return in that case. > >> } >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp >> index d91e7dee..cec07269 100644 >> --- a/src/android/camera_stream.cpp >> +++ b/src/android/camera_stream.cpp >> @@ -147,9 +147,9 @@ int CameraStream::waitFence(int fence) >> return -errno; >> } >> >> -int CameraStream::process(const FrameBuffer &source, >> - camera3_stream_buffer_t &camera3Dest, >> - Camera3RequestDescriptor *request) >> +void CameraStream::process(const FrameBuffer &source, >> + camera3_stream_buffer_t &camera3Dest, >> + Camera3RequestDescriptor *request) >> { >> /* Handle waiting on fences on the destination buffer. */ >> int fence = camera3Dest.acquire_fence; >> @@ -160,12 +160,12 @@ int CameraStream::process(const FrameBuffer &source, >> if (ret < 0) { >> LOG(HAL, Error) << "Failed waiting for fence: " >> << fence << ": " << strerror(-ret); >> - return ret; >> + return; >> } >> } >> >> if (!postProcessor_) >> - return 0; >> + return; > Can this happen btw ? If it happens, it'll be the caller's error for triggering it. A CameraStream Type::Direct and you call a ->process() on it. Doesn't make sense right? Should be up the intensity and put an assert instead? ASSERT (type_ != Type::Direct) > >> >> /* >> * \todo Buffer mapping and processing should be moved to a >> @@ -176,10 +176,10 @@ int CameraStream::process(const FrameBuffer &source, >> PROT_READ | PROT_WRITE); >> if (!dest.isValid()) { >> LOG(HAL, Error) << "Failed to create destination buffer"; >> - return -EINVAL; >> + return; >> } > We have a set of failure conditions here that don't result in any > notification to the CameraDevice, the request will be lost. You could > call CameraDevice::streamProcessingComplete() in those locations, but it > may be easier to keep returning an int from this function (not the ones > below) and handle errors in the caller. Every error handle patch should call cameraDevice_->streamProcessingComplete(this, request, Camera3RequestDescriptor::Status::Error) This is my mistake not to closely follow the error handling paths in this patch > >> >> - return postProcessor_->process(source, &dest, request); >> + postProcessor_->process(source, &dest, request); >> } >> >> FrameBuffer *CameraStream::getBuffer() >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h >> index 04cfd111..a0c5f166 100644 >> --- a/src/android/camera_stream.h >> +++ b/src/android/camera_stream.h >> @@ -120,9 +120,9 @@ public: >> libcamera::Stream *stream() const; >> >> int configure(); >> - int process(const libcamera::FrameBuffer &source, >> - camera3_stream_buffer_t &camera3Buffer, >> - Camera3RequestDescriptor *request); >> + void process(const libcamera::FrameBuffer &source, >> + camera3_stream_buffer_t &camera3Buffer, >> + Camera3RequestDescriptor *request); >> libcamera::FrameBuffer *getBuffer(); >> void putBuffer(libcamera::FrameBuffer *buffer); >> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp >> index 81d1efe6..eb87931b 100644 >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> @@ -97,12 +97,12 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, >> } >> } >> >> -int PostProcessorJpeg::process(const FrameBuffer &source, >> - CameraBuffer *destination, >> - Camera3RequestDescriptor *request) >> +void PostProcessorJpeg::process(const FrameBuffer &source, >> + CameraBuffer *destination, >> + Camera3RequestDescriptor *request) >> { >> if (!encoder_) >> - return 0; >> + return; > Same here, if this can happen, we'll lose the request, if it can't > happen, it should be dropped, or replaced with an ASSERT() if you're not > sure. > >> >> ASSERT(destination->numPlanes() == 1); >> >> @@ -198,7 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source, >> if (jpeg_size < 0) { >> LOG(JPEG, Error) << "Failed to encode stream image"; >> processComplete.emit(request, PostProcessor::Status::Error); >> - return jpeg_size; >> + return; >> } >> >> /* Fill in the JPEG blob header. */ >> @@ -212,6 +212,4 @@ int PostProcessorJpeg::process(const FrameBuffer &source, >> /* Update the JPEG result Metadata. */ >> resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); >> processComplete.emit(request, PostProcessor::Status::Success); >> - >> - return 0; >> } >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h >> index 0184d77e..5dab14e1 100644 >> --- a/src/android/jpeg/post_processor_jpeg.h >> +++ b/src/android/jpeg/post_processor_jpeg.h >> @@ -22,9 +22,9 @@ public: >> >> int configure(const libcamera::StreamConfiguration &incfg, >> const libcamera::StreamConfiguration &outcfg) override; >> - int process(const libcamera::FrameBuffer &source, >> - CameraBuffer *destination, >> - Camera3RequestDescriptor *request) override; >> + void process(const libcamera::FrameBuffer &source, >> + CameraBuffer *destination, >> + Camera3RequestDescriptor *request) override; >> >> private: >> void generateThumbnail(const libcamera::FrameBuffer &source, >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h >> index 6e67bcba..88a5f985 100644 >> --- a/src/android/post_processor.h >> +++ b/src/android/post_processor.h >> @@ -30,9 +30,9 @@ public: >> >> virtual int configure(const libcamera::StreamConfiguration &inCfg, >> const libcamera::StreamConfiguration &outCfg) = 0; >> - virtual int process(const libcamera::FrameBuffer &source, >> - CameraBuffer *destination, >> - Camera3RequestDescriptor *request) = 0; >> + virtual void process(const libcamera::FrameBuffer &source, >> + CameraBuffer *destination, >> + Camera3RequestDescriptor *request) = 0; >> >> libcamera::Signal<Camera3RequestDescriptor *, Status> processComplete; >> }; >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp >> index b34b389d..860a1a7f 100644 >> --- a/src/android/yuv/post_processor_yuv.cpp >> +++ b/src/android/yuv/post_processor_yuv.cpp >> @@ -51,20 +51,20 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, >> return 0; >> } >> >> -int PostProcessorYuv::process(const FrameBuffer &source, >> - CameraBuffer *destination, >> - Camera3RequestDescriptor *request) >> +void PostProcessorYuv::process(const FrameBuffer &source, >> + CameraBuffer *destination, >> + Camera3RequestDescriptor *request) >> { >> if (!isValidBuffers(source, *destination)) { >> processComplete.emit(request, PostProcessor::Status::Error); >> - return -EINVAL; >> + return; >> } >> >> const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read); >> if (!sourceMapped.isValid()) { >> LOG(YUV, Error) << "Failed to mmap camera frame buffer"; >> processComplete.emit(request, PostProcessor::Status::Error); >> - return -EINVAL; >> + return; >> } >> >> int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(), >> @@ -82,12 +82,10 @@ int PostProcessorYuv::process(const FrameBuffer &source, >> if (ret) { >> LOG(YUV, Error) << "Failed NV12 scaling: " << ret; >> processComplete.emit(request, PostProcessor::Status::Error); >> - return -EINVAL; >> + return; >> } >> >> processComplete.emit(request, PostProcessor::Status::Success); >> - >> - return 0; >> } >> >> bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, >> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h >> index a4e0ff5d..1278843b 100644 >> --- a/src/android/yuv/post_processor_yuv.h >> +++ b/src/android/yuv/post_processor_yuv.h >> @@ -18,9 +18,9 @@ public: >> >> int configure(const libcamera::StreamConfiguration &incfg, >> const libcamera::StreamConfiguration &outcfg) override; >> - int process(const libcamera::FrameBuffer &source, >> - CameraBuffer *destination, >> - Camera3RequestDescriptor *request) override; >> + void process(const libcamera::FrameBuffer &source, >> + CameraBuffer *destination, >> + Camera3RequestDescriptor *request) override; >> >> private: >> bool isValidBuffers(const libcamera::FrameBuffer &source,
Hi Umang, On Wed, Oct 13, 2021 at 03:33:00PM +0530, Umang Jain wrote: > On 10/12/21 9:09 AM, Laurent Pinchart wrote: > > On Mon, Oct 11, 2021 at 01:05:02PM +0530, Umang Jain wrote: > >> CameraStream::process() is invoked by CameraDevice::requestComplete() > >> in case any post-processing is required for the camera stream. The > >> failure or success is checked via the value returned by > >> CameraStream::process(). > >> > >> Now that the post-processor notifies about the post-processing > >> completion operation, we can drop the return value of > >> CameraStream::process(). The status of post-processing is passed > >> to CameraDevice::streamProcessingComplete() by the > >> PostProcessor::processComplete slot. > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> src/android/camera_device.cpp | 2 +- > >> src/android/camera_stream.cpp | 14 +++++++------- > >> src/android/camera_stream.h | 6 +++--- > >> src/android/jpeg/post_processor_jpeg.cpp | 12 +++++------- > >> src/android/jpeg/post_processor_jpeg.h | 6 +++--- > >> src/android/post_processor.h | 6 +++--- > >> src/android/yuv/post_processor_yuv.cpp | 14 ++++++-------- > >> src/android/yuv/post_processor_yuv.h | 6 +++--- > >> 8 files changed, 31 insertions(+), 35 deletions(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index 9f26c36d..eba370ea 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -1228,7 +1228,7 @@ void CameraDevice::requestComplete(Request *request) > >> if (cameraStream->type() == CameraStream::Type::Internal) > >> descriptor->internalBuffer_ = src; > >> > >> - int ret = cameraStream->process(*src, buffer, descriptor); > >> + cameraStream->process(*src, buffer, descriptor); > >> > >> return; > > > > I forgot to comment in the previous patch that you should only return > > early if no stream requires post-processing, not after the first one. > > We can only know that by iterating over the descriptor->buffers_ right? > We return early here since, we don't want to trigger > sendCaptureResults() below, outside the loop. > > Since we are now discussing multiple post-processing requests even for > single requestComplete(), this loop becomes crucial as it will queue up > all the post-processing requests to relevant cameraStreams > > I'll think how should we return in that case. You can have a local variable that indicates if at least one stream required post-processing, and return after this loop in that case. > >> } > >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > >> index d91e7dee..cec07269 100644 > >> --- a/src/android/camera_stream.cpp > >> +++ b/src/android/camera_stream.cpp > >> @@ -147,9 +147,9 @@ int CameraStream::waitFence(int fence) > >> return -errno; > >> } > >> > >> -int CameraStream::process(const FrameBuffer &source, > >> - camera3_stream_buffer_t &camera3Dest, > >> - Camera3RequestDescriptor *request) > >> +void CameraStream::process(const FrameBuffer &source, > >> + camera3_stream_buffer_t &camera3Dest, > >> + Camera3RequestDescriptor *request) > >> { > >> /* Handle waiting on fences on the destination buffer. */ > >> int fence = camera3Dest.acquire_fence; > >> @@ -160,12 +160,12 @@ int CameraStream::process(const FrameBuffer &source, > >> if (ret < 0) { > >> LOG(HAL, Error) << "Failed waiting for fence: " > >> << fence << ": " << strerror(-ret); > >> - return ret; > >> + return; > >> } > >> } > >> > >> if (!postProcessor_) > >> - return 0; > >> + return; > > > > Can this happen btw ? > > If it happens, it'll be the caller's error for triggering it. > > A CameraStream Type::Direct and you call a ->process() on it. Doesn't > make sense right? Should be up the intensity and put an assert instead? > > ASSERT (type_ != Type::Direct) If it can't happen unless there's a bug in the HAL, ASSERT() will likely be easier, yes. Otherwise you need to notify an error to the caller or the request will be lost silently. > >> > >> /* > >> * \todo Buffer mapping and processing should be moved to a > >> @@ -176,10 +176,10 @@ int CameraStream::process(const FrameBuffer &source, > >> PROT_READ | PROT_WRITE); > >> if (!dest.isValid()) { > >> LOG(HAL, Error) << "Failed to create destination buffer"; > >> - return -EINVAL; > >> + return; > >> } > > > > We have a set of failure conditions here that don't result in any > > notification to the CameraDevice, the request will be lost. You could > > call CameraDevice::streamProcessingComplete() in those locations, but it > > may be easier to keep returning an int from this function (not the ones > > below) and handle errors in the caller. > > Every error handle patch should call > > cameraDevice_->streamProcessingComplete(this, request, > Camera3RequestDescriptor::Status::Error) To avoid calling that in every single location here, I think CameraStream::process() should return an error status. The caller can then handle it. It's like queuing a request, errors can be returned synchronously, but if the call succeeds, then asynchronous errors are notified through a different mechanism. > This is my mistake not to closely follow the error handling paths in > this patch > > >> - return postProcessor_->process(source, &dest, request); > >> + postProcessor_->process(source, &dest, request); > >> } > >> > >> FrameBuffer *CameraStream::getBuffer() > >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > >> index 04cfd111..a0c5f166 100644 > >> --- a/src/android/camera_stream.h > >> +++ b/src/android/camera_stream.h > >> @@ -120,9 +120,9 @@ public: > >> libcamera::Stream *stream() const; > >> > >> int configure(); > >> - int process(const libcamera::FrameBuffer &source, > >> - camera3_stream_buffer_t &camera3Buffer, > >> - Camera3RequestDescriptor *request); > >> + void process(const libcamera::FrameBuffer &source, > >> + camera3_stream_buffer_t &camera3Buffer, > >> + Camera3RequestDescriptor *request); > >> libcamera::FrameBuffer *getBuffer(); > >> void putBuffer(libcamera::FrameBuffer *buffer); > >> > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > >> index 81d1efe6..eb87931b 100644 > >> --- a/src/android/jpeg/post_processor_jpeg.cpp > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp > >> @@ -97,12 +97,12 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > >> } > >> } > >> > >> -int PostProcessorJpeg::process(const FrameBuffer &source, > >> - CameraBuffer *destination, > >> - Camera3RequestDescriptor *request) > >> +void PostProcessorJpeg::process(const FrameBuffer &source, > >> + CameraBuffer *destination, > >> + Camera3RequestDescriptor *request) > >> { > >> if (!encoder_) > >> - return 0; > >> + return; > > > > Same here, if this can happen, we'll lose the request, if it can't > > happen, it should be dropped, or replaced with an ASSERT() if you're not > > sure. > > > >> ASSERT(destination->numPlanes() == 1); > >> > >> @@ -198,7 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source, > >> if (jpeg_size < 0) { > >> LOG(JPEG, Error) << "Failed to encode stream image"; > >> processComplete.emit(request, PostProcessor::Status::Error); > >> - return jpeg_size; > >> + return; > >> } > >> > >> /* Fill in the JPEG blob header. */ > >> @@ -212,6 +212,4 @@ int PostProcessorJpeg::process(const FrameBuffer &source, > >> /* Update the JPEG result Metadata. */ > >> resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); > >> processComplete.emit(request, PostProcessor::Status::Success); > >> - > >> - return 0; > >> } > >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > >> index 0184d77e..5dab14e1 100644 > >> --- a/src/android/jpeg/post_processor_jpeg.h > >> +++ b/src/android/jpeg/post_processor_jpeg.h > >> @@ -22,9 +22,9 @@ public: > >> > >> int configure(const libcamera::StreamConfiguration &incfg, > >> const libcamera::StreamConfiguration &outcfg) override; > >> - int process(const libcamera::FrameBuffer &source, > >> - CameraBuffer *destination, > >> - Camera3RequestDescriptor *request) override; > >> + void process(const libcamera::FrameBuffer &source, > >> + CameraBuffer *destination, > >> + Camera3RequestDescriptor *request) override; > >> > >> private: > >> void generateThumbnail(const libcamera::FrameBuffer &source, > >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h > >> index 6e67bcba..88a5f985 100644 > >> --- a/src/android/post_processor.h > >> +++ b/src/android/post_processor.h > >> @@ -30,9 +30,9 @@ public: > >> > >> virtual int configure(const libcamera::StreamConfiguration &inCfg, > >> const libcamera::StreamConfiguration &outCfg) = 0; > >> - virtual int process(const libcamera::FrameBuffer &source, > >> - CameraBuffer *destination, > >> - Camera3RequestDescriptor *request) = 0; > >> + virtual void process(const libcamera::FrameBuffer &source, > >> + CameraBuffer *destination, > >> + Camera3RequestDescriptor *request) = 0; > >> > >> libcamera::Signal<Camera3RequestDescriptor *, Status> processComplete; > >> }; > >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > >> index b34b389d..860a1a7f 100644 > >> --- a/src/android/yuv/post_processor_yuv.cpp > >> +++ b/src/android/yuv/post_processor_yuv.cpp > >> @@ -51,20 +51,20 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, > >> return 0; > >> } > >> > >> -int PostProcessorYuv::process(const FrameBuffer &source, > >> - CameraBuffer *destination, > >> - Camera3RequestDescriptor *request) > >> +void PostProcessorYuv::process(const FrameBuffer &source, > >> + CameraBuffer *destination, > >> + Camera3RequestDescriptor *request) > >> { > >> if (!isValidBuffers(source, *destination)) { > >> processComplete.emit(request, PostProcessor::Status::Error); > >> - return -EINVAL; > >> + return; > >> } > >> > >> const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read); > >> if (!sourceMapped.isValid()) { > >> LOG(YUV, Error) << "Failed to mmap camera frame buffer"; > >> processComplete.emit(request, PostProcessor::Status::Error); > >> - return -EINVAL; > >> + return; > >> } > >> > >> int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(), > >> @@ -82,12 +82,10 @@ int PostProcessorYuv::process(const FrameBuffer &source, > >> if (ret) { > >> LOG(YUV, Error) << "Failed NV12 scaling: " << ret; > >> processComplete.emit(request, PostProcessor::Status::Error); > >> - return -EINVAL; > >> + return; > >> } > >> > >> processComplete.emit(request, PostProcessor::Status::Success); > >> - > >> - return 0; > >> } > >> > >> bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, > >> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h > >> index a4e0ff5d..1278843b 100644 > >> --- a/src/android/yuv/post_processor_yuv.h > >> +++ b/src/android/yuv/post_processor_yuv.h > >> @@ -18,9 +18,9 @@ public: > >> > >> int configure(const libcamera::StreamConfiguration &incfg, > >> const libcamera::StreamConfiguration &outcfg) override; > >> - int process(const libcamera::FrameBuffer &source, > >> - CameraBuffer *destination, > >> - Camera3RequestDescriptor *request) override; > >> + void process(const libcamera::FrameBuffer &source, > >> + CameraBuffer *destination, > >> + Camera3RequestDescriptor *request) override; > >> > >> private: > >> bool isValidBuffers(const libcamera::FrameBuffer &source,
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 9f26c36d..eba370ea 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1228,7 +1228,7 @@ void CameraDevice::requestComplete(Request *request) if (cameraStream->type() == CameraStream::Type::Internal) descriptor->internalBuffer_ = src; - int ret = cameraStream->process(*src, buffer, descriptor); + cameraStream->process(*src, buffer, descriptor); return; } diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index d91e7dee..cec07269 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -147,9 +147,9 @@ int CameraStream::waitFence(int fence) return -errno; } -int CameraStream::process(const FrameBuffer &source, - camera3_stream_buffer_t &camera3Dest, - Camera3RequestDescriptor *request) +void CameraStream::process(const FrameBuffer &source, + camera3_stream_buffer_t &camera3Dest, + Camera3RequestDescriptor *request) { /* Handle waiting on fences on the destination buffer. */ int fence = camera3Dest.acquire_fence; @@ -160,12 +160,12 @@ int CameraStream::process(const FrameBuffer &source, if (ret < 0) { LOG(HAL, Error) << "Failed waiting for fence: " << fence << ": " << strerror(-ret); - return ret; + return; } } if (!postProcessor_) - return 0; + return; /* * \todo Buffer mapping and processing should be moved to a @@ -176,10 +176,10 @@ int CameraStream::process(const FrameBuffer &source, PROT_READ | PROT_WRITE); if (!dest.isValid()) { LOG(HAL, Error) << "Failed to create destination buffer"; - return -EINVAL; + return; } - return postProcessor_->process(source, &dest, request); + postProcessor_->process(source, &dest, request); } FrameBuffer *CameraStream::getBuffer() diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h index 04cfd111..a0c5f166 100644 --- a/src/android/camera_stream.h +++ b/src/android/camera_stream.h @@ -120,9 +120,9 @@ public: libcamera::Stream *stream() const; int configure(); - int process(const libcamera::FrameBuffer &source, - camera3_stream_buffer_t &camera3Buffer, - Camera3RequestDescriptor *request); + void process(const libcamera::FrameBuffer &source, + camera3_stream_buffer_t &camera3Buffer, + Camera3RequestDescriptor *request); libcamera::FrameBuffer *getBuffer(); void putBuffer(libcamera::FrameBuffer *buffer); diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 81d1efe6..eb87931b 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -97,12 +97,12 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, } } -int PostProcessorJpeg::process(const FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) +void PostProcessorJpeg::process(const FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) { if (!encoder_) - return 0; + return; ASSERT(destination->numPlanes() == 1); @@ -198,7 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source, if (jpeg_size < 0) { LOG(JPEG, Error) << "Failed to encode stream image"; processComplete.emit(request, PostProcessor::Status::Error); - return jpeg_size; + return; } /* Fill in the JPEG blob header. */ @@ -212,6 +212,4 @@ int PostProcessorJpeg::process(const FrameBuffer &source, /* Update the JPEG result Metadata. */ resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); processComplete.emit(request, PostProcessor::Status::Success); - - return 0; } diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h index 0184d77e..5dab14e1 100644 --- a/src/android/jpeg/post_processor_jpeg.h +++ b/src/android/jpeg/post_processor_jpeg.h @@ -22,9 +22,9 @@ public: int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; - int process(const libcamera::FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) override; + void process(const libcamera::FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) override; private: void generateThumbnail(const libcamera::FrameBuffer &source, diff --git a/src/android/post_processor.h b/src/android/post_processor.h index 6e67bcba..88a5f985 100644 --- a/src/android/post_processor.h +++ b/src/android/post_processor.h @@ -30,9 +30,9 @@ public: virtual int configure(const libcamera::StreamConfiguration &inCfg, const libcamera::StreamConfiguration &outCfg) = 0; - virtual int process(const libcamera::FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) = 0; + virtual void process(const libcamera::FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) = 0; libcamera::Signal<Camera3RequestDescriptor *, Status> processComplete; }; diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp index b34b389d..860a1a7f 100644 --- a/src/android/yuv/post_processor_yuv.cpp +++ b/src/android/yuv/post_processor_yuv.cpp @@ -51,20 +51,20 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, return 0; } -int PostProcessorYuv::process(const FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) +void PostProcessorYuv::process(const FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) { if (!isValidBuffers(source, *destination)) { processComplete.emit(request, PostProcessor::Status::Error); - return -EINVAL; + return; } const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read); if (!sourceMapped.isValid()) { LOG(YUV, Error) << "Failed to mmap camera frame buffer"; processComplete.emit(request, PostProcessor::Status::Error); - return -EINVAL; + return; } int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(), @@ -82,12 +82,10 @@ int PostProcessorYuv::process(const FrameBuffer &source, if (ret) { LOG(YUV, Error) << "Failed NV12 scaling: " << ret; processComplete.emit(request, PostProcessor::Status::Error); - return -EINVAL; + return; } processComplete.emit(request, PostProcessor::Status::Success); - - return 0; } bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h index a4e0ff5d..1278843b 100644 --- a/src/android/yuv/post_processor_yuv.h +++ b/src/android/yuv/post_processor_yuv.h @@ -18,9 +18,9 @@ public: int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; - int process(const libcamera::FrameBuffer &source, - CameraBuffer *destination, - Camera3RequestDescriptor *request) override; + void process(const libcamera::FrameBuffer &source, + CameraBuffer *destination, + Camera3RequestDescriptor *request) override; private: bool isValidBuffers(const libcamera::FrameBuffer &source,
CameraStream::process() is invoked by CameraDevice::requestComplete() in case any post-processing is required for the camera stream. The failure or success is checked via the value returned by CameraStream::process(). Now that the post-processor notifies about the post-processing completion operation, we can drop the return value of CameraStream::process(). The status of post-processing is passed to CameraDevice::streamProcessingComplete() by the PostProcessor::processComplete slot. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/camera_device.cpp | 2 +- src/android/camera_stream.cpp | 14 +++++++------- src/android/camera_stream.h | 6 +++--- src/android/jpeg/post_processor_jpeg.cpp | 12 +++++------- src/android/jpeg/post_processor_jpeg.h | 6 +++--- src/android/post_processor.h | 6 +++--- src/android/yuv/post_processor_yuv.cpp | 14 ++++++-------- src/android/yuv/post_processor_yuv.h | 6 +++--- 8 files changed, 31 insertions(+), 35 deletions(-)