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

[Umang Jain]

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?

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