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

[Naushir Patuck]

Hi Laurent,

On Mon, 18 Dec 2023 at 03:50, Laurent Pinchart via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> 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.

Pi 5 supports the multiplanar API, but we still only use single-plane
formats within it.  Lots of things would need to be fixed up if we
switched to use true multiplanar buffers.

Regards,
Naush


>
> > > > >         deviceFormat.colorSpace = stream->colorSpace;
> > > > >
> > > > >         return deviceFormat;
>
> --
> Regards,
>
> Laurent Pinchart