[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: use G_FORMAT if ENUM_FRAME_SIZE isn't implemented
Jacopo Mondi
jacopo at jmondi.org
Fri Nov 15 10:38:19 CET 2019
Hi Andrey,
On Tue, Nov 12, 2019 at 11:22:04PM +0300, Andrey Konovalov wrote:
> Hi Jacopo,
>
> On 09.11.2019 13:50, Jacopo Mondi wrote:
> > Hello Andrey,
> > thanks for the patch
> >
> > On Fri, Oct 25, 2019 at 04:32:08PM +0300, Andrey Konovalov wrote:
> > > Many camera sensor drivers support only one frame size, and don't
> > > implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl. And libcamera fails
>
> The key point here is VIDIOC_SUBDEV_ENUM_FRAME_SIZE support (see below)
>
> > Is this true ? A single frame size?
> Well, not quite
>
> > Do you see this often ?
>
> I do see this as one of the cameras I currently use is ov5647, and the mainline driver for it supports 640x480 only :)
> (and this might explain why it doesn't implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE).
>
> But there are quite a few sensor drivers which don't support
> VIDIOC_SUBDEV_ENUM_FRAME_SIZE even though they support several frame sizes.
> I put them into two groups:
>
> 1) Have a small set of fixed sizes:
> drivers/media/i2c/ov5647.c (only have 640x480) - 5MP
> drivers/media/i2c/imx274.c (3 fixed frame sizes)
> drivers/media/i2c/noon010pc30.c (3 fixed frame sizes)
> drivers/media/i2c/sr030pc30.c (3 fixed resolutions - 640x480 plus 320x240 and 160x120 (x2 and x4 subsampling))
> drivers/media/i2c/tc358743.c (HDMI to CSI-2 bridge; has [set|get]_fmt, but the resolution is "fixed" to what it is getting
> from HDMI if I get it correct)
>
> 2) Use cropping to set any resolution matching the limits and the restrictions
> (e.g. height and width might have to be even numbers):
> drivers/media/i2c/mt9m001.c (any frame size from 48x32 to 1280x1024 with 2 pixels step) - 1.3 MP
> drivers/media/i2c/mt9m111.c (quite similar to the one above) - 1.3 MP
> drivers/media/i2c/mt9t112.c (similar to the one above) - 2MP
> drivers/media/i2c/mt9v011.c (uses cropping to fit to the resolution requested; doesn't use selection API)
> drivers/media/i2c/ov2640.c (uses cropping to set a set of standard resolutions; has get_selection, but not set_selection)
> drivers/media/i2c/ov6650.c (uses selection API to crop the image to fit the requested resolution;
> support half scale (QCIF) and full scale (CIF))
> drivers/media/i2c/ov7640.c (empty v4l2_subdev_ops)
> drivers/media/i2c/ov772x.c (similar to ov2640.c)
> drivers/media/i2c/ov9640.c (has a set of 7 standard resolutions; has get_selection, but not set_selection) - 1.3 MP
> drivers/media/i2c/rj54n1cb0c.c (uses cropping, and selection API)
> drivers/media/i2c/s5k4ecgx.c (4 fixed "preview" - up to 720x480 - resolutions) - 5MP
> drivers/media/i2c/s5k6a3.c (?)
> drivers/media/i2c/vs6624.c (supports quite a few resolutions) - 1.3 MP
>
Thanks for checking
> > > to enumerate such cameras as it doesn't get the list of resolutions
> > > the sensor driver supports.
>
> - here. libcamera ignores cameras which don't implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE.
> And there are quite a few of such sensors.
>
> > > Fix this by using VIDIOC_SUBDEV_G_FORMAT
> > > to get the size of the sensor's active format when the ENUM_FRAME_SIZE
> > > ioctl isn't implemented by the sensor driver.
> > >
> >
> > As a general note, personally, I would prefer drivers to implement the
> > proper ioctls, and I think we should not provide incentives to work
> > this around. But if this fixes a real issue with several drivers I
> > think we could consider this. What do others think here?
>
> Again, my real issue is that libcamera uses VIDIOC_SUBDEV_ENUM_FRAME_SIZE to get the list
> of the resolutions supported by sensor. If sensor responds to this ioctl with -ENOTTY,
> libcamera creates an empty list of supported resolutions for this sensor and refuses to use it.
> Here is the call sequence to "enumerate all media bus codes and frame sizes supported by the
> subdevice on a pad":
> V4L2Subdevice::formats() -> V4L2Subdevice::enumPadSizes() -> ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))
>
> I am not sure if implementing VIDIOC_SUBDEV_ENUM_FRAME_SIZE is a must for every camera sensor;
> maybe the most from the 2) group above don't really need it.
I would say the opposite. Most drivers in 2) shold report the min and
max resolutions, and from there you could obtain the desired one by
cropping/scaling, if I'm not mistaken.
> But then it would be good for all the camera sensors to be recognized by libcamera - whether
> they implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE or not, correct?
>
As I've said, I'm not entirely sure about this, as we by using
get_format as you proposed doesn't really give you back the same
information as ENUM_FRAME_SIZE does.
>
> On a different matter:
>
> My current feeling about VIDIOC_SUBDEV_ENUM_FRAME_SIZE is that it might be useful to distinguish
> between:
> 1) the resolutions which use the full area of the sensor, have the same field of view, but
> differ in pixel density (use binning, or subsampling),
> and 2) the lower resolutions obtained by cropping when only a part of the sensor is used.
> In this sense e.g. mt9t112.c supports single frame size of 2048x1536, and can use cropping to
> provide any resolution below 2048x1536.
what you're here hitting is limitation in the v4l2 specification and
in the average sensor driver implementation, which relies on discrete
resolutions as it can often just use a set of pre-defined register
values provided by the vendors 'known to work' for that specific mode.
Ideally, ENUM_FRAME_SIZE should report the full pixel array available
for a media bus format, and you could then get all the intermediate
resolutions from there. This does very seldom happens though.
>
> In other words, VIDIOC_SUBDEV_ENUM_FRAME_SIZE could list maximum resolutions the sensor supports
> (like "16:9 Hi Res", "4:3 Hi Res", "16:9 Low Res", etc - with the same field of view for the same
> aspect ratio regardless of Hi or Low Res). Versus a (kind of continuous) range of resolutions
> implemented by cropping which is rather a "digital zoom" in my understanding.
I feel like you should get the first one from ENUM_FRAME_SIZE and the
possible cropping/scaling rectangles through the selection APIs. I'm
not sure how many drivers actually do so.
>
> If a sensor has the resolution of e.g. 640x480, the driver and the sensor support both 2x binning and
> cropping, and the application requests the resolution of 320x240 - how the sensor driver could deduce
> if the application asks to turn the binning on, or to use the 1/4 of the sensor area (both cases are
> valid, but are different requests)?
>
There's people way more expert then me that could answer that,
personally I would say that you currently can't and it really depends
on the driver implementation to decide that.
I would suggest you to turn to linux-media and Cc Sakari, who surely
knows better than me on this topic :)
Back on libcamera releated issues: I'm not sure we can actually work
around a missing ENUM_FRAME_SIZE, and I'm not sure it could be done with
G_FMT. My suggestion at the moment would be: patch the faulty driver,
it's a simple additional subdev op that report a single value, it
shouldn't be hard. Maybe others have different opinions though (maybe
let's wait for Laurent to come back and give his opinion too).
Thanks
j
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> > > ---
> > > src/libcamera/v4l2_subdevice.cpp | 20 +++++++++++++++++++-
> > > 1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index f2bcd7f..fac7586 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -317,7 +317,25 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > > sizeEnum.max_width, sizeEnum.max_height);
> > > }
> > >
> > > - if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > > + if (ret == -ENOTTY) {
> > > + struct v4l2_subdev_format format = {};
> > > +
> > > + LOG(V4L2, Debug)
> > > + << "VIDIOC_SUBDEV_ENUM_FRAME_SIZE not implemented "
> > > + << "- trying VIDIOC_SUBDEV_G_FMT";
> > > + format.pad = pad;
> > > + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +
> > > + ret = ioctl(VIDIOC_SUBDEV_G_FMT, &format);
> > > + if (ret == 0) {
> > > + sizes.emplace_back(format.format.width,
> > > + format.format.height,
> > > + format.format.width,
> > > + format.format.height);
> >
> > However, I don't think this works... ENUM_FRAME_SIZE has a .code field
> > in struct v4l2_subdev_frame_size_enum, and enumerates sizes for that
> > specific mbus code. As you can see the 'sizes' vector this operations
> > returns is associated with said mbus code in the caller's 'formats'
> > map.
> >
> > G_FMT returns the current format regardless of the mbus code, so if we
> > want to work around faulty drivers using this approach, I would do
> > that in the caller, and adjust the 'formats' map, not only the here
> > enumerated sizes.
>
> Good point!
> I'll try something like that in v2 of this patch:
>
> -----8<-----
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -166,8 +166,13 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
>
> for (unsigned int code : enumPadCodes(pad)) {
> std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> - if (sizes.empty())
> - return {};
> + if (sizes.empty()) {
> + /* try S_FMT(which = V4L2_SUBDEV_FORMAT_TRY,
> + * format.width and format.height big enough,
> + * format.code = code)
> + * if it succeeds, add the returned data to sizes
> + * else return {}; */
> + }
>
> if (formats.addFormat(code, sizes)) {
> -----8<-----
>
> Thanks,
> Andrey
>
> > Thanks
> > j
> >
> > > + }
> > > + }
> > > +
> > > + if (ret < 0 && ret != -EINVAL) {
> > > LOG(V4L2, Error)
> > > << "Unable to enumerate sizes on pad " << pad
> > > << ": " << strerror(-ret);
> > > --
> > > 2.17.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191115/175adc48/attachment.sig>
More information about the libcamera-devel
mailing list