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

[Hirokazu Honda]

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 at 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 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