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

[Laurent Pinchart]

Hi Jacopo,

On Wed, Sep 08, 2021 at 10:05:03AM +0200, Jacopo Mondi wrote:
> 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 ?

You're right. Hiro has reported the same :-)

> 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 NV12 thumbnail doesn't have to be stored in two separate memory
buffers, it's just that the encoder now needs to be told where the two
planes are, even if they're contiguous.

We could split the planes, but I'm not sure it's worth it. I'd go for
the simplest solution for now, it will be cleaned up once we get an
Image class in libcamera.

> 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.
> 
> > +			thumbnailPlanes.push_back(plane);
> > +		}
> >
> > -		int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
> > +		int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
> >  							 *thumbnail, {}, quality);
> >  		thumbnail->resize(jpeg_size);
> >

-- 
Regards,

Laurent Pinchart