Re: [PATCH v4 13/36] [media] v4l2: add a frame timeout event

[Sakari Ailus]

On Sat, Mar 04, 2017 at 04:37:43PM -0800, Steve Longerbeam wrote:
> 
> 
> On 03/04/2017 02:56 AM, Sakari Ailus wrote:
> >Hi Steve,
> >
> >On Fri, Mar 03, 2017 at 02:43:51PM -0800, Steve Longerbeam wrote:
> >>
> >>
> >>On 03/03/2017 03:45 AM, Sakari Ailus wrote:
> >>>On Thu, Mar 02, 2017 at 03:07:21PM -0800, Steve Longerbeam wrote:
> >>>>
> >>>>
> >>>>On 03/02/2017 07:53 AM, Sakari Ailus wrote:
> >>>>>Hi Steve,
> >>>>>
> >>>>>On Wed, Feb 15, 2017 at 06:19:15PM -0800, Steve Longerbeam wrote:
> >>>>>>Add a new FRAME_TIMEOUT event to signal that a video capture or
> >>>>>>output device has timed out waiting for reception or transmit
> >>>>>>completion of a video frame.
> >>>>>>
> >>>>>>Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>
> >>>>>>---
> >>>>>>Documentation/media/uapi/v4l/vidioc-dqevent.rst | 5 +++++
> >>>>>>Documentation/media/videodev2.h.rst.exceptions  | 1 +
> >>>>>>include/uapi/linux/videodev2.h                  | 1 +
> >>>>>>3 files changed, 7 insertions(+)
> >>>>>>
> >>>>>>diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> >>>>>>index 8d663a7..dd77d9b 100644
> >>>>>>--- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> >>>>>>+++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> >>>>>>@@ -197,6 +197,11 @@ call.
> >>>>>>	the regions changes. This event has a struct
> >>>>>>	:c:type:`v4l2_event_motion_det`
> >>>>>>	associated with it.
> >>>>>>+    * - ``V4L2_EVENT_FRAME_TIMEOUT``
> >>>>>>+      - 7
> >>>>>>+      - This event is triggered when the video capture or output device
> >>>>>>+	has timed out waiting for the reception or transmit completion of
> >>>>>>+	a frame of video.
> >>>>>
> >>>>>As you're adding a new interface, I suppose you have an implementation
> >>>>>around. How do you determine what that timeout should be?
> >>>>
> >>>>The imx-media driver sets the timeout to 1 second, or 30 frame
> >>>>periods at 30 fps.
> >>>
> >>>The frame rate is not necessarily constant during streaming. It may well
> >>>change as a result of lighting conditions.
> >>
> >>I think you mean that would be a _temporary_ change in frame rate, but
> >>yes I agree the data rate can temporarily fluctuate. Although I doubt
> >>lighting conditions would cause a sensor to pause data transmission
> >>for a full 1 second.
> >
> >Likely not, at least not in typical conditions. The exposure time is still
> >quite specific to applications: it could be minutes if you take photos e.g.
> >of the night sky.
> >
> >What I'm saying here is that any static value is likely not both reasonable
> >and workable in all potential situations all the time. Still there are cases
> >(as yours below) that may happen in relatively common cases on some hardware
> >(more common than taking long exposure photos of the night sky with the said
> >hardware :)).
> 
> I doubt night photography will ever be a use-case for i.MX. The most
> common use-case for this driver will be used in automotive applications
> such as rear-view or 360 degree view cameras.

Ack.

> 
> 
> >
> >>
> >>
> >>>I wouldn't add an event for this:
> >>>this is unreliable and 30 times the frame period is an arbitrary value
> >>>anyway. No other drivers do this either.
> >>
> >>If no other drivers do this I don't mind removing it. It is really meant
> >>to deal with the ADV718x CVBS decoder, which often simply stops sending
> >>data on the BT.656 bus if there is an interruption in the input analog
> >>signal. But I agree that user space could detect this timeout instead.
> >>Unless I hear from someone else that they would like to keep this
> >>feature I'll remove it in version 5.
> >
> >That's a bit of a special situation --- still there are alike conditions on
> >existing hardware. You should return the buffers to the user with the ERROR
> >flag set --- or return -EIO from VIDIOC_DQBUF, if the condition will
> >persist:
> 
> On i.MX an EOF timeout is not recoverable without a stream restart, so
> I decided to call vb2_queue_error() when the timeout occurs (instead
> of sending an event). The user will then get -EIO when it attempts to
> queue or dequeue further buffers.

It believe that's the correct thing to do.

> 
> 
> >
> ><URL:https://www.linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-qbuf.html>
> >
> >Do you already obtain the frame rate from the image source (e.g. tuner,
> >sensor, decoder) and multiply the frame time by some number in the current
> >implementation?
> 
> No the timeout is a constant value, regardless of the source frame
> rate. Should the timeout be based on a constant time, or based on a
> constant # of frames? I really don't think it matters much, what matters
> is that it be long enough to be reasonably sure no data is forthcoming,
> for most use-cases.

That should be fine. If there is a use case that requires something else,
then the implementation can be changed: it's not visible to the user space
anyway.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx