GNOME Bugzilla – Bug 597407
GstPipeline calculates base_time incorrectly when a new clock appears during PAUSED state
Last modified: 2009-10-13 16:20:09 UTC
The situation: pipeline has been running (by using GstSystemClock) and is put to PAUSED. Then a new clock appears. When pipeline is put back to PLAYING, following things happen: 1. GstPipeline starts selecting a clock and base_time in PAUSED_TO_PLAYING 2. it takes start time: start_time = GST_ELEMENT_START_TIME (pipeline); 3. update_clock flag is set, so the new clock gets selected as a master clock 4. current time is asked using the new clock: now = gst_clock_get_time (clock); 5. new_base_time = now - start_time + delay; Now this new_base_time is calculated by using timestamps from both old clock (start_time) and new clock (now), resulting an invalid base time. When pipeline is PLAYING, this causes the new clock to give only zero timestamps out because of the incorrect base_time: gstpipeline.c:415:gst_pipeline_change_state:<camerabin0> start_time=0:00:04.520843507, now=0:00:00.000000000, base_time 5124095:34:29.188708109 ... gstbaseaudiosrc.c:996:gst_base_audio_src_create:<pulsesrc0> buffer timestamp 0, ts 0:00:00.240000000 <= base_time 5124095:34:29.188708109
hmm, after further investigation it looks like the problem is not about timestamps from two different clocks, since that GST_ELEMENT_START_TIME (pipeline) is not an absolute timestamp after all. Instead, the problem is that if the new clock starts from zero, base time calculation gives a negative value, which is then casted into very big number when storing in unsigned variable.
The running_time of the pipeline (time spent in PLAYING) is taken when going to PAUSED. Then a new base_time is selected such that current clock time - base_time gives the original running_time. If the current clock time is indeed 0, this will result in a big base_time. I don't quite know how this would fail, the base time of an element is supposed to be a signed int64, so the calculation should work. Maybe there is some element that does some weird thing with the signed/unsigned value somewhere. Can you give more info on the elements involved? or how to reproduce this maybe.
(In reply to comment #2) > I don't quite know how this would fail, the base time of an element is supposed > to be a signed int64, so the calculation should work. Maybe there is some > element that does some weird thing with the signed/unsigned value somewhere. in GstBaseAudioSrc the base_time is taken to GstClockTime (which is guint64) variable and the timestamp is then compared to it: (gstbaseaudiosrc.c:965...): } else { GstClockTime base_time; /* to get the timestamp against the clock we also need to add our offset */ timestamp = gst_audio_clock_adjust (clock, timestamp); /* we are not slaved, subtract base_time */ base_time = GST_ELEMENT_CAST (src)->base_time; if (timestamp > base_time) { timestamp -= base_time; GST_LOG_OBJECT (src, "buffer timestamp %" GST_TIME_FORMAT " (base_time %" GST_TIME_FORMAT ")", GST_TIME_ARGS (timestamp), GST_TIME_ARGS (base_time)); } else { GST_LOG_OBJECT (src, "buffer timestamp 0, ts %" GST_TIME_FORMAT " <= base_time %" GST_TIME_FORMAT, GST_TIME_ARGS (timestamp), GST_TIME_ARGS (base_time)); timestamp = 0; } } It also seems that gst_element_(set/get)_base_time function take and return GstClockTime, even the value is stored in GstClockTimeDiff format. Is this an accident or done in purpose?
It sounds like an accident.
Created attachment 144965 [details] [review] ensure basetime won't be negative Wim, does that make sense?
I don't think that is going to work so well. base_time can be negative and elements should be able to deal with that (doing modulo 64bit arithmetic, I guess)
So it's impossible to fix this without breaking API/ABI? That won't be too bad I guess because it would only introduce a compiler warning in old code (if -Wsigned-compare or how it's called is enabled) and old code would break anyway in the negative cases... and would continue to work as before for the positive cases.
Would it be possible to change GstClockTime to be gint64 as well? Or does it need to be explicitly unsigned?
Created attachment 145122 [details] [review] Fix timestamp comparison code in GstBaseAudioSrc After all it looks like the problem culminated into GstBaseAudioSrc, where timestamp and basetime comparison was done incorrectly. Instead of directly comparing them we need to use GST_CLOCK_DIFF() macro and check the result. This is because basetime may be negative, so the macro handles the situation correctly by casting the result to GstClockTimeDiff. Attached is a patch which fixes the problem in GstBaseAudioSrc.
Created attachment 145124 [details] [review] Fix timestamp comparison code in GstNetClientClock There was a similar incorrect comparison in GstNetClientClock code. This patch fixes it.
Created attachment 145342 [details] [review] Test case for baseaudiosrc timestamp handling This test case reveals the problem in gstbaseaudiosrc timestamp <-> base_time comparison. Using the previously attached patch fixes the problem and test case succeeds.