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

[Jacopo Mondi]

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

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