GNOME Bugzilla – Bug 781776
decklink: Detect gaps on incoming stream times, issue warnings
Last modified: 2018-01-04 13:55:51 UTC
See commit message. Untested so far - I just wanted a remote backup.
Created attachment 350492 [details] [review] decklink: Detect gaps on incoming stream times, issue warnings When we receive a video or audio buffer, we calculate the next stream time based on the current stream time + buffer duration. If the next buffer's stream time is after that, we issue a warning. This happens because the stream time incoming from Decklink should be really constant and without gaps. If there is a gap, it means that something went wrong, e.g. the internal buffer pool is empty (too many buffers queued up downstream).
Attachment tested and found to work. Please review and feedback.
Created attachment 350631 [details] [review] decklink: Detect gaps on incoming stream times, issue warnings When we receive a video or audio buffer, we calculate the next stream time based on the current stream time + buffer duration. If the next buffer's stream time is after that, we issue a warning. This happens because the stream time incoming from Decklink should be really constant and without gaps. If there is a gap, it means that something went wrong, e.g. the internal buffer pool is empty (too many buffers queued up downstream).
Small modification - Allow up to 2 sample sizes of gap between audio buffers
Comment on attachment 350631 [details] [review] decklink: Detect gaps on incoming stream times, issue warnings There's a common submodule change in the patch (hence the needs-work). I wonder if posting QoS messages would be appropriate here. Provides more information and provides information in a way apps can process programmatically.
Created attachment 351428 [details] [review] decklink: Detect gaps on incoming stream times, issue warnings When we receive a video or audio buffer, we calculate the next stream time based on the current stream time + buffer duration. If the next buffer's stream time is after that, we issue a warning. This happens because the stream time incoming from Decklink should be really constant and without gaps. If there is a gap, it means that something went wrong, e.g. the internal buffer pool is empty (too many buffers queued up downstream).
(In reply to Tim-Philipp Müller from comment #5) > Comment on attachment 350631 [details] [review] [review] > decklink: Detect gaps on incoming stream times, issue warnings > > There's a common submodule change in the patch (hence the needs-work). Thanks, fixed :)
(In reply to Tim-Philipp Müller from comment #5) > I wonder if posting QoS messages would be appropriate here. Provides more > information and provides information in a way apps can process > programmatically. That's actually a good idea but I'm not 100% sure how to fill the QoS message. The main details are clear, set_qos_values() probably can just be omitted, set_qos_stats() could get GST_FORMAT_TIME and processed/dropped could be calculated all the time. GST_FORMAT_BUFFERS (aka frames) might also work, but I'm not sure if the dropped amount of data is always full (audio) frames.
Review of attachment 351428 [details] [review]: ::: sys/decklink/gstdecklinkaudiosrc.cpp @@ +36,3 @@ #define DEFAULT_DISCONT_WAIT (1 * GST_SECOND) #define DEFAULT_CHANNELS (GST_DECKLINK_AUDIO_CHANNELS_2) +#define DEFAULT_EXPECTED_STREAM_TIME (GST_CLOCK_TIME_NONE) This is not a property, so does not need a #define here @@ +38,3 @@ +#define DEFAULT_EXPECTED_STREAM_TIME (GST_CLOCK_TIME_NONE) + +#define ABSDIFF(x, y) ( (x) > (y) ? ((x) - (y)) : ((y) - (x)) ) #ifndef ABSDIFF #define ABSDIFF ... #endif @@ +203,3 @@ self->buffer_size = DEFAULT_BUFFER_SIZE; self->channels = DEFAULT_CHANNELS; + self->expected_stream_time = DEFAULT_EXPECTED_STREAM_TIME; Need to reset this whenever capturing is (re)started
(In reply to Sebastian Dröge (slomo) from comment #8) > but I'm not sure if the dropped amount of data is always full (audio) > frames. It's not, just confirmed in practice.
Review of attachment 351428 [details] [review]: ::: sys/decklink/gstdecklinkaudiosrc.cpp @@ +673,3 @@ + // Detect gaps in stream time + start_stream_time = p.stream_timestamp; This can be GST_CLOCK_TIME_NONE, if the audio packet came without a corresponding video packet. In which case you should assume that it has the expected time.
Created attachment 353815 [details] [review] decklink: Detect gaps on incoming stream times, issue warnings When we receive a video or audio buffer, we calculate the next stream time based on the current stream time + buffer duration. If the next buffer's stream time is after that, we issue a warning. This happens because the stream time incoming from Decklink should be really constant and without gaps. If there is a gap, it means that something went wrong, e.g. the internal buffer pool is empty (too many buffers queued up downstream).
Thanks, fixed the review comments and also did it with QoS now. Please review again :)
Review of attachment 353815 [details] [review]: ::: sys/decklink/gstdecklinkaudiosrc.cpp @@ +690,3 @@ + if (self->expected_stream_time != GST_CLOCK_TIME_NONE && + ABSDIFF (self->expected_stream_time, p.stream_timestamp) > + gst_util_uint64_scale (2, GST_SECOND, self->info.rate)) { 2 samples? Why not > 1? ::: sys/decklink/gstdecklinkvideosrc.cpp @@ +803,3 @@ } + self->processed += f.stream_duration; This and the other counter accumulate duration rounding errors @@ +805,3 @@ + self->processed += f.stream_duration; + if (self->expected_stream_time != GST_CLOCK_TIME_NONE && + ABSDIFF (self->expected_stream_time, f.stream_timestamp) > 1) { 1ns seems dangerous, are you sure? There might be rounding errors with the duration. Maybe > 1/fps?
Created attachment 362414 [details] [review] decklink: Detect gaps on incoming stream times, issue warnings When we receive a video or audio buffer, we calculate the next stream time based on the current stream time + buffer duration. If the next buffer's stream time is after that, we issue a warning. This happens because the stream time incoming from Decklink should be really constant and without gaps. If there is a gap, it means that something went wrong, e.g. the internal buffer pool is empty (too many buffers queued up downstream).
(In reply to Sebastian Dröge (slomo) from comment #14) > Review of attachment 353815 [details] [review] [review]: > > ::: sys/decklink/gstdecklinkaudiosrc.cpp > @@ +690,3 @@ > + if (self->expected_stream_time != GST_CLOCK_TIME_NONE && > + ABSDIFF (self->expected_stream_time, p.stream_timestamp) > > + gst_util_uint64_scale (2, GST_SECOND, self->info.rate)) { > > 2 samples? Why not > 1? Remember those jittery 29.97 audio timestamps? :) > > ::: sys/decklink/gstdecklinkvideosrc.cpp > @@ +803,3 @@ > } > > + self->processed += f.stream_duration; > > This and the other counter accumulate duration rounding errors Fixed, thanks! > @@ +805,3 @@ > + self->processed += f.stream_duration; > + if (self->expected_stream_time != GST_CLOCK_TIME_NONE && > + ABSDIFF (self->expected_stream_time, f.stream_timestamp) > 1) { > > 1ns seems dangerous, are you sure? There might be rounding errors with the > duration. > > Maybe > 1/fps? It's >1, so any rounding error would make it == 1, so the if wouldn't be activated. :) From what I've seen, this works well enough already.
(In reply to Vivia Nikolaidou from comment #16) > (In reply to Sebastian Dröge (slomo) from comment #14) > > Review of attachment 353815 [details] [review] [review] [review]: > > > > ::: sys/decklink/gstdecklinkaudiosrc.cpp > > @@ +690,3 @@ > > + if (self->expected_stream_time != GST_CLOCK_TIME_NONE && > > + ABSDIFF (self->expected_stream_time, p.stream_timestamp) > > > + gst_util_uint64_scale (2, GST_SECOND, self->info.rate)) { > > > > 2 samples? Why not > 1? > > Remember those jittery 29.97 audio timestamps? :) Makes sense, can you add a comment about that? With drop-frame we have gaps of 1 sample every now and then (rounding errors because of the samples-per-frame pattern which is not 100% accurate), and due to rounding errors in the calculations these can be 2>x>1. > > @@ +805,3 @@ > > + self->processed += f.stream_duration; > > + if (self->expected_stream_time != GST_CLOCK_TIME_NONE && > > + ABSDIFF (self->expected_stream_time, f.stream_timestamp) > 1) { > > > > 1ns seems dangerous, are you sure? There might be rounding errors with the > > duration. > > > > Maybe > 1/fps? > > It's >1, so any rounding error would make it == 1, so the if wouldn't be > activated. :) From what I've seen, this works well enough already. Ok, also add a comment about that please
Created attachment 366284 [details] [review] decklink: Detect gaps on incoming stream times, issue warnings When we receive a video or audio buffer, we calculate the next stream time based on the current stream time + buffer duration. If the next buffer's stream time is after that, we issue a warning. This happens because the stream time incoming from Decklink should be really constant and without gaps. If there is a gap, it means that something went wrong, e.g. the internal buffer pool is empty (too many buffers queued up downstream).