| Message ID | 20211014085405.3795243-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. 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@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 ? > unsigned int bytesused = multiPlanar ? planes[0].bytesused > : buf.bytesused; >
Quoting Laurent Pinchart (2021-10-14 21:12:18) > Hi Kieran, > > Thank you for the patch. > > 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@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. -- Kieran > > > unsigned int bytesused = multiPlanar ? planes[0].bytesused > > : buf.bytesused; > > > > -- > Regards, > > 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@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@ideasonboard.com> > > > unsigned int bytesused = multiPlanar ? planes[0].bytesused > > > : buf.bytesused; > > >
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 */ unsigned int bytesused = multiPlanar ? planes[0].bytesused : buf.bytesused;
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@ideasonboard.com> --- src/libcamera/v4l2_videodevice.cpp | 1 + 1 file changed, 1 insertion(+)