[libcamera-devel] [PATCH] android: camera_device: Generate JPEG thumbnail sizes

[Jacopo Mondi]

Hi Kieran,

On Thu, Feb 04, 2021 at 02:47:13PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 04/02/2021 14:35, Jacopo Mondi wrote:
> > The list of the available thumbnail sizes is generated from the
> > list of available JPEG resolution, one for each aspect ratio.
> >
> > This change fixes the CTS test
> > android.hardware.cts.CameraTest#testJpegThumbnailSize
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 39 ++++++++++++++++++++++++++++++-----
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index df5e295656d7..0b699c957990 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -871,12 +871,41 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  				  &availableControlModes, 1);
> >
> >  	/* JPEG static metadata. */
> > -	std::vector<int32_t> availableThumbnailSizes = {
> > -		0, 0,
> > -	};
> > +
> > +	/*
> > +	 * Create the list of supported tumbnail sizes by inspecting the
>
> s/tumbnail/thumbnail/
>
> > +	 * available JPEG resolutions collected in streamConfigurations_ and
> > +	 * generate one entry for each aspect ratio.
> > +	 *
> > +	 * The JPEG thumbnailer can freely scale, so pick an arbitrary
> > +	 * (160, 120) size as designated thumbnail size.
>
> Shouldn't we give a square size here? 160,160?
>
> That way a 9:16 ratio would be the same 'size' as a 16:9 for example.
> (Thinking of phones/devices taking portrait rather than landscape pictures).

(160, 120) is arbitrary, and you're probably right here.

I'll test with 160, 160 and resend with your comments taken in.

Thanks
  j

>
> > +	 */
> > +	constexpr Size maxJpegThumbnail(160, 120);
> > +	std::vector<Size> thumbnailSizes;
> > +	thumbnailSizes.push_back({ 0, 0 });
> > +	for (const auto &entry : streamConfigurations_) {
> > +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> > +			continue;
> > +
> > +		Size thumbnailSize = maxJpegThumbnail
> > +				     .boundedToAspectRatio({ entry.resolution.width,
> > +							     entry.resolution.height });
>
> I love how easy those helpers make maintaining aspect ratios.
>
> > +		thumbnailSizes.push_back(thumbnailSize);
> > +	}
> > +
> > +	std::sort(thumbnailSizes.begin(), thumbnailSizes.end());
> > +	auto last = std::unique(thumbnailSizes.begin(), thumbnailSizes.end());
> > +	thumbnailSizes.erase(last, thumbnailSizes.end());
> > +
> > +	/* Transform it in a list of integers that can be consumed. */
>
> 'in to a list' ?
>
> > +	std::vector<int32_t> thumbnailEntries;
> > +	thumbnailEntries.reserve(thumbnailSizes.size() * 2);
> > +	for (const auto &size : thumbnailSizes) {
> > +		thumbnailEntries.push_back(size.width);
> > +		thumbnailEntries.push_back(size.height);
> > +	}
> >  	staticMetadata_->addEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> > -				  availableThumbnailSizes.data(),
> > -				  availableThumbnailSizes.size());
> > +				  thumbnailEntries.data(), thumbnailEntries.size());
>
> The number of times we add a vector to addEntry() shouldn't we have an
> addEntry(id, vector) so we don't duplicate .data(), .size() each time?
>
> But that's a digression - not for this patch.
>
> Otherwise sounds good to me.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
> >
> >  	/*
> >  	 * \todo Calculate the maximum JPEG buffer size by asking the encoder
> >
>
> --
> Regards
> --
> Kieran