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 754356 - event: Make sure that timestamp + diff in QoS events is never smaller than 0
event: Make sure that timestamp + diff in QoS events is never smaller than 0
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-31 13:13 UTC by Sebastian Dröge (slomo)
Modified: 2017-05-05 20:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
event: Make sure that timestamp + diff in QoS events is never smaller than 0 (1.53 KB, patch)
2015-08-31 13:13 UTC, Sebastian Dröge (slomo)
committed Details | Review
aggregator: Don't forward QOS events to sinkpads that had no buffer yet (4.12 KB, patch)
2015-08-31 13:14 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2015-08-31 13:13:40 UTC
See commit message
Comment 1 Sebastian Dröge (slomo) 2015-08-31 13:13:44 UTC
Created attachment 310353 [details] [review]
event: Make sure that timestamp + diff in QoS events is never smaller than 0

When a running-time-offset is stored in the event, it could become smaller
than 0 although the event is otherwise correct. This can happen when pad
offsets are used.

To prevent this, we set the timestamp to -diff, so that in the end the sum of
both is exactly 0.
Comment 2 Sebastian Dröge (slomo) 2015-08-31 13:14:16 UTC
Created attachment 310354 [details] [review]
aggregator: Don't forward QOS events to sinkpads that had no buffer yet

Otherwise they will receive a QOS event that has earliest_time=0 (because we
can't have negative timestamps), and consider their buffer as too late
Comment 3 Sebastian Dröge (slomo) 2015-09-01 07:09:06 UTC
This is both not ideal yet btw, but it works.

The first patch will put an arbitrary timestamp into the event, which together with the diff will lead to timestamp+diff=0. This causes less hard failures than before (integer overflow, so timestamp+diff was a huge number and everything was too late) but makes elements believe that now would be a good time to produce something for timestamp 0... which might just be what they currently produce.

That leads to the second patch, where aggregator told newly added sinkpads with pad offsets in use somewhere that timestamp 0 is the earliest time now. And the first buffer was dropped because it had timestamp 0 (checks for dropping are buffer_running_time <= timestamp + diff).
Comment 4 Sebastian Dröge (slomo) 2015-09-25 22:25:20 UTC
commit a3513d6e977853a6f4d8a6062152f2b664ab8f34
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Mon Aug 31 15:35:11 2015 +0300

    event: Make sure that timestamp + diff in QoS events is never smaller than 0
    
    When a running-time-offset is stored in the event, it could become smaller
    than 0 although the event is otherwise correct. This can happen when pad
    offsets are used.
    
    To prevent this, we set the timestamp to -diff, so that in the end the sum of
    both is exactly 0.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754356
Comment 5 Sebastian Dröge (slomo) 2015-09-25 22:26:04 UTC
commit fc76c936f45e5123978b719ddd9e345a40850862
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Mon Aug 31 16:12:40 2015 +0300

    aggregator: Don't forward QOS events to sinkpads that had no buffer yet
    
    Otherwise they will receive a QOS event that has earliest_time=0 (because we
    can't have negative timestamps), and consider their buffer as too late
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754356
Comment 6 Olivier Crête 2017-05-05 20:54:11 UTC
@slomo: You added a "first_buffer" element to the GstAggregatorPadPrivate without documenting the locking, which makes sense, as you didn't do any locking!