[libcamera-devel] [PATCH v2 1/2] android: post_processor: Introduce a PostProcessor interface

[Umang Jain]

Hi Hiro,

On 10/19/20 11:03 AM, Hirokazu Honda wrote:
> Thanks Umang for this work.
> Thanks Laurent for letting me know.
> I actually recognized these works last Wednesday but haven't had time
> to review them.
>
> The patches are similar to my local WIP patches but nicer than mine.
> I haven't been able to upload them because I was interrupted by other
> works and had some troubles building and testing libcamera on ChromeOS
> environment.
> Anyway, I very much appreciate Umang to take over this work.
>
>
> On Fri, Oct 16, 2020 at 2:38 PM Umang Jain <email at uajain.com> wrote:
>> Introduce a PostProcessor interface for the streams that require any
>> kind of processing (refer to CameraStream::Type) 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 post-processing layer 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>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> ---
>>   src/android/post_processor.h | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 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..a891c43
>> --- /dev/null
>> +++ b/src/android/post_processor.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: LGPL-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 <libcamera/buffer.h>
>> +#include <libcamera/span.h>
>> +#include <libcamera/stream.h>
>> +
>> +class CameraMetadata;
>> +
>> +class PostProcessor
>> +{
>> +public:
>> +       virtual ~PostProcessor() {}
>> +
>> +       virtual int configure(const libcamera::StreamConfiguration &inCfg,
>> +                             const libcamera::StreamConfiguration &outCfg) = 0;
>> +       virtual int process(const libcamera::FrameBuffer *source,
>> +                           const libcamera::Span<uint8_t> &destination,
>> +                           CameraMetadata *metadata) = 0;
>> +};
>> +
> nit: How about
> process(const libcameraFrameBuffer&, libcamera::Span<uint8_t>, CameraMeatadata)?
>
> I think const & is preferred to const* in this interface.
> libcamera::Span looks very cheap to construct/copy/move, so that I
> would like to use it as a pass-by-value parameter.
> cf.) span in chromium
> https://source.chromium.org/chromium/chromium/src/+/master:base/containers/span.h;l=157
>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
The patches have been merged in the master branch, so I think you will 
need to incorporate your suggestions as patches over the latest master. 
As far as I know, we are not 100% strict of the interface yet, so it 
will be completely fine if you want to change it. I just wrote the best 
possible for my use-case and would be interested to see it evolve to 
address further use cases :)

Just FYI -
I am following up with work that might be few changes in the 
libjpeg-encoder (which are mostly internal) - so as it support 
thumbnailing (scale-down) of the image for embedding into exif metadata. 
Hence, I don't think I would be working out any major reworks for 
PostProcessorJpeg.

Thanks for the review.
>
> Regards,
> -Hiro
>
>> +#endif /* __ANDROID_POST_PROCESSOR_H__ */
>> --
>> 2.26.2
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
-
uajain