[libcamera-devel] [PATCH 17/30] libcamera: v4l2_videodevice: Add support for multi plane output buffers

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 02, 2019 at 11:12:02AM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2019 at 12:36:07AM +0100, Niklas Söderlund wrote:
> > When queuing an output buffer it was assumed it only had one plane. With
> > the recent rework of the buffer code it's now trivial to add support
> > multi plane output buffers, add support for it.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 8f962c7e9d0c7d01..a05dd6a1f7d86eaa 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1014,7 +1014,17 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> >  	if (V4L2_TYPE_IS_OUTPUT(buf.type)) {
> >  		const BufferInfo &info = buffer->info();
> >
> > -		buf.bytesused = info.planes()[0].bytesused;
> > +		if (multiPlanar) {
> > +			unsigned int nplane = 0;
> > +			for (const BufferInfo::Plane &plane : info.planes()) {
> > +				v4l2Planes[nplane].bytesused = plane.bytesused;
> 
> Nit: if you nplane++ here you can save the 2 following lines
> 
> > +				nplane++;
> > +			}
> > +		} else {
> > +			if (info.planes().size())
> 
> Can we have 0 planes?
> 
> > +				buf.bytesused = info.planes()[0].bytesused;
> > +		}
> > +
> 
> All nits
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Additionally, the subject line should mention that this covers queuing
buffers only.

"libcamera: v4l2_videodevice: Add support for queuing multi plane output buffers"

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

But I think you should also handle V4L2VideoDevice::dequeueBuffer(),
either here or in a separate patch.

> >  		buf.sequence = info.sequence();
> >  		buf.timestamp.tv_sec = info.timestamp() / 1000000000;
> >  		buf.timestamp.tv_usec = (info.timestamp() / 1000) % 1000000;

-- 
Regards,

Laurent Pinchart