[libcamera-devel] [PATCH v2 2/4] android: Modify Encoder interface

[Kieran Bingham]

Hi Hiro,

On 20/10/2020 11:38, Umang Jain wrote:
> Hi Hiro,
> 
> Thanks for your work.
> 
> On 10/20/20 1:12 PM, Hirokazu Honda wrote:
>> In Encoder::encode(), 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 chea to construct/copy/move. We should deal
> s/chea/cheap

Hrm, this is fine in the original. Must have caught it while editing.

>> 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>
> LGTM again.
> Reviewed-by: Umang Jain <email at uajain.com>

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

>> ---
>>   src/android/jpeg/encoder.h               | 4 ++--
>>   src/android/jpeg/encoder_libjpeg.cpp     | 6 +++---
>>   src/android/jpeg/encoder_libjpeg.h       | 4 ++--
>>   src/android/jpeg/post_processor_jpeg.cpp | 2 +-
>>   4 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
>> index 4483153..4fe71cf 100644
>> --- a/src/android/jpeg/encoder.h
>> +++ b/src/android/jpeg/encoder.h
>> @@ -17,8 +17,8 @@ public:
>>       virtual ~Encoder() {};
>>         virtual int configure(const libcamera::StreamConfiguration
>> &cfg) = 0;
>> -    virtual int encode(const libcamera::FrameBuffer *source,
>> -               const libcamera::Span<uint8_t> &destination,
>> +    virtual int encode(const libcamera::FrameBuffer &source,
>> +               libcamera::Span<uint8_t> destination,
>>                  const libcamera::Span<const uint8_t> &exifData) = 0;
>>   };
>>   diff --git a/src/android/jpeg/encoder_libjpeg.cpp
>> b/src/android/jpeg/encoder_libjpeg.cpp
>> index 8995ba5..4236c9d 100644
>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>> +++ b/src/android/jpeg/encoder_libjpeg.cpp
>> @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const
>> libcamera::MappedBuffer *frame)
>>       }
>>   }
>>   -int EncoderLibJpeg::encode(const FrameBuffer *source,
>> -               const libcamera::Span<uint8_t> &dest,
>> +int EncoderLibJpeg::encode(const FrameBuffer &source,
>> +               libcamera::Span<uint8_t> dest,
>>                  const libcamera::Span<const uint8_t> &exifData)
>>   {
>> -    MappedFrameBuffer frame(source, PROT_READ);
>> +    MappedFrameBuffer frame(&source, PROT_READ);

Perhaps MappedFrameBuffer should also take a reference, but lets not
worry about that here.

>>       if (!frame.isValid()) {
>>           LOG(JPEG, Error) << "Failed to map FrameBuffer : "
>>                    << strerror(frame.error());
>> diff --git a/src/android/jpeg/encoder_libjpeg.h
>> b/src/android/jpeg/encoder_libjpeg.h
>> index 934caef..391a53c 100644
>> --- a/src/android/jpeg/encoder_libjpeg.h
>> +++ b/src/android/jpeg/encoder_libjpeg.h
>> @@ -21,8 +21,8 @@ public:
>>       ~EncoderLibJpeg();
>>         int configure(const libcamera::StreamConfiguration &cfg)
>> override;
>> -    int encode(const libcamera::FrameBuffer *source,
>> -           const libcamera::Span<uint8_t> &destination,
>> +    int encode(const libcamera::FrameBuffer &source,
>> +           libcamera::Span<uint8_t> destination,
>>              const libcamera::Span<const uint8_t> &exifData) override;
>>     private:
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp
>> b/src/android/jpeg/post_processor_jpeg.cpp
>> index 55b9088..6f33631 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -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());
> Perfect. I was expecting this from v1.
>>       if (jpeg_size < 0) {
>>           LOG(JPEG, Error) << "Failed to encode stream image";
>>           return jpeg_size;
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran