[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: use G_FORMAT if ENUM_FRAME_SIZE isn't implemented
Andrey Konovalov
andrey.konovalov at linaro.org
Tue Nov 12 21:22:04 CET 2019
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
>> 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.
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?
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.
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.
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)?
>> 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
More information about the libcamera-devel
mailing list