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

[Hirokazu Honda]

Hi Umang,

On Wed, Sep 8, 2021 at 4:54 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> 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
>

Oh I didn't realize it. :-)
Let's go ahead then.

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