[libcamera-devel,v3,08/10] android: camera_buffer: Add method to get the JPEG blob size
diff mbox series

Message ID 20210302115108.103328-9-jacopo@jmondi.org
State Superseded
Headers show
Series
  • android: Supports memory backends
Related show

Commit Message

Jacopo Mondi March 2, 2021, 11:51 a.m. UTC 
To maintain compatibility with platforms that do not provide a memory
backend implementation add a method to be return the size of the buffer
used for JPEG encoding capped to a maximum size.

Platforms that implement a memory backend will always calculate the
correct buffer size.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_buffer.h              |  2 ++
 src/android/jpeg/post_processor_jpeg.cpp | 12 ++++++++++--
 src/android/mm/generic_camera_buffer.cpp | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart March 2, 2021, 12:03 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Mar 02, 2021 at 12:51:06PM +0100, Jacopo Mondi wrote:
> To maintain compatibility with platforms that do not provide a memory
> backend implementation add a method to be return the size of the buffer
> used for JPEG encoding capped to a maximum size.
> 
> Platforms that implement a memory backend will always calculate the
> correct buffer size.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_buffer.h              |  2 ++
>  src/android/jpeg/post_processor_jpeg.cpp | 12 ++++++++++--
>  src/android/mm/generic_camera_buffer.cpp | 14 ++++++++++++++
>  3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 2311cdaf96b2..342aac6d3f14 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -26,6 +26,8 @@ public:
>  
>  	libcamera::Span<const uint8_t> plane(unsigned int plane) const;
>  	libcamera::Span<uint8_t> plane(unsigned int plane);
> +
> +	size_t jpegBlobSize(size_t maxJpegBlobSize);

You can make this function const.

I'd name the parameter maxJpegBufferSize, in this context "blob" refers
to the small camera3_jpeg_blob structure at the end. Similarly, I'd
either name the function jpegBufferSize(), or name it jpegBlobOffset()
and include the - sizeof(struct camera3_jpeg_blob) there.

>  };
>  
>  #endif /* __ANDROID_CAMERA_BUFFER_H__ */
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 83244ce6769e..65ab6b196ad1 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -182,8 +182,16 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	}
>  
>  	/* Fill in the JPEG blob header. */
> -	uint8_t *resultPtr = destination->plane(0).data()
> -			   + destination->plane(0).size()
> +	/*
> +	 * \todo For backward compatibility reasons with the android_generic
> +	 * memory backend, continue using the maxJpegBufferSize in case the
> +	 * computed buffer size is larger. This can be dropped once all
> +	 * supported platforms will have a working memory backend that
> +	 * returns the correct buffer size.
> +	 */
> +	size_t blobSize = std::min<unsigned int>(cameraDevice_->maxJpegBufferSize(),
> +						 destination->plane(0).size());
> +	uint8_t *resultPtr = destination->plane(0).data() + blobSize
>  			   - sizeof(struct camera3_jpeg_blob);

Bad rebase ? I don't think this belongs to this patch (and I would
expect to see jpegBlobSize() used here :-)).

>  	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
>  	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index 45a83c351266..5637b3f415eb 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -27,6 +27,8 @@ public:
>  	unsigned int numPlanes() const;
>  
>  	Span<uint8_t> plane(unsigned int plane);
> +
> +	size_t jpegBlobSize(size_t maxJpegBlobSize);
>  };
>  
>  CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> @@ -77,6 +79,12 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  	return maps_[plane];
>  }
>  
> +size_t CameraBuffer::Private::jpegBlobSize(size_t maxJpegBlobSize)
> +{
> +	return std::min<unsigned int>(plane(0).size(),
> +				      maxJpegBlobSize);
> +}
> +
>  CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
>  	: Extensible(new Private(this, camera3Buffer, flags))
>  {
> @@ -109,3 +117,9 @@ Span<uint8_t> CameraBuffer::plane(unsigned int plane)
>  	Private *const d = LIBCAMERA_D_PTR();
>  	return d->plane(plane);
>  }
> +
> +size_t CameraBuffer::jpegBlobSize(size_t maxJpegBlobSize)
> +{
> +	Private *const d = LIBCAMERA_D_PTR();
> +	return d->jpegBlobSize(maxJpegBlobSize);
> +}
Jacopo Mondi March 2, 2021, 1:27 p.m. UTC | #2 
Hi Laurent,

On Tue, Mar 02, 2021 at 02:03:25PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Mar 02, 2021 at 12:51:06PM +0100, Jacopo Mondi wrote:
> > To maintain compatibility with platforms that do not provide a memory
> > backend implementation add a method to be return the size of the buffer
> > used for JPEG encoding capped to a maximum size.
> >
> > Platforms that implement a memory backend will always calculate the
> > correct buffer size.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_buffer.h              |  2 ++
> >  src/android/jpeg/post_processor_jpeg.cpp | 12 ++++++++++--
> >  src/android/mm/generic_camera_buffer.cpp | 14 ++++++++++++++
> >  3 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > index 2311cdaf96b2..342aac6d3f14 100644
> > --- a/src/android/camera_buffer.h
> > +++ b/src/android/camera_buffer.h
> > @@ -26,6 +26,8 @@ public:
> >
> >  	libcamera::Span<const uint8_t> plane(unsigned int plane) const;
> >  	libcamera::Span<uint8_t> plane(unsigned int plane);
> > +
> > +	size_t jpegBlobSize(size_t maxJpegBlobSize);
>
> You can make this function const.
>
> I'd name the parameter maxJpegBufferSize, in this context "blob" refers
> to the small camera3_jpeg_blob structure at the end. Similarly, I'd
> either name the function jpegBufferSize(), or name it jpegBlobOffset()
> and include the - sizeof(struct camera3_jpeg_blob) there.
>

I don't know, I hope we can drop this method rather soon and replace
it with a simpler plane(0).size() so I would leave the subtraction of
the struct camera3_jpeg_blob size outside of the CameraBuffer
function.

> >  };
> >
> >  #endif /* __ANDROID_CAMERA_BUFFER_H__ */
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index 83244ce6769e..65ab6b196ad1 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -182,8 +182,16 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >  	}
> >
> >  	/* Fill in the JPEG blob header. */
> > -	uint8_t *resultPtr = destination->plane(0).data()
> > -			   + destination->plane(0).size()
> > +	/*
> > +	 * \todo For backward compatibility reasons with the android_generic
> > +	 * memory backend, continue using the maxJpegBufferSize in case the
> > +	 * computed buffer size is larger. This can be dropped once all
> > +	 * supported platforms will have a working memory backend that
> > +	 * returns the correct buffer size.
> > +	 */
> > +	size_t blobSize = std::min<unsigned int>(cameraDevice_->maxJpegBufferSize(),
> > +						 destination->plane(0).size());
> > +	uint8_t *resultPtr = destination->plane(0).data() + blobSize
> >  			   - sizeof(struct camera3_jpeg_blob);
>
> Bad rebase ? I don't think this belongs to this patch (and I would
> expect to see jpegBlobSize() used here :-)).

Ah ups.
yes, that belongs to the old patch.

I'll re-send a v4, as this patch will be broken in 2: one that
introduces the function the other that uses it.

>
> >  	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> >  	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > index 45a83c351266..5637b3f415eb 100644
> > --- a/src/android/mm/generic_camera_buffer.cpp
> > +++ b/src/android/mm/generic_camera_buffer.cpp
> > @@ -27,6 +27,8 @@ public:
> >  	unsigned int numPlanes() const;
> >
> >  	Span<uint8_t> plane(unsigned int plane);
> > +
> > +	size_t jpegBlobSize(size_t maxJpegBlobSize);
> >  };
> >
> >  CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> > @@ -77,6 +79,12 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> >  	return maps_[plane];
> >  }
> >
> > +size_t CameraBuffer::Private::jpegBlobSize(size_t maxJpegBlobSize)
> > +{
> > +	return std::min<unsigned int>(plane(0).size(),
> > +				      maxJpegBlobSize);
> > +}
> > +
> >  CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> >  	: Extensible(new Private(this, camera3Buffer, flags))
> >  {
> > @@ -109,3 +117,9 @@ Span<uint8_t> CameraBuffer::plane(unsigned int plane)
> >  	Private *const d = LIBCAMERA_D_PTR();
> >  	return d->plane(plane);
> >  }
> > +
> > +size_t CameraBuffer::jpegBlobSize(size_t maxJpegBlobSize)
> > +{
> > +	Private *const d = LIBCAMERA_D_PTR();
> > +	return d->jpegBlobSize(maxJpegBlobSize);
> > +}
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
index 2311cdaf96b2..342aac6d3f14 100644
--- a/src/android/camera_buffer.h
+++ b/src/android/camera_buffer.h
@@ -26,6 +26,8 @@  public:
 
 	libcamera::Span<const uint8_t> plane(unsigned int plane) const;
 	libcamera::Span<uint8_t> plane(unsigned int plane);
+
+	size_t jpegBlobSize(size_t maxJpegBlobSize);
 };
 
 #endif /* __ANDROID_CAMERA_BUFFER_H__ */
diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
index 83244ce6769e..65ab6b196ad1 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -182,8 +182,16 @@  int PostProcessorJpeg::process(const FrameBuffer &source,
 	}
 
 	/* Fill in the JPEG blob header. */
-	uint8_t *resultPtr = destination->plane(0).data()
-			   + destination->plane(0).size()
+	/*
+	 * \todo For backward compatibility reasons with the android_generic
+	 * memory backend, continue using the maxJpegBufferSize in case the
+	 * computed buffer size is larger. This can be dropped once all
+	 * supported platforms will have a working memory backend that
+	 * returns the correct buffer size.
+	 */
+	size_t blobSize = std::min<unsigned int>(cameraDevice_->maxJpegBufferSize(),
+						 destination->plane(0).size());
+	uint8_t *resultPtr = destination->plane(0).data() + blobSize
 			   - sizeof(struct camera3_jpeg_blob);
 	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
 	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
index 45a83c351266..5637b3f415eb 100644
--- a/src/android/mm/generic_camera_buffer.cpp
+++ b/src/android/mm/generic_camera_buffer.cpp
@@ -27,6 +27,8 @@  public:
 	unsigned int numPlanes() const;
 
 	Span<uint8_t> plane(unsigned int plane);
+
+	size_t jpegBlobSize(size_t maxJpegBlobSize);
 };
 
 CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
@@ -77,6 +79,12 @@  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 	return maps_[plane];
 }
 
+size_t CameraBuffer::Private::jpegBlobSize(size_t maxJpegBlobSize)
+{
+	return std::min<unsigned int>(plane(0).size(),
+				      maxJpegBlobSize);
+}
+
 CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
 	: Extensible(new Private(this, camera3Buffer, flags))
 {
@@ -109,3 +117,9 @@  Span<uint8_t> CameraBuffer::plane(unsigned int plane)
 	Private *const d = LIBCAMERA_D_PTR();
 	return d->plane(plane);
 }
+
+size_t CameraBuffer::jpegBlobSize(size_t maxJpegBlobSize)
+{
+	Private *const d = LIBCAMERA_D_PTR();
+	return d->jpegBlobSize(maxJpegBlobSize);
+}