[libcamera-devel,2/3] android: jpeg: encoder_libjpeg: Allow encoding raw frame bytes
diff mbox series

Message ID 20201026140134.44166-3-email@uajain.com
State Accepted
Headers show
Series
  • android: jpeg: exif: Embed a JPEG-encoded thumbnail
Related show

Commit Message

Umang Jain Oct. 26, 2020, 2:01 p.m. UTC 
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@uajain.com>
---
 src/android/jpeg/encoder_libjpeg.cpp | 18 ++++++++++++------
 src/android/jpeg/encoder_libjpeg.h   |  7 +++++--
 2 files changed, 17 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart Oct. 26, 2020, 6:48 p.m. UTC | #1 
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@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@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_;
Hirokazu Honda Oct. 27, 2020, 4:34 a.m. UTC | #2 
On Tue, Oct 27, 2020 at 3:49 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@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@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);
> >

The commit message says this is an overloaded function.
But it isn't actually. Because of that, this function needs to be
invoked with EncoderLibJpeg in PostProcessorJpeg.
If I remember correctly, we'all agree to expanding FrameBuffer for
dmabuf in the future.
So the interface with FrameBuffer should be a solid single overloaded function.
I think we would rather
1.) makes the new function a specific function (which is done by this
patch actually),
2.) Use the FrameBuffer interface always.
We may want to think of the tradeoff between introducing the specific
function and how simple the code becomes thanks to it.

Best Regards,
-Hiro



> >  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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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)
 {
-	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)
 {
 	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)
+{
 	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_;