[libcamera-devel,v7,5/6] Pass StreamBuffer to Encoder::encoder
diff mbox series

Message ID 20221201092733.2042078-6-chenghaoyang@google.com
State Accepted
Headers show
Series
  • Add CrOS JEA implementation
Related show

Commit Message

Cheng-Hao Yang Dec. 1, 2022, 9:27 a.m. UTC 
From: Harvey Yang <chenghaoyang@chromium.org>

To prepare for the JEA encoder in the following patches, StreamBuffer is
passed to Encoder::encoder, which contains the original FrameBuffer and
Span |destination|.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/android/jpeg/encoder.h               |  5 +++--
 src/android/jpeg/encoder_libjpeg.cpp     | 11 +++++++++++
 src/android/jpeg/encoder_libjpeg.h       |  7 +++++--
 src/android/jpeg/post_processor_jpeg.cpp |  3 +--
 4 files changed, 20 insertions(+), 6 deletions(-)

Comments

Laurent Pinchart Dec. 7, 2022, 4:56 a.m. UTC | #1 
Hi Harvey,

Thank you for the patch.

On Thu, Dec 01, 2022 at 09:27:32AM +0000, Harvey Yang via libcamera-devel wrote:
> From: Harvey Yang <chenghaoyang@chromium.org>
> 
> To prepare for the JEA encoder in the following patches, StreamBuffer is
> passed to Encoder::encoder, which contains the original FrameBuffer and
> Span |destination|.
> 
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  src/android/jpeg/encoder.h               |  5 +++--
>  src/android/jpeg/encoder_libjpeg.cpp     | 11 +++++++++++
>  src/android/jpeg/encoder_libjpeg.h       |  7 +++++--
>  src/android/jpeg/post_processor_jpeg.cpp |  3 +--
>  4 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> index 7dc1ee27..eeff87b1 100644
> --- a/src/android/jpeg/encoder.h
> +++ b/src/android/jpeg/encoder.h
> @@ -12,14 +12,15 @@
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/stream.h>
>  
> +#include "../camera_request.h"
> +
>  class Encoder
>  {
>  public:
>  	virtual ~Encoder() = default;
>  
>  	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> -	virtual int encode(const libcamera::FrameBuffer &source,
> -			   libcamera::Span<uint8_t> destination,
> +	virtual int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
>  			   libcamera::Span<const uint8_t> exifData,
>  			   unsigned int quality) = 0;
>  	virtual int generateThumbnail(
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index cc37fde3..d8d72fbd 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -24,6 +24,8 @@
>  #include "libcamera/internal/formats.h"
>  #include "libcamera/internal/mapped_framebuffer.h"
>  
> +#include "../camera_buffer.h"
> +
>  using namespace libcamera;
>  
>  LOG_DECLARE_CATEGORY(JPEG)
> @@ -192,6 +194,15 @@ void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
>  	}
>  }
>  
> +int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> +			   libcamera::Span<const uint8_t> exifData,
> +			   unsigned int quality)
> +{
> +	return encode(*streamBuffer->srcBuffer,
> +		      streamBuffer->dstBuffer.get()->plane(0), exifData,
> +		      quality);
> +}

Do you need to create a wrapper around the encode() function that takes a
FrameBuffer, or could you merge the two together ? It seems to be called
from here only. The following diff compiles:

diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
index d8d72fbdd6b8..6d7a3cbf586d 100644
--- a/src/android/jpeg/encoder_libjpeg.cpp
+++ b/src/android/jpeg/encoder_libjpeg.cpp
@@ -198,9 +198,19 @@ int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
 			   libcamera::Span<const uint8_t> exifData,
 			   unsigned int quality)
 {
-	return encode(*streamBuffer->srcBuffer,
-		      streamBuffer->dstBuffer.get()->plane(0), exifData,
-		      quality);
+	const FrameBuffer &source = *streamBuffer->srcBuffer;
+	Span<uint8_t> dest = streamBuffer->dstBuffer.get()->plane(0);
+
+	compress_ = &image_data_.compress;
+
+	MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
+	if (!frame.isValid()) {
+		LOG(JPEG, Error) << "Failed to map FrameBuffer : "
+				 << strerror(frame.error());
+		return frame.error();
+	}
+
+	return encode(frame.planes(), dest, exifData, quality);
 }
 
 int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source,
@@ -255,21 +265,6 @@ int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source,
 	return -1;
 }
 
-int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
-			   Span<const uint8_t> exifData, unsigned int quality)
-{
-	compress_ = &image_data_.compress;
-
-	MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
-	if (!frame.isValid()) {
-		LOG(JPEG, Error) << "Failed to map FrameBuffer : "
-				 << strerror(frame.error());
-		return frame.error();
-	}
-
-	return encode(frame.planes(), dest, exifData, quality);
-}
-
 int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src,
 			   Span<uint8_t> dest, Span<const uint8_t> exifData,
 			   unsigned int quality)
diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
index 6e9b65d4fc94..5928837367dc 100644
--- a/src/android/jpeg/encoder_libjpeg.h
+++ b/src/android/jpeg/encoder_libjpeg.h
@@ -43,10 +43,6 @@ private:
 			     libcamera::PixelFormat pixelFormat,
 			     libcamera::Size size);
 
-	int encode(const libcamera::FrameBuffer &source,
-		   libcamera::Span<uint8_t> destination,
-		   libcamera::Span<const uint8_t> exifData,
-		   unsigned int quality);
 	int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
 		   libcamera::Span<uint8_t> destination,
 		   libcamera::Span<const uint8_t> exifData,


With that addressed,

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

> +
>  int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source,
>  				      const libcamera::Size &targetSize,
>  				      unsigned int quality,
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 1ec2ba13..6e9b65d4 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -24,8 +24,7 @@ public:
>  	~EncoderLibJpeg();
>  
>  	int configure(const libcamera::StreamConfiguration &cfg) override;
> -	int encode(const libcamera::FrameBuffer &source,
> -		   libcamera::Span<uint8_t> destination,
> +	int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
>  		   libcamera::Span<const uint8_t> exifData,
>  		   unsigned int quality) override;
>  	int generateThumbnail(
> @@ -44,6 +43,10 @@ private:
>  			     libcamera::PixelFormat pixelFormat,
>  			     libcamera::Size size);
>  
> +	int encode(const libcamera::FrameBuffer &source,
> +		   libcamera::Span<uint8_t> destination,
> +		   libcamera::Span<const uint8_t> exifData,
> +		   unsigned int quality);
>  	int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
>  		   libcamera::Span<uint8_t> destination,
>  		   libcamera::Span<const uint8_t> exifData,
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 60eebb11..10ac4666 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -146,8 +146,7 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
>  	const uint8_t quality = ret ? *entry.data.u8 : 95;
>  	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);
>  
> -	int jpeg_size = encoder_->encode(source, destination->plane(0),
> -					 exif.data(), quality);
> +	int jpeg_size = encoder_->encode(streamBuffer, exif.data(), quality);
>  	if (jpeg_size < 0) {
>  		LOG(JPEG, Error) << "Failed to encode stream image";
>  		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
Cheng-Hao Yang Dec. 14, 2022, 9:35 a.m. UTC | #2 
Thanks Laurent!

Adopted.

On Wed, Dec 7, 2022 at 12:56 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Harvey,
>
> Thank you for the patch.
>
> On Thu, Dec 01, 2022 at 09:27:32AM +0000, Harvey Yang via libcamera-devel
> wrote:
> > From: Harvey Yang <chenghaoyang@chromium.org>
> >
> > To prepare for the JEA encoder in the following patches, StreamBuffer is
> > passed to Encoder::encoder, which contains the original FrameBuffer and
> > Span |destination|.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  src/android/jpeg/encoder.h               |  5 +++--
> >  src/android/jpeg/encoder_libjpeg.cpp     | 11 +++++++++++
> >  src/android/jpeg/encoder_libjpeg.h       |  7 +++++--
> >  src/android/jpeg/post_processor_jpeg.cpp |  3 +--
> >  4 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> > index 7dc1ee27..eeff87b1 100644
> > --- a/src/android/jpeg/encoder.h
> > +++ b/src/android/jpeg/encoder.h
> > @@ -12,14 +12,15 @@
> >  #include <libcamera/framebuffer.h>
> >  #include <libcamera/stream.h>
> >
> > +#include "../camera_request.h"
> > +
> >  class Encoder
> >  {
> >  public:
> >       virtual ~Encoder() = default;
> >
> >       virtual int configure(const libcamera::StreamConfiguration &cfg) =
> 0;
> > -     virtual int encode(const libcamera::FrameBuffer &source,
> > -                        libcamera::Span<uint8_t> destination,
> > +     virtual int encode(Camera3RequestDescriptor::StreamBuffer
> *streamBuffer,
> >                          libcamera::Span<const uint8_t> exifData,
> >                          unsigned int quality) = 0;
> >       virtual int generateThumbnail(
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp
> b/src/android/jpeg/encoder_libjpeg.cpp
> > index cc37fde3..d8d72fbd 100644
> > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > @@ -24,6 +24,8 @@
> >  #include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/mapped_framebuffer.h"
> >
> > +#include "../camera_buffer.h"
> > +
> >  using namespace libcamera;
> >
> >  LOG_DECLARE_CATEGORY(JPEG)
> > @@ -192,6 +194,15 @@ void EncoderLibJpeg::compressNV(const
> std::vector<Span<uint8_t>> &planes)
> >       }
> >  }
> >
> > +int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer
> *streamBuffer,
> > +                        libcamera::Span<const uint8_t> exifData,
> > +                        unsigned int quality)
> > +{
> > +     return encode(*streamBuffer->srcBuffer,
> > +                   streamBuffer->dstBuffer.get()->plane(0), exifData,
> > +                   quality);
> > +}
>
> Do you need to create a wrapper around the encode() function that takes a
> FrameBuffer, or could you merge the two together ? It seems to be called
> from here only. The following diff compiles:
>
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp
> b/src/android/jpeg/encoder_libjpeg.cpp
> index d8d72fbdd6b8..6d7a3cbf586d 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -198,9 +198,19 @@ int
> EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
>                            libcamera::Span<const uint8_t> exifData,
>                            unsigned int quality)
>  {
> -       return encode(*streamBuffer->srcBuffer,
> -                     streamBuffer->dstBuffer.get()->plane(0), exifData,
> -                     quality);
> +       const FrameBuffer &source = *streamBuffer->srcBuffer;
> +       Span<uint8_t> dest = streamBuffer->dstBuffer.get()->plane(0);
> +
> +       compress_ = &image_data_.compress;
> +
> +       MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
> +       if (!frame.isValid()) {
> +               LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> +                                << strerror(frame.error());
> +               return frame.error();
> +       }
> +
> +       return encode(frame.planes(), dest, exifData, quality);
>  }
>
>  int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer
> &source,
> @@ -255,21 +265,6 @@ int EncoderLibJpeg::generateThumbnail(const
> libcamera::FrameBuffer &source,
>         return -1;
>  }
>
> -int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
> -                          Span<const uint8_t> exifData, unsigned int
> quality)
> -{
> -       compress_ = &image_data_.compress;
> -
> -       MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
> -       if (!frame.isValid()) {
> -               LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> -                                << strerror(frame.error());
> -               return frame.error();
> -       }
> -
> -       return encode(frame.planes(), dest, exifData, quality);
> -}
> -
>  int EncoderLibJpeg::encode(const std::vector<Span<uint8_t>> &src,
>                            Span<uint8_t> dest, Span<const uint8_t>
> exifData,
>                            unsigned int quality)
> diff --git a/src/android/jpeg/encoder_libjpeg.h
> b/src/android/jpeg/encoder_libjpeg.h
> index 6e9b65d4fc94..5928837367dc 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -43,10 +43,6 @@ private:
>                              libcamera::PixelFormat pixelFormat,
>                              libcamera::Size size);
>
> -       int encode(const libcamera::FrameBuffer &source,
> -                  libcamera::Span<uint8_t> destination,
> -                  libcamera::Span<const uint8_t> exifData,
> -                  unsigned int quality);
>         int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
>                    libcamera::Span<uint8_t> destination,
>                    libcamera::Span<const uint8_t> exifData,
>
>
> With that addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +
> >  int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer
> &source,
> >                                     const libcamera::Size &targetSize,
> >                                     unsigned int quality,
> > diff --git a/src/android/jpeg/encoder_libjpeg.h
> b/src/android/jpeg/encoder_libjpeg.h
> > index 1ec2ba13..6e9b65d4 100644
> > --- a/src/android/jpeg/encoder_libjpeg.h
> > +++ b/src/android/jpeg/encoder_libjpeg.h
> > @@ -24,8 +24,7 @@ public:
> >       ~EncoderLibJpeg();
> >
> >       int configure(const libcamera::StreamConfiguration &cfg) override;
> > -     int encode(const libcamera::FrameBuffer &source,
> > -                libcamera::Span<uint8_t> destination,
> > +     int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> >                  libcamera::Span<const uint8_t> exifData,
> >                  unsigned int quality) override;
> >       int generateThumbnail(
> > @@ -44,6 +43,10 @@ private:
> >                            libcamera::PixelFormat pixelFormat,
> >                            libcamera::Size size);
> >
> > +     int encode(const libcamera::FrameBuffer &source,
> > +                libcamera::Span<uint8_t> destination,
> > +                libcamera::Span<const uint8_t> exifData,
> > +                unsigned int quality);
> >       int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
> >                  libcamera::Span<uint8_t> destination,
> >                  libcamera::Span<const uint8_t> exifData,
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> b/src/android/jpeg/post_processor_jpeg.cpp
> > index 60eebb11..10ac4666 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -146,8 +146,7 @@ void
> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
> >       const uint8_t quality = ret ? *entry.data.u8 : 95;
> >       resultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);
> >
> > -     int jpeg_size = encoder_->encode(source, destination->plane(0),
> > -                                      exif.data(), quality);
> > +     int jpeg_size = encoder_->encode(streamBuffer, exif.data(),
> quality);
> >       if (jpeg_size < 0) {
> >               LOG(JPEG, Error) << "Failed to encode stream image";
> >               processComplete.emit(streamBuffer,
> PostProcessor::Status::Error);
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
index 7dc1ee27..eeff87b1 100644
--- a/src/android/jpeg/encoder.h
+++ b/src/android/jpeg/encoder.h
@@ -12,14 +12,15 @@ 
 #include <libcamera/framebuffer.h>
 #include <libcamera/stream.h>
 
+#include "../camera_request.h"
+
 class Encoder
 {
 public:
 	virtual ~Encoder() = default;
 
 	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
-	virtual int encode(const libcamera::FrameBuffer &source,
-			   libcamera::Span<uint8_t> destination,
+	virtual int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
 			   libcamera::Span<const uint8_t> exifData,
 			   unsigned int quality) = 0;
 	virtual int generateThumbnail(
diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
index cc37fde3..d8d72fbd 100644
--- a/src/android/jpeg/encoder_libjpeg.cpp
+++ b/src/android/jpeg/encoder_libjpeg.cpp
@@ -24,6 +24,8 @@ 
 #include "libcamera/internal/formats.h"
 #include "libcamera/internal/mapped_framebuffer.h"
 
+#include "../camera_buffer.h"
+
 using namespace libcamera;
 
 LOG_DECLARE_CATEGORY(JPEG)
@@ -192,6 +194,15 @@  void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
 	}
 }
 
+int EncoderLibJpeg::encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
+			   libcamera::Span<const uint8_t> exifData,
+			   unsigned int quality)
+{
+	return encode(*streamBuffer->srcBuffer,
+		      streamBuffer->dstBuffer.get()->plane(0), exifData,
+		      quality);
+}
+
 int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source,
 				      const libcamera::Size &targetSize,
 				      unsigned int quality,
diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
index 1ec2ba13..6e9b65d4 100644
--- a/src/android/jpeg/encoder_libjpeg.h
+++ b/src/android/jpeg/encoder_libjpeg.h
@@ -24,8 +24,7 @@  public:
 	~EncoderLibJpeg();
 
 	int configure(const libcamera::StreamConfiguration &cfg) override;
-	int encode(const libcamera::FrameBuffer &source,
-		   libcamera::Span<uint8_t> destination,
+	int encode(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
 		   libcamera::Span<const uint8_t> exifData,
 		   unsigned int quality) override;
 	int generateThumbnail(
@@ -44,6 +43,10 @@  private:
 			     libcamera::PixelFormat pixelFormat,
 			     libcamera::Size size);
 
+	int encode(const libcamera::FrameBuffer &source,
+		   libcamera::Span<uint8_t> destination,
+		   libcamera::Span<const uint8_t> exifData,
+		   unsigned int quality);
 	int encode(const std::vector<libcamera::Span<uint8_t>> &planes,
 		   libcamera::Span<uint8_t> destination,
 		   libcamera::Span<const uint8_t> exifData,
diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
index 60eebb11..10ac4666 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -146,8 +146,7 @@  void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
 	const uint8_t quality = ret ? *entry.data.u8 : 95;
 	resultMetadata->addEntry(ANDROID_JPEG_QUALITY, quality);
 
-	int jpeg_size = encoder_->encode(source, destination->plane(0),
-					 exif.data(), quality);
+	int jpeg_size = encoder_->encode(streamBuffer, exif.data(), quality);
 	if (jpeg_size < 0) {
 		LOG(JPEG, Error) << "Failed to encode stream image";
 		processComplete.emit(streamBuffer, PostProcessor::Status::Error);