[libcamera-devel] [PATCH] android: camera_device: Calculate MAX_JPEG_SIZE

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Feb 02, 2021 at 04:50:46PM +0100, Jacopo Mondi wrote:
> On Tue, Feb 02, 2021 at 03:23:22PM +0000, Kieran Bingham wrote:
> > On 02/02/2021 12:36, Jacopo Mondi wrote:
> > > Calculate the JPEG maximum size using the maximum preview format size
> > > multiplied by a 1.5 factor.
> > >
> > > The same multiplication factor is used in the existing HAL
> > > implementation in ChromeOS.
> >
> > This is definitely better than the somewhat arbitrary fixed value I
> > picked from another HAL.
> >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp | 21 ++++++++++-----------
> > >  1 file changed, 10 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index a50b0ebfe60e..cb87d97888ed 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -347,12 +347,6 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer
> > >  {
> > >  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> > >
> > > -	/*
> > > -	 * \todo Determine a more accurate value for this during
> > > -	 *  streamConfiguration.
> > > -	 */
> > > -	maxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */
> > > -
> > >  	maker_ = "libcamera";
> > >  	model_ = "cameraModel";
> > >
> > > @@ -629,6 +623,7 @@ int CameraDevice::initializeStreamConfigurations()
> > >  							mappedFormat,
> > >  							cameraResolutions);
> > >
> > > +		Size maxJpegSize;
> > >  		for (const Size &res : resolutions) {
> > >  			streamConfigurations_.push_back({ res, androidFormat });
> > >
> > > @@ -643,9 +638,17 @@ int CameraDevice::initializeStreamConfigurations()
> > >  			 * \todo Support JPEG streams produced by the Camera
> > >  			 * natively.
> > >  			 */
> > > -			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> > > +			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) {
> > >  				streamConfigurations_.push_back(
> > >  					{ res, HAL_PIXEL_FORMAT_BLOB });
> > > +
> > > +				if (res > maxJpegSize) {
> >
> > Aha, so this runs over each of the supported stream configurations and
> > thus only the max gets through ? is that right?
> 
> yes, this collects the maximum YUV size from which we encode JPEG
> 
> > (I think it is, so there's no issue)
> >
> > > +					maxJpegSize = res;
> > > +					maxJpegBufferSize_ = maxJpegSize.width
> > > +							   * maxJpegSize.height
> > > +							   * 1.5;

You could move the calculation of maxJpegBufferSize_ after the loop.
Youl could also write the maxJpegSize calculation as

				maxJpegSize = std::max(maxJpegSize, res);

I'd do at least the former to make sure maxJpegBufferSize_ gets
initialized in all code paths. Up to you for the latter.

> > > +				}
> > > +			}
> > >  		}
> > >  	}
> > >
> > > @@ -878,10 +881,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  				  availableThumbnailSizes.data(),
> > >  				  availableThumbnailSizes.size());
> > >
> > > -	/*
> > > -	 * \todo Calculate the maximum JPEG buffer size by asking the encoder
> > > -	 * giving the maximum frame size required.
> > > -	 */
> >
> > Is this no longer worth considering?
> > I sort of agree at the moment. And if we decide we want to ask the
> > encoders we can handle that later. I don't think we need to worry about
> > the todo.
> 
> I don't know for sure :/
> Also the 1.5 ratio is a copy&paste from the existing HALs

I think it's worth keeping the todo. You could move it to
CameraDevice::initializeStreamConfigurations() right before setting
maxJpegBufferSize_. We're one step closer to addressing this as we now
calculate the maximum frame size, the last remaining matter will be to
move the maxJpegBufferSize calculation to the encoder class.

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

> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > >  	staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);
> > >
> > >  	/* Sensor static metadata. */

-- 
Regards,

Laurent Pinchart