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

[Laurent Pinchart]

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.

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