[libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Explain multiplanar bytesused reasoning

[Laurent Pinchart]

Hi Kieran,

On Fri, Oct 15, 2021 at 11:57:14AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-10-14 21:12:18)
> > On Thu, Oct 14, 2021 at 09:54:05AM +0100, Kieran Bingham wrote:
> > > The ternary operation used to get the total bytesused of a V4L2 single
> > > planar format which is stored in a multiplanar buffer can easily be
> > > mis-read to think it's a bug, and appears to be reading the value of the
> > > first of N planes as the total.
> > > 
> > > Directly explain the reasoning for why it looks like the condition is
> > > inverted, as it is correct that the total bytes used is stored in only
> > > the first plane of the multiplanar buffer.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > >  src/libcamera/v4l2_videodevice.cpp | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index ba5f88cd41ed..f4752f8bb23f 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1711,6 +1711,7 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > >                       return buffer;
> > >               }
> > >  
> > > +             /* Single planar format stored in a multiplanar buffer */
> > 
> > Now I find this a bit confusing :-) How about the following ?
> > 
> >                 /*
> >                  * With a V4L2 single-planar format, all the data is stored in a
> >                  * single memory plane. The number of bytes used is conveyed
> >                  * through that plane when using the V4L2 multi-planar API, or
> >                  * set directly in the buffer when using the V4L2 single-planar
> >                  * API.
> >                  */
> > 
> > It's a bit verbose perhaps ?
> 
> Perhaps, but I don't mind verbose there.
> The line on it's own looks like there is a bug because of the inversion.
> So it warrants an explanation. And if you've written one - we can add
> it.

If you're fine with that text,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > >               unsigned int bytesused = multiPlanar ? planes[0].bytesused
> > >                                      : buf.bytesused;
> > >  

-- 
Regards,

Laurent Pinchart