[libcamera-devel] [PATCH] android: yuv: Fix wrong access of source buffer

[Hirokazu Honda]

Hi Laurent,

On Fri, Aug 6, 2021 at 12:57 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Aug 06, 2021 at 12:21:41AM +0900, Hirokazu Honda wrote:
> > On Fri, Aug 6, 2021 at 12:02 AM Laurent Pinchart wrote:
> > > On Thu, Aug 05, 2021 at 10:28:05PM +0900, Hirokazu Honda wrote:
> > > > File descriptors of FrameBuffer given in PostProcessor::process()
> > > > for all the planes point the same buffer. To access the beginning
> > > > of the second or later plane, it is necessary to add offsets to
> > > > the beginning of the buffer.
> > > > Fix the wrong access to the second plane of NV12 in
> > > > PostProcessorYuv.
> > >
> > > This is an issue that comes from V4L2, where NV12, a multi-planar
> > > format, can be stored with the two planes contiguously in the same
> > > dmabuf, or in separate dmabufs. We have two V4L2 pixel formats for those
> > > (for instance, for NV12, that's V4L2_PIX_FMT_NV12 and
> > > V4L2_PIX_FMT_NV12M), and two APIs, the V4L2 single-planar API that only
> > > supports the single dmabuf format, and the V4L2 multi-planar API that
> > > supports either formats.
> > >
> > > This patch will work with V4L2_PIX_FMT_NV12 but not with
> > > V4L2_PIX_FMT_NV12M. Let's not force applications to deal with this mess,
> > > I don't want to expose it through the libcamera API. We should instead
> > > handle it in the MappedBuffer class, so that maps()[1] will return the
> > > correct value.
> > >
> > > In order to support both NV12 and NV12M, we'll have to add an offset
> > > field to FrameBuffer::Plane, but in the short term, as we only support
> > > the single-dmabuf formats in our pipeline handlers, I think we can
> > > hardcode the calculation below in the MappedBuffer class (I haven't
> > > tried it though, so there may be something I'm missing).
> >
> > Yeah, that is what I would like to discuss through this patch.
> > I agree with handling this MappedBuffer.
> > The current libcamera implementation doesn't handle multi-planar format.
> > I filed https://bugs.libcamera.org/show_bug.cgi?id=9 previously.
> > Shall I also add strides() to MappedBuffer or FrameBuffer?
>
> I don't think so. Strides are a property of the format. They're
> currently exposed in StreamConfiguration. We have a single stride there,
> not a stride per plane, but I don't think it's urgent to rework this.
>

I got it. I will abandon the patch in favor of  the suggested
MappedBuffer class change.

-Hiro
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > >  src/android/yuv/post_processor_yuv.cpp | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> > > > index 772e805b..3b801e96 100644
> > > > --- a/src/android/yuv/post_processor_yuv.cpp
> > > > +++ b/src/android/yuv/post_processor_yuv.cpp
> > > > @@ -62,9 +62,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,
> > > >               return -EINVAL;
> > > >       }
> > > >
> > > > -     int ret = libyuv::NV12Scale(sourceMapped.maps()[0].data(),
> > > > +     const uint8_t *sourceY = sourceMapped.maps()[0].data();
> > > > +     const uint8_t *sourceUV = sourceY + sourceStride_[0] * sourceSize_.height;
> > > > +
> > > > +     int ret = libyuv::NV12Scale(sourceY,
> > > >                                   sourceStride_[0],
> > > > -                                 sourceMapped.maps()[1].data(),
> > > > +                                 sourceUV,
> > > >                                   sourceStride_[1],
> > > >                                   sourceSize_.width, sourceSize_.height,
> > > >                                   destination->plane(0).data(),
>
> --
> Regards,
>
> Laurent Pinchart