| Message ID | 20231211171406.37827-1-william.vinnicombe@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 577e0c6b7659f24c04476e675ea85cb666a87764 |
| Headers | show |
| Series |
|
| Related | show |
Hi William, Thank you for this fix. On Mon, 11 Dec 2023 at 17:14, William Vinnicombe via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > When converting from StreamConfiguration to V4L2DeviceFormat, the stride > was being dropped. > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > Signed-off-by: William Vinnicombe <william.vinnicombe@raspberrypi.com> Reviewed-by: Naushir Patuck <naush@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; > deviceFormat.colorSpace = stream->colorSpace; > > return deviceFormat; > -- > 2.39.2 >
Hi William Thanks for the fix. On Mon, 11 Dec 2023 at 17:14, William Vinnicombe via libcamera-devel <libcamera-devel@lists.libcamera.org> 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: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > Signed-off-by: William Vinnicombe <william.vinnicombe@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; > deviceFormat.colorSpace = stream->colorSpace; > > return deviceFormat; > -- > 2.39.2 >
Hello, 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. > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > > > Signed-off-by: William Vinnicombe <william.vinnicombe@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 ? > > deviceFormat.colorSpace = stream->colorSpace; > > > > return deviceFormat;
Hi Laurent, On Tue, 12 Dec 2023 at 10:08, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > 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@raspberrypi.com> > > > > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > > > > > Signed-off-by: William Vinnicombe <william.vinnicombe@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. Thanks, William > > > > > deviceFormat.colorSpace = stream->colorSpace; > > > > > > return deviceFormat; > > -- > Regards, > > 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@raspberrypi.com> > > > > > > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > > > > > > > Signed-off-by: William Vinnicombe <william.vinnicombe@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;
Hi Laurent, On Mon, 18 Dec 2023 at 03:50, Laurent Pinchart via libcamera-devel <libcamera-devel@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@raspberrypi.com> > > > > > > > > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > > > > > > > > > Signed-off-by: William Vinnicombe <william.vinnicombe@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
Hi Naush, On Mon, Dec 18, 2023 at 08:52:33AM +0000, Naushir Patuck wrote: > On Mon, 18 Dec 2023 at 03:50, Laurent Pinchart via libcamera-devel wrote: > > 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@raspberrypi.com> > > > > > > > > > > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > > > > > > > > > > > Signed-off-by: William Vinnicombe <william.vinnicombe@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. I may be missing something, but aren't these formats multi-planar https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be_formats.h#L127 ? > > > > > > deviceFormat.colorSpace = stream->colorSpace; > > > > > > > > > > > > return deviceFormat;
Hi Laurent, On Thu, 21 Dec 2023 at 15:43, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Mon, Dec 18, 2023 at 08:52:33AM +0000, Naushir Patuck wrote: > > On Mon, 18 Dec 2023 at 03:50, Laurent Pinchart via libcamera-devel wrote: > > > 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@raspberrypi.com> > > > > > > > > > > > > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > > > > > > > > > > > > > Signed-off-by: William Vinnicombe <william.vinnicombe@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. > > I may be missing something, but aren't these formats multi-planar > https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be_formats.h#L127 > ? The kernel driver supports them, but unless I am mistaken libcamera will only ever use the single-plane variants for the pisp/vc4 pipeline handlers. Regards, Naush > > > > > > > > deviceFormat.colorSpace = stream->colorSpace; > > > > > > > > > > > > > > return deviceFormat; > > -- > Regards, > > Laurent Pinchart
On Thu, Dec 21, 2023 at 03:57:35PM +0000, Naushir Patuck wrote: > On Thu, 21 Dec 2023 at 15:43, Laurent Pinchart wrote: > > On Mon, Dec 18, 2023 at 08:52:33AM +0000, Naushir Patuck wrote: > > > On Mon, 18 Dec 2023 at 03:50, Laurent Pinchart via libcamera-devel wrote: > > > > 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@raspberrypi.com> > > > > > > > > > > > > > > > Set the stride in the V4L2DeviceFormat to prevent this happening. > > > > > > > > > > > > > > > > Signed-off-by: William Vinnicombe <william.vinnicombe@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. > > > > I may be missing something, but aren't these formats multi-planar > > https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be_formats.h#L127 > > ? > > The kernel driver supports them, but unless I am mistaken libcamera > will only ever use the single-plane variants for the pisp/vc4 pipeline > handlers. Ah right. OK, let's ignore it for now. It would be nice to use those formats in the future though. I'll merge this patch. > > > > > > > > deviceFormat.colorSpace = stream->colorSpace; > > > > > > > > > > > > > > > > return deviceFormat;
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; deviceFormat.colorSpace = stream->colorSpace; return deviceFormat;
When converting from StreamConfiguration to V4L2DeviceFormat, the stride was being dropped. Set the stride in the V4L2DeviceFormat to prevent this happening. Signed-off-by: William Vinnicombe <william.vinnicombe@raspberrypi.com> --- src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 1 + 1 file changed, 1 insertion(+)