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).
M.
Regards,
Hans
