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

[Umang Jain]

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


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