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

[Jacopo Mondi]

Hi Paul,

On Thu, Jun 18, 2020 at 02:24:25PM +0900, Paul Elder wrote:
> 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.
>

ahah, I was suggesting to use "format" and "sizes" in place of
argFormat and frameSizes :)

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

As Laurent pointed out, this is a best effort support for existing
applications, and they likely are not interested in raw formats or
advanced capture modes, so I think it's fine!

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

I guess the last question was the relevant one, but Laurent has
reported that an invalid format is tested with:
         ret = testEnumFrameSizes(node, 0x20202020);

So I think we're good with reporting EINVAL here

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

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