GNOME Bugzilla – Bug 627229
fpsdisplaysink should not measure fps relative to pipeline clock
Last modified: 2010-09-06 08:09:11 UTC
pipeline clock is running at same speed as playback, so this only shows the framerate of the clip being measured.
Created attachment 168169 [details] [review] patch to change fpsdisplaysink to measure raw fps
Review of attachment 168169 [details] [review]: ::: gst/debugutils/fpsdisplaysink.c @@ +225,3 @@ + self->last_ts = self->start_ts = gst_util_get_timestamp(); + self->timeout_id = + g_timeout_add (self->fps_update_interval, display_current_fps, Hmm, actually the g_timeout_add() is a problem. It adds a g_main_loop() dependency :/. But it was there before. @@ +331,3 @@ + + frames_rendered = g_atomic_int_get (&self->frames_rendered); + frames_dropped = g_atomic_int_get (&self->frames_dropped); If we want ultimate consistency we would need a lock instead of the atomics. Theoretically we can read the first var and then it changes before we read the 2nd. Would that be bad?
I tried the patch, will commit it after freeze.
Created attachment 168179 [details] [review] patch to change fpsdisplaysink to measure raw fps run gst-indent before making the patch
Created attachment 168180 [details] [review] don't use a timeout update the message in the probe handler I wonder if this way we actually don't even need the atomic ops.
(In reply to comment #2) > Review of attachment 168169 [details] [review]: > > ::: gst/debugutils/fpsdisplaysink.c > @@ +225,3 @@ > + self->last_ts = self->start_ts = gst_util_get_timestamp(); > + self->timeout_id = > + g_timeout_add (self->fps_update_interval, display_current_fps, > > Hmm, actually the g_timeout_add() is a problem. It adds a g_main_loop() > dependency :/. But it was there before. > > @@ +331,3 @@ > + > + frames_rendered = g_atomic_int_get (&self->frames_rendered); > + frames_dropped = g_atomic_int_get (&self->frames_dropped); > > If we want ultimate consistency we would need a lock instead of the atomics. > Theoretically we can read the first var and then it changes before we read the > 2nd. Would that be bad? fwiw, I don't think it would be bad if there was a context switch between reading those two values. I guess for either value, only one or the other would be incremented at each QOS event. So essentially it would be as if the timer ran either before or after the QOS event. It isn't even so much about race conditions.. the main thing is to assure that a sufficient memory barrier instruction happens so that if other thread is running on different processor, it has a chance to see the updated value at all.
(In reply to comment #5) > Created an attachment (id=168180) [details] [review] > don't use a timeout > > update the message in the probe handler > > I wonder if this way we actually don't even need the atomic ops. Yeah, with this approach we don't need atomic ops.. all is happening in same thread. :-)
If noone sees any downside with doing it in the probe-handler and it works for you (rob), coudl you change the first patch to kick out the atomic_ops once again (sorry about that).
Created attachment 168558 [details] [review] updated patch which preserves the non-atomic but 64bit counters updated patch which preserves the non-atomic but 64bit counters
Created attachment 168619 [details] [review] don't use a timeout
Rob, thanks for updated patch. I'll push them after freeze.