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

[Umang Jain]

Hi Hiro,

On 9/8/21 1:20 PM, Hirokazu Honda wrote:
> 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.


Oh oops, yes you are right. Forgot to take into consideration :(

Will need to think more a bit hmm!

>> +                       };
>> +                       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?


The series is merged as far as I can see. And thumbnailing is broken on 
master. So, we need to fix it

>
> +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
>>