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

[Jacopo Mondi]

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 at 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