[libcamera-devel] [PATCH 2/3] android: jpeg: encoder_libjpeg: Allow encoding raw frame bytes

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Mon, Oct 26, 2020 at 07:31:33PM +0530, Umang Jain wrote:
> Allow encoding frames which are directly handed over to the encoder
> via a span or vector i.e. a raw frame bytes. Introduce an overloaded
> EncoderLibJpeg::encode() with libcamera::Span source parameter to
> achieve this functionality. This makes the libjpeg-encoder a bit
> flexible for use case such as compressing a thumbnail generated for
> Exif.
> 
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/android/jpeg/encoder_libjpeg.cpp | 18 ++++++++++++------
>  src/android/jpeg/encoder_libjpeg.h   |  7 +++++--
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index cfa5332..129ca27 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -104,9 +104,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>  	return 0;
>  }
>  
> -void EncoderLibJpeg::compressRGB(const MappedBuffer *frame)
> +void EncoderLibJpeg::compressRGB(Span<uint8_t> frame)

Can you make this a Span<const uint8_t> to express that the function
doesn't modify the contents of the frame ? It will likely require a few
changes within the function, making local variables const, and/or using
const_cast<> to cast away the const as some libjpeg functions take a
non-const pointer to data they never modify (an unfortunate API choice,
but that's the way it is).

>  {
> -	unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
> +	unsigned char *src = frame.data();
>  	/* \todo Stride information should come from buffer configuration. */
>  	unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);
>  
> @@ -122,7 +122,7 @@ void EncoderLibJpeg::compressRGB(const MappedBuffer *frame)
>   * Compress the incoming buffer from a supported NV format.
>   * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.
>   */
> -void EncoderLibJpeg::compressNV(const MappedBuffer *frame)
> +void EncoderLibJpeg::compressNV(Span<uint8_t> frame)

Same here.

>  {
>  	uint8_t tmprowbuf[compress_.image_width * 3];
>  
> @@ -144,7 +144,7 @@ void EncoderLibJpeg::compressNV(const MappedBuffer *frame)
>  	unsigned int cb_pos = nvSwap_ ? 1 : 0;
>  	unsigned int cr_pos = nvSwap_ ? 0 : 1;
>  
> -	const unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
> +	const unsigned char *src = frame.data();
>  	const unsigned char *src_c = src + y_stride * compress_.image_height;
>  
>  	JSAMPROW row_pointer[1];
> @@ -189,6 +189,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
>  		return frame.error();
>  	}
>  
> +	return encode(frame.maps()[0], dest, exifData);
> +}
> +
> +int EncoderLibJpeg::encode(Span<uint8_t> src, Span<uint8_t> dest,
> +			   Span<const uint8_t> exifData)

And same here for src (dest obviously needs to be mutable :-)).

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +{
>  	unsigned char *destination = dest.data();
>  	unsigned long size = dest.size();
>  
> @@ -214,9 +220,9 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
>  			 << "x" << compress_.image_height;
>  
>  	if (nv_)
> -		compressNV(&frame);
> +		compressNV(src);
>  	else
> -		compressRGB(&frame);
> +		compressRGB(src);
>  
>  	jpeg_finish_compress(&compress_);
>  
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 40505dd..06f884b 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -24,10 +24,13 @@ public:
>  	int encode(const libcamera::FrameBuffer &source,
>  		   libcamera::Span<uint8_t> destination,
>  		   libcamera::Span<const uint8_t> exifData) override;
> +	int encode(libcamera::Span<uint8_t> source,
> +		   libcamera::Span<uint8_t> destination,
> +		   libcamera::Span<const uint8_t> exifData);
>  
>  private:
> -	void compressRGB(const libcamera::MappedBuffer *frame);
> -	void compressNV(const libcamera::MappedBuffer *frame);
> +	void compressRGB(libcamera::Span<uint8_t> frame);
> +	void compressNV(libcamera::Span<uint8_t> frame);
>  
>  	struct jpeg_compress_struct compress_;
>  	struct jpeg_error_mgr jerr_;

-- 
Regards,

Laurent Pinchart