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

[Kieran Bingham]

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

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