[libcamera-devel,v3,20/30] android: jpeg: Support multi-planar buffers
diff mbox series

Message ID 20210906225636.14683-20-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Handle fallout of FrameBuffer offset support
Related show

Commit Message

Laurent Pinchart Sept. 6, 2021, 10:56 p.m. UTC 
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@ideasonboard.com>
Acked-by: Umang Jain <umang.jain@ideasonboard.com>
Tested-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/jpeg/encoder_libjpeg.cpp     | 17 +++++++++--------
 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(+), 14 deletions(-)

Comments

Kieran Bingham Sept. 7, 2021, 10:12 a.m. UTC | #1 
On 06/09/2021 23:56, 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@ideasonboard.com>
> Acked-by: Umang Jain <umang.jain@ideasonboard.com>
> Tested-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/jpeg/encoder_libjpeg.cpp     | 17 +++++++++--------
>  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(+), 14 deletions(-)
> 
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 807a0949a8fc..d0b1da5016d2 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)
>  {

Should we ASSERT(planes.size() >= 2); here? (or even == 2?)

>  	uint8_t tmprowbuf[compress_.image_width * 3];
>  
> @@ -143,8 +143,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 +188,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);
>  

Possibly ASSERT(frame.planes().size() >= 2 here too.

But either way,

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

>  	/* 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;
>  
>
Laurent Pinchart Sept. 7, 2021, 2:16 p.m. UTC | #2 
Hi Kieran,

On Tue, Sep 07, 2021 at 11:12:18AM +0100, Kieran Bingham wrote:
> On 06/09/2021 23:56, 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@ideasonboard.com>
> > Acked-by: Umang Jain <umang.jain@ideasonboard.com>
> > Tested-by: Umang Jain <umang.jain@ideasonboard.com>
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/jpeg/encoder_libjpeg.cpp     | 17 +++++++++--------
> >  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(+), 14 deletions(-)
> > 
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> > index 807a0949a8fc..d0b1da5016d2 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)
> >  {
> 
> Should we ASSERT(planes.size() >= 2); here? (or even == 2?)

I'll add

	ASSERT(src.size() == pixelFormatInfo_->numPlanes());

in EncoderLibJpeg::encode() instead, that will cover the RGB case too.

> >  	uint8_t tmprowbuf[compress_.image_width * 3];
> >  
> > @@ -143,8 +143,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 +188,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);
> >  
> 
> Possibly ASSERT(frame.planes().size() >= 2 here too.
> 
> But either way,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >  	/* 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;
> >

Patch
diff mbox series 

diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
index 807a0949a8fc..d0b1da5016d2 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];
 
@@ -143,8 +143,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 +188,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;