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

[Kieran Bingham]

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 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.


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