[libcamera-devel] [RFC PATCH 1/3] android: post_processor: Introduce a PostProcessor interface

[Kieran Bingham]

Hi Laurent,

On 08/10/2020 20:10, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Thu, Oct 08, 2020 at 07:40:36PM +0530, Umang Jain wrote:
>> Introduce a PostProcessor interface for the streams that require any
>> kind of processing for their consumption by the HAL layer. The
>> PostProcessor interface can be configured via configure() and the actual
>> processing can be initiated using process().
>>
>> The interface is similar to the Encoder interface. The PostProcessor is
>> meant to replace the Encoder interface and introduce a more generic
>> post-processing layer which can be extended to have multiple post
>> processors for various stream configurations. As of now, we only have
>> one post processor (JPEG), hence the subsequent commit will port its
>> function to this interface.
>>
>> Signed-off-by: Umang Jain <email at uajain.com>
>> ---
>>  src/android/post_processor.h | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 src/android/post_processor.h
>>
>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
>> new file mode 100644
>> index 0000000..fa676c9
>> --- /dev/null
>> +++ b/src/android/post_processor.h
>> @@ -0,0 +1,30 @@
>> +/* SPDX-License-Identifier: GPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * post_processor.h - CameraStream Post Processing Interface
>> + */
>> +#ifndef __ANDROID_POST_PROCESSOR_H__
>> +#define __ANDROID_POST_PROCESSOR_H__
>> +
>> +#include <stdarg.h>
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/span.h>
>> +#include <libcamera/stream.h>
>> +
>> +class CameraMetadata;
>> +
>> +class PostProcessor
>> +{
>> +public:
>> +	virtual ~PostProcessor() {};
> 
> No need for a semicolumn at the end of the line.
> 
>> +
>> +	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
>> +	virtual int process(const libcamera::FrameBuffer *source,
> 
> If we have multiple post-processors that run on the same source frame
> buffer, it would be more efficient to map the frame buffer in the caller
> and passed a mapped frame buffer instead. On the other hand, some
> post-processor may be hardware-based, and creating a CPU mapping would
> then be a waste. I think we can keep the API as-is for now, but it will
> likely need to be reworked.

Originally I had wanted to keep the mapping within the FrameBuffer - but
everyone seemed to dislike that.

Keeping the mapping with the FrameBuffer (and having the required
support there) would mean that a mapping could be done once, on the
first instance it was needed, or if it was not needed, then not at all.
And it would then be unmapped when the FrameBuffer was released.

If the FrameBuffer is re-used... the mapping persists, and no need to
unmap/remap.

...

Kieran



> 
>> +			    const libcamera::Span<uint8_t> &destination,
>> +			    CameraMetadata *metadata,
>> +			    ...) = 0;
> 
> Variadic arguments are problematic if you want this interface to be
> generic. The whole point of this base class is to offer an API to users
> that doesn't require handling any implementation-specific detail. If the
> caller needs to know what extra arguments to pass to a particular
> post-processor, it defeats the point completely.
> 
> I'll comment more about this on the next patch, but I think you can
> simply drop the variadic arguments.
> 
>> +};
>> +
>> +#endif /* __ANDROID_POST_PROCESSOR_H__ */
> 

-- 
Regards
--
Kieran