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

2365 diff mbox series

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@ragnatech.se>
---
 src/libcamera/v4l2_videodevice.cpp | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Hi Niklas,

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@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@jmondi.org>

Thanks
   j
>  		buf.sequence = info.sequence();
>  		buf.timestamp.tv_sec = info.timestamp() / 1000000000;
>  		buf.timestamp.tv_usec = (info.timestamp() / 1000) % 1000000;
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@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@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;