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

10074 diff mbox series

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@uajain.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/post_processor.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 src/android/post_processor.h

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@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@uajain.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@chromium.org>

Regards,
-Hiro

> +#endif /* __ANDROID_POST_PROCESSOR_H__ */
> --
> 2.26.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@uajain.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
-
uajain

Thank Umang for informing your plan.
I will work to address my comments.

Regards,
-Hiro

On Mon, Oct 19, 2020 at 2:43 PM Umang Jain <email@uajain.com> wrote:
>
> 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@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@uajain.com>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> -
> uajain