[libcamera-devel] [PATCH v2 3/9] android: post_processor: Plumb down the process() with context

[Umang Jain]

Hi Laurent,

On 9/13/21 5:31 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Fri, Sep 10, 2021 at 12:36:32PM +0530, Umang Jain wrote:
>> The context required for post processing of a camera request is saved
>> via Camera3RequestDescriptor. The context needs to be plumbed to
>> PostProcessor::process() which should be passed a const pointer to
>> Camera3RequestDescriptor. In subsequent commits, we will prepare
>> the post-processor to run asynchrnoously hence, it will require the
>> context pointer to be emitted back to the upper layers, to identify for
>> which request the post-processing has been completed.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   src/android/camera_device.cpp            | 3 ++-
>>   src/android/camera_stream.cpp            | 6 ++++--
>>   src/android/camera_stream.h              | 5 ++++-
>>   src/android/jpeg/post_processor_jpeg.cpp | 3 ++-
>>   src/android/jpeg/post_processor_jpeg.h   | 3 ++-
>>   src/android/post_processor.h             | 5 ++++-
>>   src/android/yuv/post_processor_yuv.cpp   | 3 ++-
>>   src/android/yuv/post_processor_yuv.h     | 3 ++-
>>   8 files changed, 22 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 0840c056..84549d04 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1150,7 +1150,8 @@ void CameraDevice::requestComplete(Request *request)
>>   
>>   		int ret = cameraStream->process(src, *buffer.buffer,
>>   						descriptor.settings_,
> Now that the request metadata can be accessed through the descriptor
> pointer, could you please drop this argument ?


Since it's const reference and only read down in the function chain, I 
think that would make sense to drop it. I'll double it though if it's 
not meant to be read anywhere.

>
>> -						resultMetadata.get());
>> +						resultMetadata.get(),
>> +						&descriptor);
> I'd move the descriptor parameter before the metadata. I think it would
> also make sense to store the result metadata in
> Camera3RequestDescriptor, but that can go to another patch.


ResultMetadata seems to be populated/modified in the JPEG 
post-processor. It won't make sense to access it through the descriptor 
pointer since it's a const *.

The idea here is to not do any access / modification through the 
descriptor pointer passed (you see I have passed it as const). Which 
ever member (even part of Camera3RequestDescriptor struct) that needs 
modification as such, should be passed explicitly in the process() 
function argument. Does it make sense?


>
>>   		/*
>>   		 * Return the FrameBuffer to the CameraStream now that we're
>>   		 * done processing it.
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 0fed5382..d1c54797 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -101,7 +101,8 @@ int CameraStream::configure()
>>   int CameraStream::process(const FrameBuffer *source,
>>   			  buffer_handle_t camera3Dest,
>>   			  const CameraMetadata &requestMetadata,
>> -			  CameraMetadata *resultMetadata)
>> +			  CameraMetadata *resultMetadata,
>> +			  const Camera3RequestDescriptor *context)
>>   {
>>   	if (!postProcessor_)
>>   		return 0;
>> @@ -118,7 +119,8 @@ int CameraStream::process(const FrameBuffer *source,
>>   		return -EINVAL;
>>   	}
>>   
>> -	return postProcessor_->process(source, &dest, requestMetadata, resultMetadata);
>> +	return postProcessor_->process(source, &dest, requestMetadata,
>> +				       resultMetadata, context);
>>   }
>>   
>>   FrameBuffer *CameraStream::getBuffer()
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index 5c232cb6..d589f699 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -23,6 +23,8 @@ class CameraDevice;
>>   class CameraMetadata;
>>   class PostProcessor;
>>   
>> +struct Camera3RequestDescriptor;
>> +
>>   class CameraStream
>>   {
>>   public:
>> @@ -121,7 +123,8 @@ public:
>>   	int process(const libcamera::FrameBuffer *source,
>>   		    buffer_handle_t camera3Dest,
>>   		    const CameraMetadata &requestMetadata,
>> -		    CameraMetadata *resultMetadata);
>> +		    CameraMetadata *resultMetadata,
>> +		    const Camera3RequestDescriptor *context);
>>   	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 cb45f86b..741eeb21 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -100,7 +100,8 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer *source,
>>   int PostProcessorJpeg::process(const FrameBuffer *source,
>>   			       CameraBuffer *destination,
>>   			       const CameraMetadata &requestMetadata,
>> -			       CameraMetadata *resultMetadata)
>> +			       CameraMetadata *resultMetadata,
>> +			       const Camera3RequestDescriptor *context)
>>   {
>>   	if (!encoder_)
>>   		return 0;
>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> index c4b2e9ef..72c38fdb 100644
>> --- a/src/android/jpeg/post_processor_jpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -25,7 +25,8 @@ public:
>>   	int process(const libcamera::FrameBuffer *source,
>>   		    CameraBuffer *destination,
>>   		    const CameraMetadata &requestMetadata,
>> -		    CameraMetadata *resultMetadata) override;
>> +		    CameraMetadata *resultMetadata,
>> +		    const Camera3RequestDescriptor *context) override;
>>   
>>   private:
>>   	void generateThumbnail(const libcamera::FrameBuffer *source,
>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
>> index 61dfb6d4..1213e7e6 100644
>> --- a/src/android/post_processor.h
>> +++ b/src/android/post_processor.h
>> @@ -14,6 +14,8 @@
>>   
>>   class CameraMetadata;
>>   
>> +struct Camera3RequestDescriptor;
>> +
>>   class PostProcessor
>>   {
>>   public:
>> @@ -24,7 +26,8 @@ public:
>>   	virtual int process(const libcamera::FrameBuffer *source,
>>   			    CameraBuffer *destination,
>>   			    const CameraMetadata &requestMetadata,
>> -			    CameraMetadata *resultMetadata) = 0;
>> +			    CameraMetadata *resultMetadata,
>> +			    const Camera3RequestDescriptor *context) = 0;
>>   };
>>   
>>   #endif /* __ANDROID_POST_PROCESSOR_H__ */
>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>> index 0a874886..13a78abe 100644
>> --- a/src/android/yuv/post_processor_yuv.cpp
>> +++ b/src/android/yuv/post_processor_yuv.cpp
>> @@ -52,7 +52,8 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,
>>   int PostProcessorYuv::process(const FrameBuffer *source,
>>   			      CameraBuffer *destination,
>>   			      [[maybe_unused]] const CameraMetadata &requestMetadata,
>> -			      [[maybe_unused]] CameraMetadata *metadata)
>> +			      [[maybe_unused]] CameraMetadata *metadata,
>> +			      [[maybe_unused]] const Camera3RequestDescriptor *context)
>>   {
>>   	if (!isValidBuffers(source, *destination))
>>   		return -EINVAL;
>> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
>> index 44a04113..38a30f91 100644
>> --- a/src/android/yuv/post_processor_yuv.h
>> +++ b/src/android/yuv/post_processor_yuv.h
>> @@ -23,7 +23,8 @@ public:
>>   	int process(const libcamera::FrameBuffer *source,
>>   		    CameraBuffer *destination,
>>   		    const CameraMetadata &requestMetadata,
>> -		    CameraMetadata *metadata) override;
>> +		    CameraMetadata *metadata,
>> +		    const Camera3RequestDescriptor *context) override;
> I'd name the parameter "request" instead of "context", "context" is more
> vague.
>
> With those changes,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>>   
>>   private:
>>   	bool isValidBuffers(const libcamera::FrameBuffer *source,