[libcamera-devel] [PATCH v3 12/30] libcamera: v4l2_videodevice: Take stride into account to compute offsets

[Laurent Pinchart]

Hi Kieran,

On Tue, Sep 07, 2021 at 12:37:49AM +0100, Kieran Bingham wrote:
> On 06/09/2021 23:56, Laurent Pinchart wrote:
> > When creating FrameBuffer instances, the V4L2VideoDevice computes plane
> > offsets using minimal stride for the format. This doesn't always produce
> > a valid result when the device requires padding at the end of lines. Fix
> > it by computing offsets using the stride reported by V4L2.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 88535f5a07c7..c6c9263c49e9 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1354,11 +1354,21 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> >  		size_t offset = 0;
> >  
> >  		for (size_t i = 0; i < planes.size(); ++i) {
> > +			/*
> > +			 * The stride is reported by V4L2 for the first plane
> > +			 * only. Compute the stride of the other planes by
> > +			 * taking the horizontal subsampling factor into
> > +			 * account, which is equal to the bytesPerGroup ratio of
> > +			 * the planes.
> > +			 */
> 
> Yikes, this feels like it might be fragile, but I can't see where it
> would break yet so ... it sounds ok I think.

That's the rule that V4L2 mandates, so I think it should be fine.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +			unsigned int stride = format_.planes[0].bpl
> > +					    * formatInfo_->planes[i].bytesPerGroup
> > +					    / formatInfo_->planes[0].bytesPerGroup;
> > +
> >  			planes[i].fd = fd;
> >  			planes[i].offset = offset;
> > -
> > -			/* \todo Take the V4L2 stride into account */
> > -			planes[i].length = formatInfo_->planeSize(format_.size, i);
> > +			planes[i].length = formatInfo_->planeSize(format_.size.height,
> > +								  i, stride);
> >  			offset += planes[i].length;
> >  		}
> >  	}

-- 
Regards,

Laurent Pinchart