[libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail planes correctly to encoder

[Umang Jain]

Hi Jacopo

On 9/8/21 1:35 PM, Jacopo Mondi wrote:
> Hi Umang
>
> On Wed, Sep 08, 2021 at 11:53:16AM +0530, Umang Jain wrote:
>> After multi-planar support was introduced for jpeg encoding as well,
>> EncoderLibJpeg::encode() expects a vector of planes as the source of
>> framebuffer to be encoded. Currently, this is broken for thumbnail
>> encoding as the thumbnail generated is directly passed as a single
>> plane.
>>
>> Hence, piece the thumbnail data as planes if the parent Framebuffer is
> Did you mean 'place', as in 'to place' as a verb ?


place? I wrote 'piece'. I was going to write 'chop', okay, nevermind...

>
>> multi-planar. This fixes the encoding of the corresponding thumbnail.
>>
>> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers")
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index 68d74842..d896581f 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>>   	int ret = thumbnailEncoder_.configure(thCfg);
>>
>>   	if (!rawThumbnail.empty() && !ret) {
>> -		/*
>> -		 * \todo Avoid value-initialization of all elements of the
>> -		 * vector.
>> -		 */
>> -		thumbnail->resize(rawThumbnail.size());
>> +		std::vector<Span<uint8_t>> thumbnailPlanes;
>> +		for (int i = 0; i < source.planes().size(); i++) {
>> +			unsigned int thumbnailSize =
>> +				targetSize.height * targetSize.width;
>> +			Span<uint8_t> plane{
>> +				rawThumbnail.data() + (i * thumbnailSize),
>> +				thumbnailSize
>> +			};
> So, I never really understood the thumbnailer bits but this looks
> surprising to me. Are all the planes of the same size ? Aren't we
> assuming NV12 as the format of the rawThumbnail ? Shouldn't the CbCr
> plane be half in size ?
>
> So, I haven't followed much the fallout of the multi-planar support
> but what I see here is that now the libjpeg encoder requires the
> Y and CbCr planes to be stored in different 'planes()' so I
> understand you have to artificially create those planes as a
> consequence of the fact that createThumbnail() scales-down the image
> into a single span of chars
>
> 	/* Stores the raw scaled-down thumbnail bytes. */
> 	std::vector<unsigned char> rawThumbnail;
> 	thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);
>
> Wouldn't it be better to:
> 	/* Stores the raw scaled-down thumbnail bytes. */
> 	std::vector<Span<unsigned char>> rawThumbnail;
> 	thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);


Yes this is definitely better. I would also need to ensure correct Span 
is used to store the scaled down bits - which will probably change quite 
a bit of pointer arithmetic in the Thumbnailer code. It's not hard to do 
per say, but I guess Laurent is in favor of a quick and simpler patch 
for now (and I too).

I have added your suggestion as a \todo though in v2, so the idea 
doesn't gets lost in future.

>
> And have the raw thumbnail already displaced in two planes ?
> The code seems to be there
>
>                  dstC[(y / 2) * tw + x + 0] = srcCb[(sourceX / 2) * 2];
>                  dstC[(y / 2) * tw + x + 1] = srcCr[(sourceX / 2) * 2];
>
> You just need to point dstC to the second plane if I'm not mistaken.
>
> Thanks
>     j
>
>
>
>> +			thumbnailPlanes.push_back(plane);
>> +		}
>>
>> -		int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
>> +		int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
>>   							 *thumbnail, {}, quality);
>>   		thumbnail->resize(jpeg_size);
>>
>> --
>> 2.31.1
>>