| Message ID | 20211008101556.22987-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On 10/8/21 1:15 PM, Laurent Pinchart wrote: > When a dequeued buffer is too small, the condition is logged and an > error is returned. The logged message doesn't provide any information > about the sizes, making debugging more difficult. Improve it by logging > both the bytesused value and the length of each plane. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Compile-tested only. Eugen, could you give it a try to debug the problem > you're facing ? Tested-by: Eugen Hristev <eugen.hristev@microchip.com> Tested this patch and the information is more verbose, making it easy to debug my problem. > > --- > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index ba5f88cd41ed..bd74103de7af 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > unsigned int bytesused = multiPlanar ? planes[0].bytesused > : buf.bytesused; > + unsigned int remaining = bytesused; > > for (auto [i, plane] : utils::enumerate(buffer->planes())) { > - if (!bytesused) { > + if (!remaining) { > LOG(V4L2, Error) > - << "Dequeued buffer is too small"; > + << "Dequeued buffer (" << bytesused > + << " bytes) too small for plane lengths " > + << utils::join(buffer->planes(), "/", > + [](const FrameBuffer::Plane &p){ > + return p.length; > + }); > > metadata.status = FrameMetadata::FrameError; > return buffer; > } > > metadata.planes()[i].bytesused = > - std::min(plane.length, bytesused); > - bytesused -= metadata.planes()[i].bytesused; > + std::min(plane.length, remaining); > + remaining -= metadata.planes()[i].bytesused; > } > } else if (multiPlanar) { > /* > > base-commit: 962df634bd0afe12e6f38464f5e602cf1460c430 > -- > Regards, > > Laurent Pinchart >
Quoting Laurent Pinchart (2021-10-08 11:15:56) > When a dequeued buffer is too small, the condition is logged and an > error is returned. The logged message doesn't provide any information > about the sizes, making debugging more difficult. Improve it by logging > both the bytesused value and the length of each plane. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Compile-tested only. Eugen, could you give it a try to debug the problem > you're facing ? > > --- > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index ba5f88cd41ed..bd74103de7af 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > unsigned int bytesused = multiPlanar ? planes[0].bytesused > : buf.bytesused; > + unsigned int remaining = bytesused; > > for (auto [i, plane] : utils::enumerate(buffer->planes())) { > - if (!bytesused) { > + if (!remaining) { > LOG(V4L2, Error) > - << "Dequeued buffer is too small"; > + << "Dequeued buffer (" << bytesused > + << " bytes) too small for plane lengths " > + << utils::join(buffer->planes(), "/", > + [](const FrameBuffer::Plane &p){ > + return p.length; > + }); > > metadata.status = FrameMetadata::FrameError; > return buffer; > } > > metadata.planes()[i].bytesused = > - std::min(plane.length, bytesused); > - bytesused -= metadata.planes()[i].bytesused; > + std::min(plane.length, remaining); > + remaining -= metadata.planes()[i].bytesused; Should this prevent underflowing 'remaining' - or otherwise, make remaining a signed int, and check against if (remaining <= 0) above instead. I'd prefer in this context to use the signed value and make sure we clearly catch negative remaining.. With that, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > } else if (multiPlanar) { > /* > > base-commit: 962df634bd0afe12e6f38464f5e602cf1460c430 > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Tue, Oct 12, 2021 at 02:01:10PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2021-10-08 11:15:56) > > When a dequeued buffer is too small, the condition is logged and an > > error is returned. The logged message doesn't provide any information > > about the sizes, making debugging more difficult. Improve it by logging > > both the bytesused value and the length of each plane. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Compile-tested only. Eugen, could you give it a try to debug the problem > > you're facing ? > > > > --- > > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index ba5f88cd41ed..bd74103de7af 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > > unsigned int bytesused = multiPlanar ? planes[0].bytesused > > : buf.bytesused; > > + unsigned int remaining = bytesused; > > > > for (auto [i, plane] : utils::enumerate(buffer->planes())) { > > - if (!bytesused) { > > + if (!remaining) { > > LOG(V4L2, Error) > > - << "Dequeued buffer is too small"; > > + << "Dequeued buffer (" << bytesused > > + << " bytes) too small for plane lengths " > > + << utils::join(buffer->planes(), "/", > > + [](const FrameBuffer::Plane &p){ > > + return p.length; > > + }); > > > > metadata.status = FrameMetadata::FrameError; > > return buffer; > > } > > > > metadata.planes()[i].bytesused = > > - std::min(plane.length, bytesused); > > - bytesused -= metadata.planes()[i].bytesused; > > + std::min(plane.length, remaining); > > + remaining -= metadata.planes()[i].bytesused; > > Should this prevent underflowing 'remaining' - or otherwise, make > remaining a signed int, and check against if (remaining <= 0) above > instead. > > I'd prefer in this context to use the signed value and make sure we > clearly catch negative remaining.. It's not just about overflows, it's also there to support variable-length payloads. There's a comment above the code that explains it: /* * If we have a multi-planar buffer with a V4L2 * single-planar format, split the V4L2 buffer across * the buffer planes. Only the last plane may have less * bytes used than its length. */ It's probably not a very common use case as we mostly deal with fixed-size payloads, but it doesn't cost much to keep it. Let's also note that we will still need to calculate metadata.planes()[i].bytesused as std::min(plane.length, remaining) in either case, so I don't really see what we would gain by decrementing remaining with remaining -= plane.length; instead. > With that, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > } > > } else if (multiPlanar) { > > /* > >
Quoting Laurent Pinchart (2021-10-12 20:50:43) > Hi Kieran, > > On Tue, Oct 12, 2021 at 02:01:10PM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2021-10-08 11:15:56) > > > When a dequeued buffer is too small, the condition is logged and an > > > error is returned. The logged message doesn't provide any information > > > about the sizes, making debugging more difficult. Improve it by logging > > > both the bytesused value and the length of each plane. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Compile-tested only. Eugen, could you give it a try to debug the problem > > > you're facing ? > > > > > > --- > > > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++---- > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index ba5f88cd41ed..bd74103de7af 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > even with the banner comment at the top of this conditional, I think we should add something here as it's too easy to think this is a bug... (I'll send a separate patch): /* Single planar format stored in a multiplanar buffer */ > > > unsigned int bytesused = multiPlanar ? planes[0].bytesused > > > : buf.bytesused; > > > + unsigned int remaining = bytesused; > > > > > > for (auto [i, plane] : utils::enumerate(buffer->planes())) { > > > - if (!bytesused) { > > > + if (!remaining) { > > > LOG(V4L2, Error) > > > - << "Dequeued buffer is too small"; > > > + << "Dequeued buffer (" << bytesused > > > + << " bytes) too small for plane lengths " > > > + << utils::join(buffer->planes(), "/", > > > + [](const FrameBuffer::Plane &p){ > > > + return p.length; > > > + }); > > > > > > metadata.status = FrameMetadata::FrameError; > > > return buffer; > > > } > > > > > > metadata.planes()[i].bytesused = > > > - std::min(plane.length, bytesused); > > > - bytesused -= metadata.planes()[i].bytesused; > > > + std::min(plane.length, remaining); > > > + remaining -= metadata.planes()[i].bytesused; > > > > Should this prevent underflowing 'remaining' - or otherwise, make > > remaining a signed int, and check against if (remaining <= 0) above > > instead. > > > > I'd prefer in this context to use the signed value and make sure we > > clearly catch negative remaining.. > > It's not just about overflows, it's also there to support > variable-length payloads. There's a comment above the code that explains > it: > > /* > * If we have a multi-planar buffer with a V4L2 > * single-planar format, split the V4L2 buffer across > * the buffer planes. Only the last plane may have less > * bytes used than its length. > */ > > It's probably not a very common use case as we mostly deal with > fixed-size payloads, but it doesn't cost much to keep it. > > Let's also note that we will still need to calculate > metadata.planes()[i].bytesused as std::min(plane.length, remaining) in > either case, so I don't really see what we would gain by decrementing > remaining with > > remaining -= plane.length; > > instead. My concern is in subtracting values from an external source (kernels can have bugs), from an unsigned int, and guaranteeing that it will never underflow. Which would mean "if (!remaining)" would not be able to catch the issue. If you can guarantee that there is no possibility for a kernel bug here, then fine, but otherwise, I think a signed int is safer code. > > With that, > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > } > > > } else if (multiPlanar) { > > > /* > > > > > -- > Regards, > > Laurent Pinchart
Hi Kieran, On Thu, Oct 14, 2021 at 09:54:39AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2021-10-12 20:50:43) > > On Tue, Oct 12, 2021 at 02:01:10PM +0100, Kieran Bingham wrote: > > > Quoting Laurent Pinchart (2021-10-08 11:15:56) > > > > When a dequeued buffer is too small, the condition is logged and an > > > > error is returned. The logged message doesn't provide any information > > > > about the sizes, making debugging more difficult. Improve it by logging > > > > both the bytesused value and the length of each plane. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > Compile-tested only. Eugen, could you give it a try to debug the problem > > > > you're facing ? > > > > > > > > --- > > > > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++---- > > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > index ba5f88cd41ed..bd74103de7af 100644 > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > > > > even with the banner comment at the top of this conditional, I think we > should add something here as it's too easy to think this is a bug... > (I'll send a separate patch): > > /* Single planar format stored in a multiplanar buffer */ > > > > unsigned int bytesused = multiPlanar ? planes[0].bytesused > > > > : buf.bytesused; > > > > + unsigned int remaining = bytesused; > > > > > > > > for (auto [i, plane] : utils::enumerate(buffer->planes())) { > > > > - if (!bytesused) { > > > > + if (!remaining) { > > > > LOG(V4L2, Error) > > > > - << "Dequeued buffer is too small"; > > > > + << "Dequeued buffer (" << bytesused > > > > + << " bytes) too small for plane lengths " > > > > + << utils::join(buffer->planes(), "/", > > > > + [](const FrameBuffer::Plane &p){ > > > > + return p.length; > > > > + }); > > > > > > > > metadata.status = FrameMetadata::FrameError; > > > > return buffer; > > > > } > > > > > > > > metadata.planes()[i].bytesused = > > > > - std::min(plane.length, bytesused); > > > > - bytesused -= metadata.planes()[i].bytesused; > > > > + std::min(plane.length, remaining); > > > > + remaining -= metadata.planes()[i].bytesused; > > > > > > Should this prevent underflowing 'remaining' - or otherwise, make > > > remaining a signed int, and check against if (remaining <= 0) above > > > instead. > > > > > > I'd prefer in this context to use the signed value and make sure we > > > clearly catch negative remaining.. > > > > It's not just about overflows, it's also there to support > > variable-length payloads. There's a comment above the code that explains > > it: > > > > /* > > * If we have a multi-planar buffer with a V4L2 > > * single-planar format, split the V4L2 buffer across > > * the buffer planes. Only the last plane may have less > > * bytes used than its length. > > */ > > > > It's probably not a very common use case as we mostly deal with > > fixed-size payloads, but it doesn't cost much to keep it. > > > > Let's also note that we will still need to calculate > > metadata.planes()[i].bytesused as std::min(plane.length, remaining) in > > either case, so I don't really see what we would gain by decrementing > > remaining with > > > > remaining -= plane.length; > > > > instead. > > My concern is in subtracting values from an external source (kernels can > have bugs), from an unsigned int, and guaranteeing that it will never > underflow. Which would mean "if (!remaining)" would not be able to catch > the issue. metadata.planes()[i].bytesused = std::min(plane.length, remaining); If the kernel doesn't give us enough data, remaining will be smaller than plane.length. metadata.planes()[i].bytesused will thus be equal to remaining. remaining -= metadata.planes()[i].bytesused; And here remaining will become zero. I don't see where the issue is. > If you can guarantee that there is no possibility for a kernel bug here, > then fine, but otherwise, I think a signed int is safer code. > > > > > With that, > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > } > > > > } else if (multiPlanar) { > > > > /* > > > >
Quoting Laurent Pinchart (2021-10-14 10:48:28) > Hi Kieran, > > On Thu, Oct 14, 2021 at 09:54:39AM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2021-10-12 20:50:43) > > > On Tue, Oct 12, 2021 at 02:01:10PM +0100, Kieran Bingham wrote: > > > > Quoting Laurent Pinchart (2021-10-08 11:15:56) > > > > > When a dequeued buffer is too small, the condition is logged and an > > > > > error is returned. The logged message doesn't provide any information > > > > > about the sizes, making debugging more difficult. Improve it by logging > > > > > both the bytesused value and the length of each plane. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > Compile-tested only. Eugen, could you give it a try to debug the problem > > > > > you're facing ? > > > > > > > > > > --- > > > > > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++---- > > > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > > index ba5f88cd41ed..bd74103de7af 100644 > > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > > > > > > > even with the banner comment at the top of this conditional, I think we > > should add something here as it's too easy to think this is a bug... > > (I'll send a separate patch): > > > > /* Single planar format stored in a multiplanar buffer */ > > > > > unsigned int bytesused = multiPlanar ? planes[0].bytesused > > > > > : buf.bytesused; > > > > > + unsigned int remaining = bytesused; > > > > > > > > > > for (auto [i, plane] : utils::enumerate(buffer->planes())) { > > > > > - if (!bytesused) { > > > > > + if (!remaining) { > > > > > LOG(V4L2, Error) > > > > > - << "Dequeued buffer is too small"; > > > > > + << "Dequeued buffer (" << bytesused > > > > > + << " bytes) too small for plane lengths " > > > > > + << utils::join(buffer->planes(), "/", > > > > > + [](const FrameBuffer::Plane &p){ > > > > > + return p.length; > > > > > + }); > > > > > > > > > > metadata.status = FrameMetadata::FrameError; > > > > > return buffer; > > > > > } > > > > > > > > > > metadata.planes()[i].bytesused = > > > > > - std::min(plane.length, bytesused); > > > > > - bytesused -= metadata.planes()[i].bytesused; > > > > > + std::min(plane.length, remaining); > > > > > + remaining -= metadata.planes()[i].bytesused; > > > > > > > > Should this prevent underflowing 'remaining' - or otherwise, make > > > > remaining a signed int, and check against if (remaining <= 0) above > > > > instead. > > > > > > > > I'd prefer in this context to use the signed value and make sure we > > > > clearly catch negative remaining.. > > > > > > It's not just about overflows, it's also there to support > > > variable-length payloads. There's a comment above the code that explains > > > it: > > > > > > /* > > > * If we have a multi-planar buffer with a V4L2 > > > * single-planar format, split the V4L2 buffer across > > > * the buffer planes. Only the last plane may have less > > > * bytes used than its length. > > > */ > > > > > > It's probably not a very common use case as we mostly deal with > > > fixed-size payloads, but it doesn't cost much to keep it. > > > > > > Let's also note that we will still need to calculate > > > metadata.planes()[i].bytesused as std::min(plane.length, remaining) in > > > either case, so I don't really see what we would gain by decrementing > > > remaining with > > > > > > remaining -= plane.length; > > > > > > instead. > > > > My concern is in subtracting values from an external source (kernels can > > have bugs), from an unsigned int, and guaranteeing that it will never > > underflow. Which would mean "if (!remaining)" would not be able to catch > > the issue. > > metadata.planes()[i].bytesused = > std::min(plane.length, remaining); > > If the kernel doesn't give us enough data, remaining will be smaller > than plane.length. metadata.planes()[i].bytesused will thus be equal to > remaining. > > remaining -= metadata.planes()[i].bytesused; > > And here remaining will become zero. I don't see where the issue is. The issue was me believing that the .bytesused values were coming from the kernel, and that there could be more than one value coming from there. Now that I see more context after applying the patch manually, (Is there not a better way to see more context?) I can see that we can only enter into this statement if numV4l2Planes == 1, so there can be only one plane from the perspective of the kernel anyway. I'll keep the onus on you though. As long as you can guarantee there is no way for 'remaining' to be subtracted below zero you can have a Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> :-) > > > If you can guarantee that there is no possibility for a kernel bug here, > > then fine, but otherwise, I think a signed int is safer code. > > > > > > > > With that, > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > } > > > > > } else if (multiPlanar) { > > > > > /* > > > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index ba5f88cd41ed..bd74103de7af 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() unsigned int bytesused = multiPlanar ? planes[0].bytesused : buf.bytesused; + unsigned int remaining = bytesused; for (auto [i, plane] : utils::enumerate(buffer->planes())) { - if (!bytesused) { + if (!remaining) { LOG(V4L2, Error) - << "Dequeued buffer is too small"; + << "Dequeued buffer (" << bytesused + << " bytes) too small for plane lengths " + << utils::join(buffer->planes(), "/", + [](const FrameBuffer::Plane &p){ + return p.length; + }); metadata.status = FrameMetadata::FrameError; return buffer; } metadata.planes()[i].bytesused = - std::min(plane.length, bytesused); - bytesused -= metadata.planes()[i].bytesused; + std::min(plane.length, remaining); + remaining -= metadata.planes()[i].bytesused; } } else if (multiPlanar) { /*
When a dequeued buffer is too small, the condition is logged and an error is returned. The logged message doesn't provide any information about the sizes, making debugging more difficult. Improve it by logging both the bytesused value and the length of each plane. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Compile-tested only. Eugen, could you give it a try to debug the problem you're facing ? --- src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) base-commit: 962df634bd0afe12e6f38464f5e602cf1460c430