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

[Hirokazu Honda]

Hi Umang, thank you for the patch.

On Wed, Sep 8, 2021 at 3:23 PM Umang Jain <umang.jain at ideasonboard.com> 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
> 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

This is not correct. Since rawThumbanil is NV12, the second plane size
is a half of the first plane size.

> +                       };
> +                       thumbnailPlanes.push_back(plane);
> +               }

Uhm, I would avoid introducing the dependency how rawThumbnail is
created in thumbnailer_.
Since this will be resolved by my patch series [1] later, I am fine
with this as a temporary solution.
By the way, this is related to Launrent's change, should this be a
part of the series?

+Laurent Pinchart for visibility.

[1] https://patchwork.libcamera.org/project/libcamera/list/?series=2423

-Hiro
>
> -               int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
> +               int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
>                                                          *thumbnail, {}, quality);
>                 thumbnail->resize(jpeg_size);
>
> --
> 2.31.1
>