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

[Jacopo Mondi]

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 ?

> 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);

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
>