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

[Hirokazu Honda]

On Thu, Oct 22, 2020 at 8:59 PM Umang Jain <email at uajain.com> wrote:
>
> Hi Hiro / Kieran,
>
> On 10/22/20 4:28 PM, Hirokazu Honda wrote:
> > 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.
> I think I would digress here. I think what we needed here from the start
> was: `const X *p`  i.e. pointer to an X that is constant.
> https://isocpp.org/wiki/faq/const-correctness#ptr-to-const
> > 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?
>
> I don't think we need the ability to modify "Z", in this case (patch 3/4
> and 4/4), Z is the cameraDevice_.  looking at both CameraStream and
> PostProcessorJpeg on how they use CameraDevice API, one can make out the
> both (CameraStream and PostProcessor) does not _intent_ to change the
> state of cameraDevice_ (you can also double-check via how all getters in
> camera_device.h are declared `const`). Does this reasoning makes sense?
>

I am fine to change it to so. It is aligned with the current usage.

Best Regards,
-Hiro
> --
> uajain
>
>
> >
> > 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
>