[libcamera-devel] [PATCH 09/12] android: jpeg: Use maxJpegBufferSize() for compatibility

[Jacopo Mondi]

Hi Laurent,

On Mon, Mar 01, 2021 at 11:28:22AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Mar 01, 2021 at 08:41:10AM +0100, Jacopo Mondi wrote:
> > On Sun, Feb 28, 2021 at 08:48:32PM +0200, Laurent Pinchart wrote:
> > > On Fri, Feb 26, 2021 at 02:29:29PM +0100, Jacopo Mondi wrote:
> > > > Platforms that do not provide a memory backend implementation should
> > > > keep using the maxJpegBufferSize() value to calculate the location where
> > > > to place the JPEG blob id, as the android_generic backend returns the
> > > > allocated buffer size as calculated using lseek which is larger than
> > > > the maximum JPEG frame size, which is where the framework expects the
> > > > JPEG blob to be placed.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/android/jpeg/post_processor_jpeg.cpp | 14 +++++++++++---
> > > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > > > index d6eeb962e81d..e7f66d66698c 100644
> > > > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > > > @@ -185,9 +185,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > > >  	}
> > > >
> > > >  	/* Fill in the JPEG blob header. */
> > > > -	uint8_t *resultPtr = destination->plane(0) +
> > > > -			     destination->planeSize(0) -
> > > > -			     sizeof(struct camera3_jpeg_blob);
> > > > +	/*
> > > > +	 * \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->planeSize(0));
> > >
> > > Can't Chrome OS allocate buffers larger than maxJpegBufferSize() ? In
> >
> > I don't think the real buffer size, as calculated through the correct
> > memory backend could be larger than the maximum reported size. The
> > purpose of the jpeg.MaxSize is to report the maximum allocation the
> > camera framework has to perform to support the largest JPEG size,
> > don't it ?
>
> Yes, but there's nothing that would preclude the camera service from
> allocating more, especially if the maximum JPEG size we report isn't
> page-aligned. The page alignment could be included in or could be left

And that's what happen at the moment with the generic backend. The
size computed through it is larger, but android wants the JPEG_BLOB_ID
at the end of maxJpegBufferSize().

When using the correct backend I always get a size smaller than the
max jpeg one.

> out from the buffer size as seen from the camera service, that's an
> implementation decision we can't control. That's why I'd prefer not
> depending on this assumption here.
>

How it works exactly and where the camera service expects the blob id
to be in case the buffer size gets past the max size, I cannot tell.

What's the alternative solution ? Provide a method in the CameraBuffer
that says "yes, I'm reliable" and if not fall-back on the
maxJpegBufferSize ? That's two bad hacks competing for the one being
less nasty, I don't see a nice way to get away with this tbh.


> > This (seems) to only happen when the backend mis-calculate the buffer
> > size and returns a value larger than jpeg.MaxSize
> >
> > > that case it would expect the JPEG blob to be at the end of the buffer,
> > > not after maxJpegBufferSize() bytes.
> > >
> > > I'm tempted to just drop this patch. If we want to keep it for other
> > > Android platforms, we'll need an additional function in CameraBuffer to
> > > report if the implementation has retrieved the size correctly. It's a
> > > bit of a hack, but this is a hack anyway :-)
> > >
> > > Up to you whether you prefer dropping the patch or keeping it with a new
> > > CameraBuffer function.
> > >
> > > > +	uint8_t *resultPtr = destination->plane(0) + blobSize
> > > > +			   - sizeof(struct camera3_jpeg_blob);
> > > >  	auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);
> > > >  	blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;
> > > >  	blob->jpeg_size = jpeg_size;
>
> --
> Regards,
>
> Laurent Pinchart