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

[Kieran Bingham]

Hi Hiro,

On 20/10/2020 11:30, Umang Jain wrote:
> Hi Hiro,
> 
> On 10/20/20 1:12 PM, Hirokazu Honda wrote:
>> In PostProcessor::process(), the |source| argument doesn't have
>> to be a pointer. This replace its type, const pointer, with const

s/replace/replaces/
or
s/This replace/Replace/

>> 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>
> 
> This looks good to me.
> Reviewed-by: Umang Jain <email at uajain.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 1b8afa8..28a6e09 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 753c28e..55b9088 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());
> I see that this change is required to keep the patch compile-able
> because the argument's type has been changed. I would expect this would
> be reworked in next patches while correctly the encoder interface.


Indeed, I see it in the next patch.

This is the right order to do things, as we couldn't change the encoder
interface first, as we couldn't give it a reference.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>>       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;
>>   };
>>   
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran