[libcamera-devel] [PATCH v3 1/4] android: Modify PostProcessor interface

[Hirokazu Honda]

On Thu, Oct 22, 2020 at 6:27 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Umang, / Hiro,
>
> On 22/10/2020 08:46, Umang Jain wrote:
> > Hi  all,
> >
> > I have re-reviewed the series and changes look good to me.
> >
> > Patches 3/4 and 4/4 will require changing const pointer style to use
> > prefix-ed const. Apart from that, I don't think anything is blocking for
> > merging.
>
> Thanks, but these patches went in yesterday. And now I take a second
> look, I am more dubious about the const conditions on some of those entries.
>
> In locations where I think it looks like the intent was to stop the
> object being modified, indeed, the const declarations are only stopping
> the 'pointer' from being modified...
>
> Hiro - Could you take another look please and evaluate if the code
> matches your intentions?
>

I checked the patches.
Yes, they were what I intended. i.e. a const pointer to a class, e.g.,
CameraDevice *const CameraDevice.
For instance, A class "X" is constructed with being given a class
pointer of "Z" but doesn't have the ownership of Z.
The X unlikely changes the pointer "Z" but should still be able to
modify the state of "Z".
Does it make sense?

Best Regards,
-Hiro

> I find this to be a really useful tool in translating C sometimes ;-)
>
>         https://cdecl.org/
>
> For example:
>
>   const class CameraDevice *cameraDevice_;
>   > declare cameraDevice_ as pointer to const class CameraDevice
>
>   (https://cdecl.org/?q=const+class+CameraDevice+*cameraDevice_%3B)
>
> vs
>
>   class CameraDevice *const cameraDevice_;
>   > declare cameraDevice_ as const pointer to class CameraDevice
>
>   (https://cdecl.org/?q=class+CameraDevice+*const+cameraDevice_%3B+)
>
> Which while likely a true statement, stating that we can't modify the
> pointer isn't as protective as stating that we can't modify the class
> instance itself.
>
> --
> Regards
>
> Kieran
>
>
> >
> > Thanks!
> >
> > On 10/21/20 7:09 AM, Hirokazu Honda wrote:
> >> In PostProcessor::process(), the |source| argument doesn't have
> >> to be a pointer. This replaces its type, const pointer, with
> >> const reference as the latter is preferred to the former.
> >> libcamera::Span is cheap to construct/copy/move. We should deal
> >> with the type as pass-by-value parameter. Therefore this also
> >> drops the const reference in the |destination| argument.
> >>
> >> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> >> Reviewed-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/camera_stream.cpp            | 2 +-
> >>   src/android/jpeg/post_processor_jpeg.cpp | 6 +++---
> >>   src/android/jpeg/post_processor_jpeg.h   | 4 ++--
> >>   src/android/post_processor.h             | 4 ++--
> >>   4 files changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/src/android/camera_stream.cpp
> >> b/src/android/camera_stream.cpp
> >> index eae451e..3e5d6be 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -102,7 +102,7 @@ int CameraStream::process(const
> >> libcamera::FrameBuffer &source,
> >>       if (!postProcessor_)
> >>           return 0;
> >>   -    return postProcessor_->process(&source, dest->maps()[0],
> >> metadata);
> >> +    return postProcessor_->process(source, dest->maps()[0], metadata);
> >>   }
> >>     FrameBuffer *CameraStream::getBuffer()
> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> >> b/src/android/jpeg/post_processor_jpeg.cpp
> >> index 9d452b7..90bf10d 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const
> >> StreamConfiguration &inCfg,
> >>       return encoder_->configure(inCfg);
> >>   }
> >>   -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> >> -                   const libcamera::Span<uint8_t> &destination,
> >> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,
> >> +                   libcamera::Span<uint8_t> destination,
> >>                      CameraMetadata *metadata)
> >>   {
> >>       if (!encoder_)
> >> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const
> >> libcamera::FrameBuffer *source,
> >>       if (exif.generate() != 0)
> >>           LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> >>   -    int jpeg_size = encoder_->encode(source, destination,
> >> exif.data());
> >> +    int jpeg_size = encoder_->encode(&source, destination, exif.data());
> >>       if (jpeg_size < 0) {
> >>           LOG(JPEG, Error) << "Failed to encode stream image";
> >>           return jpeg_size;
> >> diff --git a/src/android/jpeg/post_processor_jpeg.h
> >> b/src/android/jpeg/post_processor_jpeg.h
> >> index 62c8650..ae636ff 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.h
> >> +++ b/src/android/jpeg/post_processor_jpeg.h
> >> @@ -23,8 +23,8 @@ public:
> >>         int configure(const libcamera::StreamConfiguration &incfg,
> >>                 const libcamera::StreamConfiguration &outcfg) override;
> >> -    int process(const libcamera::FrameBuffer *source,
> >> -            const libcamera::Span<uint8_t> &destination,
> >> +    int process(const libcamera::FrameBuffer &source,
> >> +            libcamera::Span<uint8_t> destination,
> >>               CameraMetadata *metadata) override;
> >>     private:
> >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> >> index a891c43..5f87a5d 100644
> >> --- a/src/android/post_processor.h
> >> +++ b/src/android/post_processor.h
> >> @@ -20,8 +20,8 @@ public:
> >>         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,
> >> +    virtual int process(const libcamera::FrameBuffer &source,
> >> +                libcamera::Span<uint8_t> destination,
> >>                   CameraMetadata *metadata) = 0;
> >>   };
> >>
> >
>
> --
> Regards
> --
> Kieran