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

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

-- 
Regards,

Laurent Pinchart