[libcamera-devel] [PATCH] pipeline: rpi: Respect provided stride

[Laurent Pinchart]

Hi William,

On Tue, Dec 12, 2023 at 01:24:52PM +0000, William Vinnicombe wrote:
> On Tue, 12 Dec 2023 at 10:08, Laurent Pinchart wrote:
> > On Tue, Dec 12, 2023 at 09:56:22AM +0000, David Plowman via libcamera-devel wrote:
> > > On Mon, 11 Dec 2023 at 17:14, William Vinnicombe via libcamera-devel wrote:
> > > >
> > > > When converting from StreamConfiguration to V4L2DeviceFormat, the stride
> > > > was being dropped.
> > >
> > > I might add a "with the result that users could not request a custom
> > > stride" (a bit like playing consequences!) just for the extra
> > > clarification... but with or without that it looks good to me:
> >
> > That looks like a nice addition, I can update the commit message when
> > pushing if a v2 is not needed.
> 
> That sounds good, thanks
> 
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > >
> > > > Set the stride in the V4L2DeviceFormat to prevent this happening.
> > > >
> > > > Signed-off-by: William Vinnicombe <william.vinnicombe at raspberrypi.com>
> > > > ---
> > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > index 9f788c9d..5afa8dbb 100644
> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > @@ -367,6 +367,7 @@ V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
> > > >         deviceFormat.planesCount = info.numPlanes();
> > > >         deviceFormat.fourcc = dev->toV4L2PixelFormat(stream->pixelFormat);
> > > >         deviceFormat.size = stream->size;
> > > > +       deviceFormat.planes[0].bpl = stream->stride;
> >
> > Shouldn't the stride be set for all planes ?
> 
> As there isn't any per-plane stride information in the stream
> configuration, and our pipeline handler only reads the stride from the
> first plane, I think it's safer to only set the stride for the first
> plane in this configuration.

>From a libcamera API point of view, we've opted for a single stride
value related to the first plane, with the stride of the other planes
derived from the first plane. The rationale is that not all hardware
platforms would allow configuration of the stride per-plane, and we
haven't come across use cases for per-plane stride values.

>From a V4L2 point of view, however, the stride is defined per plane.
Unless I'm mistaken, Pi 4 supports the single planar API only (formats
with multiple planes, such as NV12, use a single V4L2 plane to store all
the planes), but Pi 5 uses the V4L2 multi planar API. As this patch is
for common code, I think it would be best to initialize all the planes
correctly instead of relying on the kernel driver to fix the values.

> > > >         deviceFormat.colorSpace = stream->colorSpace;
> > > >
> > > >         return deviceFormat;

-- 
Regards,

Laurent Pinchart