[libcamera-devel] [PATCH v2] android: jpeg: Split and pass the thumbnail planes to encoder

[Umang Jain]

Hi Hiro?

On 9/8/21 3:32 PM, Hirokazu Honda wrote:
> Hi Umang, thank you for the patch.
>
> On Wed, Sep 8, 2021 at 6:50 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, we are passing a contiguous buffer
>> which is treated as only one plane (instead of two, as thumbnail is NV12).
>>
>> Hence, split the thumbnail data into respective planes according to NV12.
>> This fixes a crash in encoding of thumbnails.
>>
>> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers")
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> Changes in v2:
>> - Fix basic split logic error.
>> - Add Jacopo's suggestion as a \todo since it's a superior method to fix
>>    this.
>> ---
>>   src/android/jpeg/post_processor_jpeg.cpp | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index 68d74842..1bf507a4 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -66,13 +66,23 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>>          int ret = thumbnailEncoder_.configure(thCfg);
>>
>>          if (!rawThumbnail.empty() && !ret) {
>> +               thumbnail->resize(rawThumbnail.size());
>> +
>>                  /*
>> -                * \todo Avoid value-initialization of all elements of the
>> -                * vector.
> I think this comment should not be removed.
>
>> +                * Split planes manually as the encoder expects a vector of
>> +                * planes.
>> +                *
>> +                * \todo Pass a vector of planes directly to
>> +                * Thumbnailer::createThumbnailer above and remove the manual
>> +                * planes split from here.
>>                   */
>> -               thumbnail->resize(rawThumbnail.size());
>> +               std::vector<Span<uint8_t>> thumbnailPlanes;
>> +               unsigned int thumbnailSize = targetSize.height * targetSize.width;
>> +               thumbnailPlanes.push_back({ rawThumbnail.data(), thumbnailSize });
>> +               thumbnailPlanes.push_back({ rawThumbnail.data() + thumbnailSize,
>> +                                           thumbnailSize / 2 });
> Shall PixelFormatInfo::planeSize() be used?


Used for what? The " / 2" part?

Also, Did you mean PixelFormatInfo::numPlanes() instead? I won't worry 
too much here, as the Thumbnail class is tightly bound to NV12. So that 
planes are constant. See Thumbnailer::configure

When we add support for more diverse thumbnailing for formats, I expect 
this to be changed in a major way.


>
> With nits,
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
>
> -Hiro
>> -               int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
>> +               int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
>>                                                           *thumbnail, {}, quality);
>>                  thumbnail->resize(jpeg_size);
>>
>> --
>> 2.31.0
>>