[libcamera-devel] [PATCH v3 2/2] android: Fix generation of thumbnail for EXIF data

[paul.elder at ideasonboard.com]

Hi Umang,

On Thu, Sep 23, 2021 at 01:23:53PM +0530, Umang Jain wrote:
> Hi Paul,
> 
> On 9/23/21 1:10 PM, paul.elder at ideasonboard.com wrote:
> > Hi Umang,
> > 
> > On Thu, Sep 23, 2021 at 12:54:53PM +0530, Umang Jain wrote:
> > > Generation of thumbnail is not occuring currently because
> > > ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed
> > > to PostProcessorJpeg::process(). The commit 1264628d3c92("android:
> > > jpeg: Configure thumbnailer based on request metadata") introduced
> > > the mechanism to retrieve the thumbanil size from request metadata,
> > > however it didn't add the counterpart i.e. inserting the size in
> > > the request metadata in request metadata template, at the first place.
> > > 
> > > The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in
> > > the request metadata template populated by
> > > CameraCapabilities::requestTemplatePreview(). The value for
> > > ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size
> > > reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.
> > > 
> > > Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on request metadata")
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > >   src/android/camera_capabilities.cpp | 24 +++++++++++++++++++++++-
> > >   1 file changed, 23 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index 238b44db..ba6adf73 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -1347,7 +1347,7 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con
> > >   	 * CameraMetadata is capable to resize the container on the fly, if the
> > >   	 * number of entries get exceeded.
> > >   	 */
> > > -	auto requestTemplate = std::make_unique<CameraMetadata>(21, 36);
> > > +	auto requestTemplate = std::make_unique<CameraMetadata>(22, 38);
> > >   	if (!requestTemplate->isValid()) {
> > >   		return nullptr;
> > >   	}
> > > @@ -1368,6 +1368,28 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con
> > >   	requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > >   				  entry.data.i32, 2);
> > > +	/*
> > > +	 * Get thumbnail sizes from static metadata and add the first non-zero
> > > +	 * size to the template.
> > > +	 */
> > > +	found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> > > +					  &entry);
> > > +	ASSERT(found && entry.count >= 4);
> > > +	unsigned int j = 0;
> > > +	while (j < entry.count / 2) {
> > Since you're incrementing j by 2, shouldn't the limit be entry.count?
> 
> 
> My understanding is 2 entries make 1 size, no?
> ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES be like for e.g.
> 
>     {0, 0, w1, h1, w2, h2}
> 
> So entry.count is 6 but sizes are 3
> 
> To go to next Size, incremement by 2 so, j += 2

That's true, but for example:

{0, 0, w1, h1, w2, h2}

Iteration 1: j = 0, so you take [0, 1]; afterward j += 2
Iteration 2: j = 2, so you take [2, 3]; afterward j += 2
Iteration 3: j = 4, so you take [4, 5]; afterward j += 2
Itreation 4: j = 6, fails j < entry.count; skip

If you stop at entry.count / 2 then you'll skip the second half of
pairs.


Paul

> 
> Does it make sense?
> 
> 
> > 
> > > +		if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] == 0) {
> > > +			j += 2;
> > > +			continue;
> > > +		}
> > > +
> > > +		requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
> > > +					  entry.data.i32 + j, 2);
> > > +		break;
> > > +	}
> > > +
> > > +	requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
> > > +				  entry.data.i32 + 2, 2);
> > iirc adding an entry that already exists causes weird problems...
> 
> 
> Ah crap, this should be outside the loop. I am re-assigning it here. Sorry,
> rebase fiasco it seems.
> 
> > 
> > 
> > Paul
> > 
> > > +
> > >   	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> > >   	requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
> > > -- 
> > > 2.31.0
> > >