On 09. 01. 24 13:21, Hans Verkuil wrote:
On 1/9/24 12:46, Martin Tůma wrote:
Hi Hans,
On 09. 01. 24 11:43, Hans Verkuil wrote:
Hi Martin,
On 1/9/24 11:17, Martin Tůma wrote:
Hi,
If a driver implements VIDIOC_ENUM_FRAMESIZES for a output device, qv4l2
crashes in general-tab.cpp:2169 due to m_frameSize being 0. As all other
usages of m_frameSize in the GeneralTab::updateFrameSize() function are
guarded by "NULL checks" an obvious fix would be to guard the "addItem
while cycle" as well. But maybe a better solution would be to add the
frame size combobox to output devices as well. Or are the output devices
not supposed to have frame sizes enumeration?
You do not expect to see it for output devices. Those will typically use
VIDIOC_ENUMSTD or VIDIOC_ENUM_DV_TIMINGS. VIDIOC_ENUM_FRAMESIZES is something
that only makes sense for output devices if they do not support ENUMSTD or
ENUM_DV_TIMINGS, i.e. it has some very odd output hardware. In particular
vivid doesn't support this for the emulated video output.
I don't think v4l2-compliance has a check for this. I think it should at
least issue a warning if the video output devices supports ENUM_FRAMESIZES
and also ENUMSTD or ENUM_DV_TIMINGS.
In any case, qv4l2 shouldn't crash, so an initial fix should be to check
for m_frameSize being 0. A patch is welcome.
Ok, I will add the check, test it with my current driver (see below)
that triggers it and send a patch.
Which driver is this? I assume it is an out-of-tree driver, since I am
not aware of any mainline drivers that do this.
I came across this when fiddling with our mgb4 driver. We have updated
the FW to support YUV in addition to RGB and added independent timers
for frame dropping (the current driver is "wrong" as it provides
VIDIOC_G_PARM/VIDIOC_S_PARM with V4L2_CAP_TIMEPERFRAME but can not in
fact set it with the original FW). The inputs in mgb4 have
ENUM_FRAMESIZES and a request from a card user to add the callback to
the outputs to be "symmetrical" with the inputs.
From what you wrote above I get the message that the "proper" way how
to extend the output callbacks is rather to add ENUM_DV_TIMINGS (which
is also missing, the driver implements it only for the inputs at the
moment), but the question "what to do with ENUM_FRAMESIZES?" stays. I
would like to either have it implemented for both the inputs/outputs or
for none of them. And adding the callback to the output looks to me as a
better solution than to remove it from the input as some people already
use it. But to call this a bug and remove it is also ok for me (the
driver will at least work with the current, unpatched, version of qv4l2).
Your hardware is rather odd :-) so let me describe how it is supposed
to work for 'normal' capture hardware first:
It is odd, but I'm trying to make it usable as a standard v4l2 device ;-)
ENUM_FRAMESIZES is typically only supported for sensor inputs since sensors
usually have a discrete set of sizes that they support. And you also need
that since you typically explicitly set the size that you want from the
sensor, so you need to know what is supported.
For video capture it is different: the video source typically decides what
size to transmit. For analog video through e.g. S-Video that's typically
PAL or NTSC, so for that you need ENUMSTD and QUERYSTD ioctls to handle that.
For e.g. HDMI or similar interfaces it is the DV Timings API that you need.
In both cases you do not have any control over what video size the source
will transmit, you have to detect it with the QUERYSTD or QUERY_DV_TIMINGS
ioctls.
So ENUM_FRAMESIZES makes no sense for standard analog or digital video
capture devices.
For output video it is similar: for analog video you just set the standard
(e.g. PAL or NTSC), and that defines the size automatically. For e.g. HDMI
you set it as well based on the capabilities reported by the display through
the EDID. Again, ENUM_FRAMESIZES makes no sense, this is driven by the
analog or digital video timings.
My goal is to separate the "unusual" part of our HW to the custom sysfs
options and from the v4l2 point of view to work as a standard HDMI
device, i.e. the sysfs stuff configurable with UDEV rules shall replace
the missing EDID and the rest shall work in a standard way including all
the QUERY/ENUM ioctls that shall report resolutions/framerates as
previously set in sysfs.
ENUM_FRAMESIZES for something that is not a sensor would only make sense
if it is some specialized interface that supports a limited set of sizes
and has no concept of PAL/NTSC or Digital Video Timings.
Ok. Then I will drop ENUM_FRAMESIZES for the inputs and implement the
*_DV_TIMINGS API for the outputs to make the outputs "symetrical" to the
inputs.
M.
Regards,
Hans
M.
Regards,
Hans
