GNOME Bugzilla – Bug 797130
decklink: sometimes displays a flashed frame of a previously played sequence
Last modified: 2018-09-12 12:54:58 UTC
See commit
Created attachment 373616 [details] [review] decklink: start scheduled playback in paused
Review of attachment 373616 [details] [review]: Generally makes sense, just some comments. Also this code becomes constantly more complicated, that's annoying :) ::: sys/decklink/gstdecklinkvideosink.cpp @@ +537,2 @@ clock = gst_element_get_clock (GST_ELEMENT_CAST (self)); + if (!clock || clock != self->output->clock) { Are you sure this even works without a clock in PLAYING? @@ +618,3 @@ + + if (GST_CLOCK_TIME_IS_VALID (self->paused_start_time)) { + /* only valid whil we're in PAUSED and most likely without a set clock, missing 'e' in while :) @@ +928,3 @@ + /* cause sometimes decklink stops without notifying us... */ + guint64 wait_time = g_get_monotonic_time () + G_TIME_SPAN_SECOND; + g_cond_wait_until (&self->output->cond, &self->output->lock, wait_time); Looks like this should get a separate commit? @@ +956,3 @@ // Sample the clocks again to get the most accurate values // after we started scheduled playback + if (clock) { And otherwise? We would still do calculations without a clock or not? @@ +997,3 @@ + guint64 wait_time = g_get_monotonic_time () + G_TIME_SPAN_SECOND; + g_cond_wait_until (&self->output->cond, &self->output->lock, wait_time); + self->output->output->IsScheduledPlaybackRunning (&active); Separate commit, as above
(In reply to Sebastian Dröge (slomo) from comment #2) > Review of attachment 373616 [details] [review] [review]: > > Generally makes sense, just some comments. Also this code becomes constantly > more complicated, that's annoying :) > > ::: sys/decklink/gstdecklinkvideosink.cpp > @@ +537,2 @@ > clock = gst_element_get_clock (GST_ELEMENT_CAST (self)); > + if (!clock || clock != self->output->clock) { > > Are you sure this even works without a clock in PLAYING? Well, the element doesn't allow no clock in playing so I don't think that really applies. The code doesn't actually sample from the external clock, only from the internal clock which is not something I changed. > @@ +618,3 @@ > + > + if (GST_CLOCK_TIME_IS_VALID (self->paused_start_time)) { > + /* only valid whil we're in PAUSED and most likely without a set clock, > > missing 'e' in while :) > > @@ +928,3 @@ > + /* cause sometimes decklink stops without notifying us... */ > + guint64 wait_time = g_get_monotonic_time () + G_TIME_SPAN_SECOND; > + g_cond_wait_until (&self->output->cond, &self->output->lock, > wait_time); > > Looks like this should get a separate commit? Maybe, it mostly happens from a previous run that I interrupted (Ctrl-C) without a clean shutdown where we won't get a notification normally through the associated callback. > @@ +956,3 @@ > // Sample the clocks again to get the most accurate values > // after we started scheduled playback > + if (clock) { > > And otherwise? We would still do calculations without a clock or not? Overall calculations without a clock are also fine. It will simply take the internal clock values and not offset from the timestamps provided.
Created attachment 373617 [details] [review] decklink: start scheduled playback in paused
Created attachment 373618 [details] [review] decklink: wait for stop with timeout
Review of attachment 373618 [details] [review]: ::: sys/decklink/gstdecklinkvideosink.cpp @@ +928,3 @@ + /* cause sometimes decklink stops without notifying us... */ + guint64 wait_time = g_get_monotonic_time () + G_TIME_SPAN_SECOND; + g_cond_wait_until (&self->output->cond, &self->output->lock, wait_time); I was wondering if a timeout should maybe be an error message here, but at least should be logged as a GST_WARNING
commit 946cbbccc15f707857a550065fa7ba0028e4d627 Author: Matthew Waters <matthew@centricular.com> Date: Wed Sep 12 05:29:09 2018 -0500 decklink: wait for stop with a timeout Decklink sometimes does not notify us through the callback that it has stopped scheduled playback either because it was uncleanly shutdown without an explicit stop or for unknown other reasons. Wait on the cond for a short amount of time before checking if scheduled playback has stopped without notification. https://bugzilla.gnome.org/show_bug.cgi?id=797130 commit bf849e9a69442f7a6f9d4f0a1ef30d5a8009f689 Author: Matthew Waters <matthew@centricular.com> Date: Sun Jul 8 09:54:04 2018 -0500 decklink: start scheduled playback in paused This is part of a much larger goal to always keep the frames we schedule to decklink be always increasing. This also allows us to avoid using both the sync and async frame display functions which aren't recomended to be used together. If the output timestatmsp is not always increasing decklink seems to hold onto the latest frame and may cause a flash in the output if the played sequence has a framerate less than the video output. Scenario is play for N seconds, pause, flushing seek to some other position, play again. Each of the play sequences would normally start at 0 with the decklink time. As a result, the latest frame from the previous sequence is kept alive waiting for it's timestamp to pass before either dropping (if a subsequent frame in the new sequence overrides it) or displayed causing the out of place frame to be displayed. This is also supported by the debug logs from the decklink video sink element where a ScheduledFrameCompleted() callback would not occur for the frame until the above had happened. It was timing related as to whether the frame was displayed based on the decklink refresh cycle (which seems to be 16ms here), when the frame was scheduled by the sink and the difference between the 'time since vblank' of the two play requests (and thus start times of scheduled playback). https://bugzilla.gnome.org/show_bug.cgi?id=797130
Comment on attachment 373618 [details] [review] decklink: wait for stop with timeout Committed with a warning.
Review of attachment 373616 [details] [review]: > This also allows us to avoid using both the > sync and async frame display functions which aren't recomended to be used > together. This is not generally correct, sync and async can be used together, and can be useful. Maybe you meant that this is not supported for this element ?
(In reply to Nicolas Dufresne (ndufresne) from comment #9) > Review of attachment 373616 [details] [review] [review]: > > > This also allows us to avoid using both the > > sync and async frame display functions which aren't recomended to be used > > together. > > This is not generally correct, sync and async can be used together, and can > be useful. Maybe you meant that this is not supported for this element ? He's talking about the Decklink API