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

[Kieran Bingham]

Hi Umang,

On 21/10/2020 09:08, Umang Jain wrote:
> Allow encoding frame 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.
> 

This looks good to me, It changes the internals of this to support only
single-plane formats, but we only support single-plane formats so I'm
not worried by that.

It also gives the compressRGB, compressNV12 better input parameters in
my opinion, as it's clear what data they are operating on. In other
words, I like that they deal with span internally, rather than operating
on a MappedBuffer type. Particularly as the MappedBuffer topics will
likely need more thought later.

> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/android/jpeg/encoder_libjpeg.cpp | 37 +++++++++++++++++-----------
>  src/android/jpeg/encoder_libjpeg.h   |  7 ++++--
>  2 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index f11e004..bfaf9d5 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 libcamera::MappedBuffer *frame)
> +void EncoderLibJpeg::compressRGB(const libcamera::Span<uint8_t> &frame)
>  {
> -	unsigned char *src = static_cast<unsigned char *>(frame->maps()[0].data());
> +	unsigned char *src = static_cast<unsigned char *>(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 libcamera::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 libcamera::MappedBuffer *frame)
> +void EncoderLibJpeg::compressNV(const libcamera::Span<uint8_t> &frame)
>  {
>  	uint8_t tmprowbuf[compress_.image_width * 3];
>  
> @@ -144,7 +144,7 @@ void EncoderLibJpeg::compressNV(const libcamera::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 = static_cast<unsigned char *>(frame.data());
>  	const unsigned char *src_c = src + y_stride * compress_.image_height;
>  
>  	JSAMPROW row_pointer[1];
> @@ -179,17 +179,10 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>  	}
>  }
>  
> -int EncoderLibJpeg::encode(const FrameBuffer *source,
> +int EncoderLibJpeg::encode(const libcamera::Span<uint8_t> &frame,
>  			   const libcamera::Span<uint8_t> &dest,
>  			   const libcamera::Span<const uint8_t> &exifData)
>  {
> -	MappedFrameBuffer frame(source, PROT_READ);
> -	if (!frame.isValid()) {
> -		LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> -				 << strerror(frame.error());
> -		return frame.error();
> -	}
> -
>  	unsigned char *destination = dest.data();
>  	unsigned long size = dest.size();
>  
> @@ -215,11 +208,27 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
>  			 << "x" << compress_.image_height;
>  
>  	if (nv_)
> -		compressNV(&frame);
> +		compressNV(frame);
>  	else
> -		compressRGB(&frame);
> +		compressRGB(frame);
>  
>  	jpeg_finish_compress(&compress_);
>  
>  	return size;
>  }
> +
> +int EncoderLibJpeg::encode(const FrameBuffer *source,
> +			   const libcamera::Span<uint8_t> &dest,
> +			   const libcamera::Span<const uint8_t> &exifData)
> +{
> +	MappedFrameBuffer frame(source, PROT_READ);
> +	if (!frame.isValid()) {
> +		LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> +				 << strerror(frame.error());
> +		return frame.error();
> +	}
> +
> +	libcamera::Span<uint8_t> src = frame.maps()[0];
> +
> +	return encode(src, dest, exifData);

This could also be:
	return encode(frame.maps()[0], dest, exifData);

I don't think there's a need to declare a new span for this, but it's
fine if you prefer it / find it more readable.

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

> +}
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 934caef..bf2e512 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -21,13 +21,16 @@ public:
>  	~EncoderLibJpeg();
>  
>  	int configure(const libcamera::StreamConfiguration &cfg) override;
> +	int encode(const libcamera::Span<uint8_t> &frames,
> +		   const libcamera::Span<uint8_t> &destination,
> +		   const libcamera::Span<const uint8_t> &exifData);
>  	int encode(const libcamera::FrameBuffer *source,
>  		   const libcamera::Span<uint8_t> &destination,
>  		   const libcamera::Span<const uint8_t> &exifData) override;
>  
>  private:
> -	void compressRGB(const libcamera::MappedBuffer *frame);
> -	void compressNV(const libcamera::MappedBuffer *frame);
> +	void compressRGB(const libcamera::Span<uint8_t> &frame);
> +	void compressNV(const libcamera::Span<uint8_t> &frame);
>  
>  	struct jpeg_compress_struct compress_;
>  	struct jpeg_error_mgr jerr_;
> 

-- 
Regards
--
Kieran