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

10022 diff mbox series

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@uajain.com>
---
 src/android/post_processor.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 src/android/post_processor.h

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@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__ */

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@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__ */
>

Hi Kieran,

On Fri, Oct 09, 2020 at 09:49:25AM +0100, Kieran Bingham wrote:
> On 08/10/2020 20:10, Laurent Pinchart wrote:
> > 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@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.

I've thought exactly the same thing while writing the previous e-mail.
Bundling the FrameBuffer and the mapping in a single class is a good
idea. I'm still not entirely convinced we should add that featuer to the
FrameBuffer class itself though, albeit if you wanted to convince me it
should be done, you would be one step closer to the goal :-)

Would it make sense (not as part of this series) to refactor the
MappedFrammeBuffer class to bundle the FrameBuffer and the mapping, and
experiment with that in the HAL ?

> >> +			    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__ */