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

[Kieran Bingham]

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