Re: [Workshop-2011] RFC: V4L2 API ambiguities

[Hans de Goede <hdegoede@redhat.com>]

Hi,

<snip>

> Easy:
>
> 1) Split off the control part from videodev2.h. Controls are almost 30% of
>     videodev2.h. I think maintaining controls would be easier if they are moved
>     to e.g. linux/v4l2-controls.h which is included by videodev2.h.
>

Ack.

> 2) Currently there are three types of controls: standard controls, controls
>     that are specific to a chipset (e.g. cx2341x, mfc51) and driver-specific
>     controls. The controls of the first two types have well defined and unique
>     IDs. For driver-specific controls however there are no clear rules.
>
>     It all depends on one question: should driver-specific controls have a
>     unique control ID as well, or can they overlap with other drivers?
>
>     If the answer is that they should be unique as well, then all driver-specific
>     controls will have to be defined in a single header so you can be certain
>     that there is no overlap.
>
>     If the answer is that they may overlap, then each driver can either define
>     their controls inside their own driver, or in a driver-specific public header.
>
>     In both cases a control ID range has to be defined for such controls, to
>     ensure that they never clash with standard or chipset-specific control IDs.
>     E.g. >= V4L2_CTRL_CLASS_XXX + 0x8000 (no overlap) or + 0xf000 (overlap
>     allowed).
>
>     My preference is to allow overlap.
>

+1 for allowing overlap, and only when it is expected that some special app will
actually use the device specific controls (versus the user changing them in
a generic v4l2 control panel app), then the private controls should be defined
in a public header. If no such special app is expected, the private controls
should be defined inside a private header of the driver.

> 3) What should VIDIOC_STREAMON/OFF do if the stream is already started/stopped?
>     I believe they should do nothing and just return 0. The main reason for that
>     is it can be useful if an application can just call VIDIOC_STREAMOFF without
>     having to check whether streaming is in progress.

+1 for just returning 0

> 4) What should a driver return in TRY_FMT/S_FMT if the requested format is not
>     supported (possible behaviours include returning the currently selected format
>     or a default format).
>
>     The spec says this: "Drivers should not return an error code unless the input
>     is ambiguous", but it does not explain what constitutes an ambiguous input.
>     Frankly, I can't think of any and in my opinion TRY/S_FMT should never return
>     an error other than EINVAL (if the buffer type is unsupported) or EBUSY (for
>     S_FMT if streaming is in progress).
>
>     Returning an error for any other reason doesn't help the application since
>     the app will have no way of knowing what to do next.
>

Ack on not returning an error for requesting an unavailable format. As for what the
driver should do (default versus current format) I've no preference, I vote for
letting this be decided by the driver implementation.

> 5) VIDIOC_QUERYCAP allows bus_info to be empty. Since the purpose of bus_info
>     is to distinguish multiple identical devices this makes no sense. I propose
>     to make the spec more strict and require that bus_info is always filled in
>     with a unique string.
>

Ack.

> 6) Deprecate V4L2_BUF_TYPE_PRIVATE. None of the kernel drivers use it, and I
>     cannot see any good use-case for this. If some new type of buffer is needed,
>     then that should be added instead of allowing someone to abuse this buffer
>     type.
>

Ack.

> 7) A driver that has both a video node and a vbi node (for example) that uses
>     the same struct v4l2_ioctl_ops for both nodes will have to check in e.g.
>     vidioc_g_fmt_vid_cap or vidioc_g_fmt_vbi_cap whether it is called from the
>     correct node (video or vbi) and return an error if it isn't.
>
>     That's an annoying test that can be done in the V4L2 core as well. Especially
>     since few drivers actually test for that.
>
>     Should such checks be added to the V4L2 core? And if so, should we add some
>     additional VFL types? Currently we have GRABBER (video nodes), VBI, RADIO
>     and SUBDEV. But if we want to do proper core checks, then we would also need
>     OUTPUT, VBI_OUT and M2M.

I'm in favor of adding checks to the core.

>
> 8) Remove the experimental tag from the following old drivers:
>
> 	VIDEO_TLV320AIC23B
> 	USB_STKWEBCAM
> 	VIDEO_CX18
> 	VIDEO_CX18_ALSA
> 	VIDEO_ZORAN_AVS6EYES
> 	DVB_USB_AF9005
> 	MEDIA_TUNER_TEA5761

ACK.

>
>     Removing this tag from these drivers might be too soon, though:
>
> 	VIDEO_NOON010PC30
> 	VIDEO_OMAP3
>

I've no opinion on these.

> 9) What should VIDIOC_G_STD/DV_PRESET/DV_TIMINGS return if the current input
>     or output does not support that particular timing approach? EINVAL? ENODATA?
>     This is relevant for the case where a driver has multiple inputs/outputs
>     where some are SDTV (and support the STD API) and others are HDTV (and
>     support the DV_TIMINGS API).
>
>     I propose ENODATA.

+1 for ENODATA, EINVAL makes no sense if all the input parameters are correct.

>
> 10) Proposal: add these defines:
>
> #define V4L2_IN_CAP_TIMINGS V4L2_IN_CAP_CUSTOM_TIMINGS
> #define V4L2_OUT_CAP_TIMINGS V4L2_OUT_CAP_CUSTOM_TIMINGS
>
> Since DV_TIMINGS is now used for any HDTV timings and no longer just for
> custom, non-standard timings, the word "CUSTOM" is no longer appropriate.
>

No opinion.

> 11) What should video output drivers do with the sequence and timestamp
>      fields when they return a v4l2_buffer from VIDIOC_DQBUF?
>
>      I think the spec is clear with respect to the timestamp:
>
>      "The driver stores the time at which the first data byte was actually
>       sent out in the timestamp field."
>
>      For sequence the spec just says:
>
>      "Set by the driver, counting the frames in the sequence."
>
>      So I think that output drivers should indeed set both sequence and
>      timestemp.
>

Ack.

> 12) Make the argument of write-only ioctls const in v4l2-ioctls.h. This makes
>      it obvious to drivers that they shouldn't change the contents of the input
>      struct since it won't make it back to userspace. It also simplifies
>      v4l2-ioctl.c since it can rely on the fact that after the ioctl call the
>      contents of the struct hasn't changed. Right now the struct contents is
>      logged (if debugging is on) before the ioctl call for write-only ioctls.
>

Ack (although this will break compilation of some drivers, but that can be fixed).

> Hard(er):
>
> 1) What is the right/best way to set the timestamp? The spec says gettimeofday,
>     but is it my understanding that ktime_get_ts is much more efficient.
>
>     Some drivers are already using ktime_get_ts.
>
>     Options:
>
>     a) all drivers must comply to the spec and use gettimeofday
>     b) we change the spec and all drivers must use the more efficient ktime_get_ts
>     c) we add a buffer flag V4L2_BUF_FLAG_MONOTONIC to tell userspace that a
>        monotonic clock like ktime_get_ts is used and all drivers that use
>        ktime_get_ts should set that flag.
>
>     If we go for c, then we should add a recommendation to use one or the other
>     as the preferred timestamp for new drivers.

Wouldn't b/c break the API?

> 2) If a driver supports only formats with more than one plane, should
>     V4L2_CAP_VIDEO_CAPTURE still be defined?

No

>     And if a driver also supports
>     single-plane formats in addition to >1 plane formats, should
>     V4L2_CAP_VIDEO_CAPTURE be compulsary?

Yes, so that non multi-plane aware apps keep working.

> 3) VIDIOC_CROPCAP: the spec says that CROPCAP must be implemented by all
>     capture and output devices (Section "Image Cropping, Inserting and Scaling").
>     In reality only a subset of the drivers support cropcap.
>
>     Should cropcap really be compulsory? Or only for drivers that can scale? And
>     in that case, should we make a default implementation for those drivers that
>     do not support it? (E.g.: call g_fmt and use the width/height as the
>     default and bounds rectangles, and set the pixel aspect to 1/1)
>

I vote for making it non compulsory, and simply returning -ENOTTY for drivers which
don't support it.

> 4) Pixel aspect: currently this is only available through VIDIOC_CROPCAP. It
>     never really belonged to VIDIOC_CROPCAP IMHO. It's just not a property of
>     cropping/composing. It really belongs to the input/output timings (STD or
>     DV_TIMINGS). That's where the pixel aspect ratio is determined.
>
>     While it is possible to add it to the dv_timings struct, I see no way of
>     cleanly adding it to struct v4l2_standard (mostly because VIDIOC_ENUMSTD
>     is now handled inside the V4L2 core and doesn't call the drivers anymore).
>
>     An alternative is to add it to struct v4l2_input/output, but I don't know
>     if it is possible to defined a pixelaspect for inputs that are not the
>     current input.
>
>     What I am thinking of is just to add a new ioctl for this VIDIOC_G_PIXELASPECT.
>     The argument is then:
>
> 	struct v4l2_pixelaspect {
> 		__u32 type;
> 		struct v4l2_fract pixelaspect;
> 		__u32 reserved[5];
> 	};
>
>     This combines well with the selection API.

We will want to be able to enumerate this too, so I vote for extending v4l2_frmsize_discrete
with a struct v4l2_fract pixelaspect, and likewise for v4l2_frmsize_stepwise.

Likewise we also want to get the pixelaspect on a TRY_FMT, which is a bit tricky, since
we cannot extend  v4l2_pix_format (*) instead we could add a v4l2_pix_format_w_aspect, and add
that to the v4l2_format union. Then apps who want to pixelratio can look inside
v4l2_pix_format_w_aspect instead, with the note that the aspect may be reported as 0/0 by
drivers which don't support reporting it. This avoids adding a new ioctl, and gives us
a way to get the pixelratio without actually having to set the fmt.

(*) no reserved space inside it, and if we would allow it to grow, we still would have an
issue because that would also grow v4l2_framebuffer, which we certainly cannot do.

> 5) How to handle tuner ownership if both a video and radio node share the same
>     tuner?
>
>     Obvious rules:
>
>     - Calling S_FREQ, S_TUNER, S_MODULATOR or S_HW_FREQ_SEEK will change owner
>       or return EBUSY if streaming is in progress.

That won't work, as there is no such thing as streaming from a radio node, I
suggest we go with the simple approach we discussed at our last meeting in
your Dutch House: Calling S_FREQ, S_TUNER, S_MODULATOR or S_HW_FREQ_SEEK will
make an app the tuner-owner, and *closing* the device handle makes an app
release its tuner ownership. If an other app already is the tuner owner
-EBUSY is returned.

>     - Ditto for STREAMON, read/write and polling for read/write.

No, streaming and tuning are 2 different things, if an app does both, it
will likely tune before streaming, but in some cases a user may use a streaming
only app together with say v4l2-ctl to do the actual tuning. I think keeping
things simple here is key. Lets just treat the "tuner" and "stream" as 2 separate
entities with a separate ownership.

>     - Ditto for ioctls that expect a valid tuner configuration like QUERYSTD.

QUERY is a read only ioctl, so it should not be influenced by any ownership, nor
imply ownership.

>     - Just opening a device node should *not* switch ownership.
Ack!

>     But it is not clear what to do when any of these ioctls are called:
>
>     - G_FREQUENCY: could just return the last set frequency for radio or TV:
>       requires that that is remembered when switching ownership. This is what
>       happens today, so G_FREQUENCY does not have to switch ownership.

Ack.

>     - G_TUNER: the rxsubchans, signal and afc fields all require ownership of
>       the tuner. So in principle you would want to switch ownership when
>       G_TUNER is called. On the other hand, that would mean that calling
>       v4l2-ctl --all -d /dev/radio0 would change tuner ownership to radio for
>       /dev/video0. That's rather unexpected.
>
>       It is possible to just set rxsubchans, signal and afc to 0 if the device
>       node doesn't own the tuner. I'm inclined to do that.

Right, G_TUNER should not change ownership, if the tuner is currently in radio
mode and a G_TUNER is done on the video node just 0 out the fields which we cannot
fill with useful info.

>     - Should closing a device node switch ownership? E.g. if nobody has a radio
>       device open, should the tuner switch back to TV mode automatically? I don't
>       think it should.

+1 on delaying the mode switch until it is actually necessary to switch mode.

>     - How about hybrid tuners?

No opinion.

Regards,

Hans