| Message ID | 20210805132805.824754-1-hiroh@chromium.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. 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). > Signed-off-by: Hirokazu Honda <hiroh@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(),
Hi Laurent, On Fri, Aug 6, 2021 at 12:02 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > 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? Regards, -Hiro > > Signed-off-by: Hirokazu Honda <hiroh@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
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@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(),
Hi Laurent, On Fri, Aug 6, 2021 at 12:57 AM Laurent Pinchart <laurent.pinchart@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@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
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(),
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. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/yuv/post_processor_yuv.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)