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

[Laurent Pinchart]

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.

> +			    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,

Laurent Pinchart