[libcamera-devel] [PATCH v2 28/27] android: jpeg: Support multi-planar buffers

[Umang Jain]

Hi Laurent,

On 9/6/21 7:10 PM, Laurent Pinchart wrote:
> The JPEG post-processor uses MappedFrameBuffer to access pixel data, but
> only uses data from the first plane. Pass the vector of planes to the
> encode() function to correctly handle multi-planar formats (currently
> limited to NV12).
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


The diff looks good to me but I haven't the entire series yet. Also, 
this fixes the segfault observed with the series while trying to click a 
picture so,

Acked-by: Umang Jain <umang.jain at ideasonboard.com>

Tested-by: Umang Jain <umang.jain at ideasonboard.com>

> ---
>   src/android/jpeg/encoder_libjpeg.cpp     | 18 +++++++++---------
>   src/android/jpeg/encoder_libjpeg.h       |  8 +++++---
>   src/android/jpeg/post_processor_jpeg.cpp |  2 +-
>   src/android/jpeg/thumbnailer.cpp         |  4 ++--
>   4 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index b8b01e20a001..7f63f36e793e 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)
>   	return 0;
>   }
>   
> -void EncoderLibJpeg::compressRGB(Span<const uint8_t> frame)
> +void EncoderLibJpeg::compressRGB(const std::vector<Span<uint8_t>> &planes)
>   {
> -	unsigned char *src = const_cast<unsigned char *>(frame.data());
> +	unsigned char *src = const_cast<unsigned char *>(planes[0].data());
>   	/* \todo Stride information should come from buffer configuration. */
>   	unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);
>   
> @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span<const uint8_t> 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(Span<const uint8_t> frame)
> +void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
>   {
>   	uint8_t tmprowbuf[compress_.image_width * 3];
>   
> @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame)
>   	 * Possible hints at:
>   	 * https://sourceforge.net/p/libjpeg/mailman/message/30815123/
>   	 */
> -	unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0);
>   	unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1);
>   
>   	unsigned int horzSubSample = 2 * compress_.image_width / c_stride;
> @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame)
>   	unsigned int cb_pos = nvSwap_ ? 1 : 0;
>   	unsigned int cr_pos = nvSwap_ ? 0 : 1;
>   
> -	const unsigned char *src = frame.data();
> -	const unsigned char *src_c = src + y_stride * compress_.image_height;
> +	const unsigned char *src = planes[0].data();
> +	const unsigned char *src_c = planes[1].data();
>   
>   	JSAMPROW row_pointer[1];
>   	row_pointer[0] = &tmprowbuf[0];
> @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
>   		return frame.error();
>   	}
>   
> -	return encode(frame.planes()[0], dest, exifData, quality);
> +	return encode(frame.planes(), dest, exifData, quality);
>   }
>   
> -int EncoderLibJpeg::encode(Span<const uint8_t> src, Span<uint8_t> dest,
> -			   Span<const uint8_t> exifData, unsigned int quality)
> +int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src,
> +			   Span<uint8_t> dest, Span<const uint8_t> exifData,
> +			   unsigned int quality)
>   {
>   	unsigned char *destination = dest.data();
>   	unsigned long size = dest.size();
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 61fbd1a69278..45ffbd7fae5d 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -9,6 +9,8 @@
>   
>   #include "encoder.h"
>   
> +#include <vector>
> +
>   #include "libcamera/internal/formats.h"
>   
>   #include <jpeglib.h>
> @@ -24,14 +26,14 @@ public:
>   		   libcamera::Span<uint8_t> destination,
>   		   libcamera::Span<const uint8_t> exifData,
>   		   unsigned int quality) override;
> -	int encode(libcamera::Span<const uint8_t> source,
> +	int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
>   		   libcamera::Span<uint8_t> destination,
>   		   libcamera::Span<const uint8_t> exifData,
>   		   unsigned int quality);
>   
>   private:
> -	void compressRGB(libcamera::Span<const uint8_t> frame);
> -	void compressNV(libcamera::Span<const uint8_t> frame);
> +	void compressRGB(const std::vector<libcamera::Span<uint8_t>> &planes);
> +	void compressNV(const std::vector<libcamera::Span<uint8_t>> &planes);
>   
>   	struct jpeg_compress_struct compress_;
>   	struct jpeg_error_mgr jerr_;
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 3160a784419c..68d74842925d 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>   		 */
>   		thumbnail->resize(rawThumbnail.size());
>   
> -		int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,
> +		int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
>   							 *thumbnail, {}, quality);
>   		thumbnail->resize(jpeg_size);
>   
> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> index 043c7b33f06a..2c341535de9e 100644
> --- a/src/android/jpeg/thumbnailer.cpp
> +++ b/src/android/jpeg/thumbnailer.cpp
> @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,
>   	ASSERT(tw % 2 == 0 && th % 2 == 0);
>   
>   	/* Image scaling block implementing nearest-neighbour algorithm. */
> -	unsigned char *src = static_cast<unsigned char *>(frame.planes()[0].data());
> -	unsigned char *srcC = src + sh * sw;
> +	unsigned char *src = frame.planes()[0].data();
> +	unsigned char *srcC = frame.planes()[1].data();
>   	unsigned char *srcCb, *srcCr;
>   	unsigned char *dstY, *srcY;
>