[libcamera-devel,v6,6/7] android: post_processor: Drop return value for process()
diff mbox series

Message ID 20211023103302.152769-7-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • Async Post Processor
Related show

Commit Message

Umang Jain Oct. 23, 2021, 10:33 a.m. UTC 
PostProcessor::process() is invoked by CameraStream class
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
PostProcessor::process(). The status of post-processing is passed
to CameraDevice::streamProcessingComplete() by the
PostProcessor::processComplete signal's slot.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_stream.cpp            |  4 +++-
 src/android/jpeg/post_processor_jpeg.cpp |  6 ++----
 src/android/jpeg/post_processor_jpeg.h   |  2 +-
 src/android/post_processor.h             |  2 +-
 src/android/yuv/post_processor_yuv.cpp   | 10 ++++------
 src/android/yuv/post_processor_yuv.h     |  2 +-
 6 files changed, 12 insertions(+), 14 deletions(-)

Comments

Hirokazu Honda Oct. 25, 2021, 11:56 a.m. UTC | #1 
Hi, Umang, thank you for the patch.

On Sat, Oct 23, 2021 at 7:33 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> PostProcessor::process() is invoked by CameraStream class
> 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
> PostProcessor::process(). The status of post-processing is passed
> to CameraDevice::streamProcessingComplete() by the
> PostProcessor::processComplete signal's slot.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_stream.cpp            |  4 +++-
>  src/android/jpeg/post_processor_jpeg.cpp |  6 ++----
>  src/android/jpeg/post_processor_jpeg.h   |  2 +-
>  src/android/post_processor.h             |  2 +-
>  src/android/yuv/post_processor_yuv.cpp   | 10 ++++------
>  src/android/yuv/post_processor_yuv.h     |  2 +-
>  6 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 4e275cde..45d0607d 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -194,7 +194,9 @@ int CameraStream::process(const FrameBuffer &source,
>
>         dest.srcBuffer = &source;
>
> -       return postProcessor_->process(&dest);
> +       postProcessor_->process(&dest);
> +
> +       return 0;
>  }

Shall we also drop the return value of CameraStream::process() as we
process any case by streamProcessingComplete()?
I think whether streamProcessingComplete() is called depends on the
return value of CameraStream::process(), which may be error-prone?

>
>  FrameBuffer *CameraStream::getBuffer()
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index cbbe7128..9d7523cb 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -98,7 +98,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>         }
>  }
>
> -int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
> +void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
>  {
>         ASSERT(encoder_);
>
> @@ -199,7 +199,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>         if (jpeg_size < 0) {
>                 LOG(JPEG, Error) << "Failed to encode stream image";
>                 processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> -               return jpeg_size;
> +               return;
>         }
>
>         /* Fill in the JPEG blob header. */
> @@ -213,6 +213,4 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>         /* Update the JPEG result Metadata. */
>         resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
>         processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> -
> -       return 0;
>  }
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 92385548..43fcbe60 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -22,7 +22,7 @@ public:
>
>         int configure(const libcamera::StreamConfiguration &incfg,
>                       const libcamera::StreamConfiguration &outcfg) override;
> -       int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
> +       void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
>
>  private:
>         void generateThumbnail(const libcamera::FrameBuffer &source,
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index 4ac74fcf..5ec71c93 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -27,7 +27,7 @@ public:
>
>         virtual int configure(const libcamera::StreamConfiguration &inCfg,
>                               const libcamera::StreamConfiguration &outCfg) = 0;
> -       virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
> +       virtual void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
>
>         libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
>  };
> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> index 8e77bf57..1ac7995a 100644
> --- a/src/android/yuv/post_processor_yuv.cpp
> +++ b/src/android/yuv/post_processor_yuv.cpp
> @@ -49,21 +49,21 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,
>         return 0;
>  }
>
> -int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
> +void PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
>  {
>         const FrameBuffer &source = *streamBuffer->srcBuffer;
>         CameraBuffer *destination = streamBuffer->destBuffer.get();
>
>         if (!isValidBuffers(source, *destination)) {
>                 processComplete.emit(streamBuffer, 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(streamBuffer, PostProcessor::Status::Error);
> -               return -EINVAL;
> +               return;
>         }
>
>         int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(),
> @@ -81,12 +81,10 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
>         if (ret) {
>                 LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
>                 processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> -               return -EINVAL;
> +               return;
>         }
>
>         processComplete.emit(streamBuffer, 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 5954e11b..39ec7994 100644
> --- a/src/android/yuv/post_processor_yuv.h
> +++ b/src/android/yuv/post_processor_yuv.h
> @@ -18,7 +18,7 @@ public:
>
>         int configure(const libcamera::StreamConfiguration &incfg,
>                       const libcamera::StreamConfiguration &outcfg) override;
> -       int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
> +       void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
>
>  private:
>         bool isValidBuffers(const libcamera::FrameBuffer &source,
> --
> 2.31.1
>
Umang Jain Oct. 25, 2021, 12:54 p.m. UTC | #2 
Hi Hiro,

On 10/25/21 5:26 PM, Hirokazu Honda wrote:
> Hi, Umang, thank you for the patch.
>
> On Sat, Oct 23, 2021 at 7:33 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>> PostProcessor::process() is invoked by CameraStream class
>> 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
>> PostProcessor::process(). The status of post-processing is passed
>> to CameraDevice::streamProcessingComplete() by the
>> PostProcessor::processComplete signal's slot.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   src/android/camera_stream.cpp            |  4 +++-
>>   src/android/jpeg/post_processor_jpeg.cpp |  6 ++----
>>   src/android/jpeg/post_processor_jpeg.h   |  2 +-
>>   src/android/post_processor.h             |  2 +-
>>   src/android/yuv/post_processor_yuv.cpp   | 10 ++++------
>>   src/android/yuv/post_processor_yuv.h     |  2 +-
>>   6 files changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 4e275cde..45d0607d 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -194,7 +194,9 @@ int CameraStream::process(const FrameBuffer &source,
>>
>>          dest.srcBuffer = &source;
>>
>> -       return postProcessor_->process(&dest);
>> +       postProcessor_->process(&dest);
>> +
>> +       return 0;
>>   }
> Shall we also drop the return value of CameraStream::process() as we
> process any case by streamProcessingComplete()?
> I think whether streamProcessingComplete() is called depends on the
> return value of CameraStream::process(), which may be error-prone?


No, this was discussed before. We want to handle CameraStream::process() 
synchronously, and streamProcessingComplete() will error-handle for the 
async only. This is the design we agreed upon.

>
>>   FrameBuffer *CameraStream::getBuffer()
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index cbbe7128..9d7523cb 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -98,7 +98,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>>          }
>>   }
>>
>> -int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
>> +void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
>>   {
>>          ASSERT(encoder_);
>>
>> @@ -199,7 +199,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>>          if (jpeg_size < 0) {
>>                  LOG(JPEG, Error) << "Failed to encode stream image";
>>                  processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>> -               return jpeg_size;
>> +               return;
>>          }
>>
>>          /* Fill in the JPEG blob header. */
>> @@ -213,6 +213,4 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>>          /* Update the JPEG result Metadata. */
>>          resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
>>          processComplete.emit(streamBuffer, PostProcessor::Status::Success);
>> -
>> -       return 0;
>>   }
>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> index 92385548..43fcbe60 100644
>> --- a/src/android/jpeg/post_processor_jpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -22,7 +22,7 @@ public:
>>
>>          int configure(const libcamera::StreamConfiguration &incfg,
>>                        const libcamera::StreamConfiguration &outcfg) override;
>> -       int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
>> +       void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
>>
>>   private:
>>          void generateThumbnail(const libcamera::FrameBuffer &source,
>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
>> index 4ac74fcf..5ec71c93 100644
>> --- a/src/android/post_processor.h
>> +++ b/src/android/post_processor.h
>> @@ -27,7 +27,7 @@ public:
>>
>>          virtual int configure(const libcamera::StreamConfiguration &inCfg,
>>                                const libcamera::StreamConfiguration &outCfg) = 0;
>> -       virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
>> +       virtual void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
>>
>>          libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
>>   };
>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>> index 8e77bf57..1ac7995a 100644
>> --- a/src/android/yuv/post_processor_yuv.cpp
>> +++ b/src/android/yuv/post_processor_yuv.cpp
>> @@ -49,21 +49,21 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,
>>          return 0;
>>   }
>>
>> -int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
>> +void PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
>>   {
>>          const FrameBuffer &source = *streamBuffer->srcBuffer;
>>          CameraBuffer *destination = streamBuffer->destBuffer.get();
>>
>>          if (!isValidBuffers(source, *destination)) {
>>                  processComplete.emit(streamBuffer, 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(streamBuffer, PostProcessor::Status::Error);
>> -               return -EINVAL;
>> +               return;
>>          }
>>
>>          int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(),
>> @@ -81,12 +81,10 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
>>          if (ret) {
>>                  LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
>>                  processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>> -               return -EINVAL;
>> +               return;
>>          }
>>
>>          processComplete.emit(streamBuffer, 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 5954e11b..39ec7994 100644
>> --- a/src/android/yuv/post_processor_yuv.h
>> +++ b/src/android/yuv/post_processor_yuv.h
>> @@ -18,7 +18,7 @@ public:
>>
>>          int configure(const libcamera::StreamConfiguration &incfg,
>>                        const libcamera::StreamConfiguration &outcfg) override;
>> -       int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
>> +       void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
>>
>>   private:
>>          bool isValidBuffers(const libcamera::FrameBuffer &source,
>> --
>> 2.31.1
>>

Patch
diff mbox series 

diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 4e275cde..45d0607d 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -194,7 +194,9 @@  int CameraStream::process(const FrameBuffer &source,
 
 	dest.srcBuffer = &source;
 
-	return postProcessor_->process(&dest);
+	postProcessor_->process(&dest);
+
+	return 0;
 }
 
 FrameBuffer *CameraStream::getBuffer()
diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
index cbbe7128..9d7523cb 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -98,7 +98,7 @@  void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
 	}
 }
 
-int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
+void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
 {
 	ASSERT(encoder_);
 
@@ -199,7 +199,7 @@  int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
 	if (jpeg_size < 0) {
 		LOG(JPEG, Error) << "Failed to encode stream image";
 		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
-		return jpeg_size;
+		return;
 	}
 
 	/* Fill in the JPEG blob header. */
@@ -213,6 +213,4 @@  int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
 	/* Update the JPEG result Metadata. */
 	resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
 	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
-
-	return 0;
 }
diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
index 92385548..43fcbe60 100644
--- a/src/android/jpeg/post_processor_jpeg.h
+++ b/src/android/jpeg/post_processor_jpeg.h
@@ -22,7 +22,7 @@  public:
 
 	int configure(const libcamera::StreamConfiguration &incfg,
 		      const libcamera::StreamConfiguration &outcfg) override;
-	int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
+	void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
 
 private:
 	void generateThumbnail(const libcamera::FrameBuffer &source,
diff --git a/src/android/post_processor.h b/src/android/post_processor.h
index 4ac74fcf..5ec71c93 100644
--- a/src/android/post_processor.h
+++ b/src/android/post_processor.h
@@ -27,7 +27,7 @@  public:
 
 	virtual int configure(const libcamera::StreamConfiguration &inCfg,
 			      const libcamera::StreamConfiguration &outCfg) = 0;
-	virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
+	virtual void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
 
 	libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
 };
diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
index 8e77bf57..1ac7995a 100644
--- a/src/android/yuv/post_processor_yuv.cpp
+++ b/src/android/yuv/post_processor_yuv.cpp
@@ -49,21 +49,21 @@  int PostProcessorYuv::configure(const StreamConfiguration &inCfg,
 	return 0;
 }
 
-int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
+void PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
 {
 	const FrameBuffer &source = *streamBuffer->srcBuffer;
 	CameraBuffer *destination = streamBuffer->destBuffer.get();
 
 	if (!isValidBuffers(source, *destination)) {
 		processComplete.emit(streamBuffer, 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(streamBuffer, PostProcessor::Status::Error);
-		return -EINVAL;
+		return;
 	}
 
 	int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(),
@@ -81,12 +81,10 @@  int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
 	if (ret) {
 		LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
 		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
-		return -EINVAL;
+		return;
 	}
 
 	processComplete.emit(streamBuffer, 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 5954e11b..39ec7994 100644
--- a/src/android/yuv/post_processor_yuv.h
+++ b/src/android/yuv/post_processor_yuv.h
@@ -18,7 +18,7 @@  public:
 
 	int configure(const libcamera::StreamConfiguration &incfg,
 		      const libcamera::StreamConfiguration &outcfg) override;
-	int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
+	void process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
 
 private:
 	bool isValidBuffers(const libcamera::FrameBuffer &source,