After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 597407 - GstPipeline calculates base_time incorrectly when a new clock appears during PAUSED state
GstPipeline calculates base_time incorrectly when a new clock appears during ...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
0.10.23
Other Linux
: Normal normal
: 0.10.26
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-10-05 11:30 UTC by Tommi Myöhänen
Modified: 2009-10-13 16:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ensure basetime won't be negative (741 bytes, patch)
2009-10-07 15:11 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
rejected Details | Review
Fix timestamp comparison code in GstBaseAudioSrc (1001 bytes, patch)
2009-10-09 11:55 UTC, Tommi Myöhänen
accepted-commit_now Details | Review
Fix timestamp comparison code in GstNetClientClock (660 bytes, patch)
2009-10-09 11:59 UTC, Tommi Myöhänen
accepted-commit_now Details | Review
Test case for baseaudiosrc timestamp handling (4.96 KB, patch)
2009-10-13 11:19 UTC, Tommi Myöhänen
accepted-commit_now Details | Review

Description Tommi Myöhänen 2009-10-05 11:30:58 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
Comment 1 Tommi Myöhänen 2009-10-05 12:06:45 UTC
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.
Comment 2 Wim Taymans 2009-10-06 17:08:04 UTC
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.
Comment 3 Tommi Myöhänen 2009-10-07 13:15:51 UTC
(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?
Comment 4 Wim Taymans 2009-10-07 13:20:16 UTC
It sounds like an accident.
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2009-10-07 15:11:07 UTC
Created attachment 144965 [details] [review]
ensure basetime won't be negative

Wim, does that make sense?
Comment 6 Wim Taymans 2009-10-07 15:19:17 UTC
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)
Comment 7 Sebastian Dröge (slomo) 2009-10-08 11:06:45 UTC
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.
Comment 8 Tommi Myöhänen 2009-10-09 08:53:42 UTC
Would it be possible to change GstClockTime to be gint64 as well? Or does it need to be explicitly unsigned?
Comment 9 Tommi Myöhänen 2009-10-09 11:55:25 UTC
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.
Comment 10 Tommi Myöhänen 2009-10-09 11:59:01 UTC
Created attachment 145124 [details] [review]
Fix timestamp comparison code in GstNetClientClock

There was a similar incorrect comparison in GstNetClientClock code. This patch fixes it.
Comment 11 Tommi Myöhänen 2009-10-13 11:19:13 UTC
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.