[libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve debugging when buffer is too small

[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 at 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 at ideasonboard.com>
> 
> >                 }
> >         } else if (multiPlanar) {
> >                 /*
> > 

-- 
Regards,

Laurent Pinchart