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

[Paul Elder]

Hi Jacopo,

Thanks for the review.

On Wed, Jun 17, 2020 at 04:08:39PM +0200, Jacopo Mondi wrote:
> Hi Paul,
> 
> 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 ?

I don't understand; what's wrong with just format and just sizes? The
only two input parameters to enum_framesizes are index and pixel_format.

> Anyway, very few pipeline handlers report StreamFormats at the moment,
> I would record this with a \todo entry.

Oh, okay.

> 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.
> 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

Yeah :/ I'd rather not.

> 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 ?

I can't keep what I already have here? tbh I just added this as a
convenience for applications where the user can explicitly choose the
format, like qv4l2, and not for applications that choose it
automatically, like firefox and skype. So in the absence of this ioctl,
applications would just go with the usual g/s_fmt negotiation.

> > +
> > +	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

Yeah, that was my intention.

> expected behaviour in that case. Does v4l2-compliance try this call
> with an invalid format ?


Thanks,

Paul

> > +
> > +	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);
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel