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

[Umang Jain]

Hi Jacopo,


On 9/9/21 3:39 PM, Jacopo Mondi wrote:
> Hi Umang,
>
> On Thu, Sep 09, 2021 at 12:54:18PM +0530, Umang Jain wrote:
>> Hi Hiro,
>>
>> On 9/9/21 12:26 PM, Hirokazu Honda wrote:
>>> Hi Umang,
>>>
>>> On Thu, Sep 9, 2021 at 3:38 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>>>> 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.
> But please move it below the following paragraph to keep todo items at
> the end of the comment block


I am going to undo the deletion so it will stay as is. I'll make sure 
any \todos are at end of comment block

>
>>>>>> +                * 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.
>>>>
>>> I would
>>> size_t YPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 0);
>>> size_t UVPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 1);
>>>    thumbnailPlanes.push_back({ rawThumbnail.data(), YPlaneSize });
>>>    thumbnailPlanes.push_back({ rawThumbnail.data() + YPlaneSize, UVPlaneSize });
>> Ok, I see. These helpers are quite recent, and I wasn't that familiar with
>> these helpers.
>>
>> These are more readable, I think. I'll wait for more reviews to pitch in
>> meanwhile.
> Whatever you decide to use:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>


Thanks, I'll apply Hiro's suggestion and test once before pushing to master.

This is slightly urgent patch to merge since CTS will fail to run 
without this. So I'll merge it in a few minutes.

>
> Thanks
>    j
>>
>>> I don't have a strong opinion. Up to you.
>>> -Hiro
>>>>> 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
>>>>>>