[libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy: Implement VIDIOC_ENUM_FRAMESIZES

[Laurent Pinchart]

Hi Jacopo,

On Wed, Jun 17, 2020 at 04:08:39PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 16, 2020 at 10:12:35PM +0900, Paul Elder wrote:
> > Implement VIDIOC_ENUM_FRAMESIZES in the V4L2 compatibility layer.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 24 ++++++++++++++++++++++++
> >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 6c0dacc..a5fa478 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -251,6 +251,27 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)
> >  	return 0;
> >  }
> >
> > +int V4L2CameraProxy::vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_framesizes fd = " << fd;
> > +
> > +	if (arg == nullptr)
> > +		return -EFAULT;
> > +
> > +	PixelFormat argFormat = v4l2ToDrm(arg->pixel_format);
>  just format ?
> 
> > +	const std::vector<Size> &frameSizes = streamConfig_.formats().sizes(argFormat);
> 
> just sizes ?
> 
> Anyway, very few pipeline handlers report StreamFormats at the moment,
> I would record this with a \todo entry.
> 
> Another point is that the streamConfig_ you here use to enumerate formats is
> generated using the Viewfinder role only. In this case, I expect
> pipeline handlers not to report any entry for, for example, raw
> formats, while the Camera actually supports it.

Is that a problem though ? The V4L2 compat layer is meant to support
existing V4L2 applications that are not yet ported to libcamera (or that
can't easily be ported, for instance because they're not open-source),
on a best effort basis. I would not expect such applications to really
have a use for raw formats. I think the viewfinder role makes sense as a
best-effort implementation.

> There's no easy way around this I'm afraid, or at least I don't see an
> easy one. I had to do a similar thing for Android, as you can see in
> src/android/camera_device.cpp:243
> 
> There I decided to create a list of image resolutions, and test
> them against the application provided pixel format, by using
> CameraConfiguration::validate(). This probably won't scale here, as
> the list of resolution would be quite arbitrary, while in Android I
> have a specification to follow which dictates which resolution are
> mandatory to be supported.
> 
> Generating multiple CameraConfiguration (one for each supported role)
> and test all of them until we don't find on which reports the desired
> image format is another option, but would require you to introduce an
> API to do so in your V4L2Camera class. In both cases, a bit of rework
> is required. What do you think ?
> 
> > +
> > +	if (arg->index >= frameSizes.size())
> > +		return -EINVAL;
> 
> That's fine, as if the format is not supported StreamFormats::sizes()
> will return an empty vector, but the v4l2 specs do not say what is the
> expected behaviour in that case. Does v4l2-compliance try this call
> with an invalid format ?

	ret = testEnumFrameSizes(node, 0x20202020);
	if (ret != ENOTTY)
		return fail("Accepted framesize for invalid format\n");

and testEnumFrameSizes has a special case:

		if (f == 0 && ret == EINVAL)
			return ENOTTY;

(f being the loop index). So I think the code is fine.

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

> > +
> > +	arg->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> > +	arg->discrete.width = frameSizes[arg->index].width;
> > +	arg->discrete.height = frameSizes[arg->index].height;
> > +	memset(arg->reserved, 0, sizeof(arg->reserved));
> > +
> > +	return 0;
> > +}
> > +
> >  int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg)
> >  {
> >  	LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << fd;
> > @@ -676,6 +697,9 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)
> >  	case VIDIOC_QUERYCAP:
> >  		ret = vidioc_querycap(static_cast<struct v4l2_capability *>(arg));
> >  		break;
> > +	case VIDIOC_ENUM_FRAMESIZES:
> > +		ret = vidioc_enum_framesizes(fd, static_cast<struct v4l2_frmsizeenum *>(arg));
> > +		break;
> >  	case VIDIOC_ENUM_FMT:
> >  		ret = vidioc_enum_fmt(fd, static_cast<struct v4l2_fmtdesc *>(arg));
> >  		break;
> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > index 2e90bfd..2ff9571 100644
> > --- a/src/v4l2/v4l2_camera_proxy.h
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -45,6 +45,7 @@ private:
> >  	int freeBuffers();
> >
> >  	int vidioc_querycap(struct v4l2_capability *arg);
> > +	int vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg);
> >  	int vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg);
> >  	int vidioc_g_fmt(int fd, struct v4l2_format *arg);
> >  	int vidioc_s_fmt(int fd, struct v4l2_format *arg);

-- 
Regards,

Laurent Pinchart