Created attachment 298267 [details] [review]
v4l2src: add frame loss detection

Review of attachment 298267 [details] [review]:

Do you plan on a patchset to actually do something with that ? Lost frame should be signalled using GAP event. Note about patch submission that we don't use Signed-off-by in this project. We do enforce bugzilla cross-reference, so you last line should be this bug URI.

::: sys/v4l2/gstv4l2src.c
@@ -753,3 @@
-    GST_BUFFER_OFFSET (*buf) = v4l2src->offset++;
-    GST_BUFFER_OFFSET_END (*buf) = v4l2src->offset;
-  }

Is there a reason to move code around here ?

*** Bug 668018 has been marked as a duplicate of this bug. ***

(In reply to Nicolas Dufresne (stormer) from comment #2)
> Review of attachment 298267 [details] [review] [review]:
> 
> Do you plan on a patchset to actually do something with that ? Lost frame
> should be signalled using GAP event. Note about patch submission that we
> don't use Signed-off-by in this project. We do enforce bugzilla
> cross-reference, so you last line should be this bug URI.
> 

The primary goal of this patchset was to show a usage of the sequence counter
as offset (first patch). Secondary usage was performance checking/optimisation of
gst-launch pipelines and a custom application.

I can try to implement GAP events instead...

Thanks for the hint for Signed-off/Bug-Refrence, will try to get it
right on next submission...

> ::: sys/v4l2/gstv4l2src.c
> @@ -753,3 @@
> -    GST_BUFFER_OFFSET (*buf) = v4l2src->offset++;
> -    GST_BUFFER_OFFSET_END (*buf) = v4l2src->offset;
> -  }
> 
> Is there a reason to move code around here ?

Yes, the reason was to get the timestamps in the 'lost frames' warning
message right (see the timestamp adjusting code between the original
location and the new location), means the timestamp is the
same as you see pipeline-downstream...

A trace is good btw, it's just that tracing it and not reporting it is not ideal. Reason for moving the code make sense. I'll test and merge that one later.

I don't think we should send GAP events here, since we're about to push a buffer anyway, so doesn't really make sense IMHO.

Please also don't use non-portable printf format modifiers like %llu but "%" G_GUINT64_FORMAT instead.

(In reply to Tim-Philipp Müller from comment #6)
> I don't think we should send GAP events here, since we're about to push a
> buffer anyway, so doesn't really make sense IMHO.

Though some people have sent request to be able to track this pro-grammatically. What would you recomand instead ?

The source should post a QoS message (not event) on the bus IMHO.

That would allow the application to track frames not captured, but not downstream elements. Downstream elements will have to deduce that from the offsets then.

Ok, I'll reduce the scope of this bug then. The offsets need to be reviewed (specially encoded vs raw, which may or may not require the same behaviour).

Created attachment 298375 [details] [review]
[PATCH v2 1/2] v4l2: use v4l2 capture device sequence counter

Changes v1 -> v2:
  - removed Signed-off-by
  - add bugzilla cross-reference URI

Created attachment 298376 [details] [review]
[PATCH v2 2/2] v4l2src: add frame loss detection

Changes v1 -> v2:
  - use G_GUINT64_FORMAT instead of %llu
  - reword commit message to better explain reason for code move
  - removed Signed-off-by
  - add bugzilla cross-reference URI

Created attachment 298437 [details] [review]
[PATCH v3 1/2] v4l2: use v4l2 capture device sequence counter

Chnages v2 -> v3:
  - rebased against changed #745438

Created attachment 298438 [details] [review]
[PATCH v3 2/2] v4l2src: add frame loss detection

Chnages v2 -> v3:
  - rebased against changed #745438

Created attachment 300278 [details] [review]
[PATCH v4 1/2] v4l2: use v4l2 capture device sequence counter

Changes v3 -> v4:
  - rebase against mainline (including commit
    eeb4d2e 'v4l2bufferpool: Don't update buffer for OUTPUT'
    for bug #745438)

Created attachment 300279 [details] [review]
[PATCH v4 2/2] v4l2src: add frame loss detection

Changes v3 -> v4:
  - rebase against mainline
  - add QoS message (suggested by Tim-Philipp Müller)

Review of attachment 300278 [details] [review]:

Looks good.

Review of attachment 300279 [details] [review]:

::: sys/v4l2/gstv4l2src.c
@@ +815,3 @@
+  } else {
+    /* check for frame loss with given (from v4l2 device) buffer offset */
+    if ((GST_BUFFER_OFFSET (*buf) != (v4l2src->offset + 1)) && (v4l2src->offset != 0)) {

Could you use OFFSET_END to avoid the +1 ? And maybe check offset != 0 before ?

@@ +827,3 @@
+
+    }
+    v4l2src->offset = GST_BUFFER_OFFSET (*buf);

Cool, but we need to reset the offset everything we reset the HW. Otherwise we'll send a false positive on every renegotiation.

Hello Nicolas,

thanks for reveiew...

(In reply to Nicolas Dufresne (stormer) from comment #17)
> Review of attachment 300279 [details] [review] [review]:
> 
> ::: sys/v4l2/gstv4l2src.c
> @@ +815,3 @@
> +  } else {
> +    /* check for frame loss with given (from v4l2 device) buffer offset */
> +    if ((GST_BUFFER_OFFSET (*buf) != (v4l2src->offset + 1)) &&
> (v4l2src->offset != 0)) {
> 
> Could you use OFFSET_END to avoid the +1 ? And maybe check offset != 0
> before ?

The diff (inclusive OFFSET_END usage) would look like the following:

   } else {
     /* check for frame loss with given (from v4l2 device) buffer offset */
-    if ((GST_BUFFER_OFFSET (*buf) != (v4l2src->offset + 1)) && (v4l2src->offset != 0)) {
-      guint64 lost_frame_count = GST_BUFFER_OFFSET (*buf) - v4l2src->offset - 1;
+    if ((v4l2src->offset != 0) && (GST_BUFFER_OFFSET (*buf) != v4l2src->offset)) {
+      guint64 lost_frame_count = GST_BUFFER_OFFSET (*buf) - v4l2src->offset;
       GST_WARNING_OBJECT (v4l2src,
           "lost frames detected: count = %" G_GUINT64_FORMAT " - ts: %" GST_TIME_FORMAT,
               lost_frame_count, GST_TIME_ARGS (timestamp));
@@ -826,7 +826,7 @@ retry:
       gst_element_post_message (GST_ELEMENT_CAST (v4l2src), qos_msg);
 
     }
-    v4l2src->offset = GST_BUFFER_OFFSET (*buf);
+    v4l2src->offset = GST_BUFFER_OFFSET_END (*buf);
   }


Not sure if the (micro) optimization to get rid of the '+1' code
is justifying the minor code obfuscation (v4l2src->offset meaning changed
from actual offset to expected offset)?

> 
> @@ +827,3 @@
> +
> +    }
> +    v4l2src->offset = GST_BUFFER_OFFSET (*buf);
> 
> Cool, but we need to reset the offset everything we reset the HW. Otherwise
> we'll send a false positive on every renegotiation.

I have no example for renogotiation at the moment, does the  v4l2 driver
reset the  sequence counter in case of renegotition?

If no: no need to reset v4l2src->offset, lost frame indication will stay...

If yes: I can take a look where to reset v4l2src->offset...

Regards,
Peter

(In reply to Peter Seiderer from comment #18)
> Not sure if the (micro) optimization to get rid of the '+1' code
> is justifying the minor code obfuscation (v4l2src->offset meaning changed
> from actual offset to expected offset)?

Ok I don't care much.

> 
> > 
> > @@ +827,3 @@
> > +
> > +    }
> > +    v4l2src->offset = GST_BUFFER_OFFSET (*buf);
> > 
> > Cool, but we need to reset the offset everything we reset the HW. Otherwise
> > we'll send a false positive on every renegotiation.
> 
> I have no example for renogotiation at the moment, does the  v4l2 driver
> reset the  sequence counter in case of renegotition?
> 
> If no: no need to reset v4l2src->offset, lost frame indication will stay...
> 
> If yes: I can take a look where to reset v4l2src->offset...


So ... I've asked Laurent P. He said that driver are supposed to restart the sequence if you streamoff/streamon. While in GST you are not supposed to for just a renegotiation. The sequence is only started if you actually flush the pipeline.

So yes, this patch need a little more work to respect the GST contract when their is a renegotiation (this is supported in v4l2src atm).

Created attachment 300488 [details] [review]
[PATCH v5 2/3] v4l2src: add frame loss detection

Changes v4 -> v5:
  - change frame loss check compare order (suggested by Nicolas Dufresne)

Created attachment 300490 [details] [review]
[PATCH v5 3/3] v4l2src: device sequence/offset correction in case of renegotiation

The v4l2 device restarts the sequence counter in case of streamoff/streamon,
the GST offset values are supposed to increment strictly monotonic, so
adjust the sequence counter/offset values in case of caps
renegotiation.

Tested with usb webcam and v4l2src-renegotiate test program [1] from [2].

[1] https://bugzilla.gnome.org/attachment.cgi?id=298324
[2] https://bugzilla.gnome.org/show_bug.cgi?id=682770

Review of attachment 300488 [details] [review]:

Good.

Review of attachment 300490 [details] [review]:

Good.

Thanks.