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 629410 - GstBaseTransform: position query refers to sink pad, not source pad
GstBaseTransform: position query refers to sink pad, not source pad
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
0.10.30
Other All
: Normal blocker
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-09-12 10:42 UTC by Leo Singer
Modified: 2010-10-29 13:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (1.45 KB, patch)
2010-09-12 10:42 UTC, Leo Singer
needs-work Details | Review
use input position instead of reporting GST_CLOCK_TIME_NONE (1.58 KB, patch)
2010-10-28 13:31 UTC, Jonathan Matthew
committed Details | Review

Description Leo Singer 2010-09-12 10:42:16 UTC
Created attachment 170072 [details] [review]
Proposed patch

In order to answer position queries, GstBaseTransform stores the (timestamp + duration) of the last received buffer on the sink pad.  For elements that have a fixed delay or history -- for instance, an FIR or IIR filter -- this is not necessarily the same as the (timestamp + duration) of the buffers that get pushed on the source pad.

One consequence of this is confusing output from the progressreport element.

The attached patch takes the position from the buffers that are pushed on the source pad, instead of the buffers that are received on the sink pad.
Comment 1 Sebastian Dröge (slomo) 2010-09-12 11:30:35 UTC
This is partly correct. For the sinkpad you should still return this value as position but for the srcpad your new value is more correct
Comment 2 Sebastian Dröge (slomo) 2010-09-21 17:25:30 UTC
Instead of tracking timestamps on both sides it would also be possible to always take the input timestamps and return this as the position of the sinkpad and return this minus the latency as the position of the srcpad. Do you want to provide a patch for that?
Comment 3 Leo Singer 2010-09-22 10:12:13 UTC
(In reply to comment #2)
> Instead of tracking timestamps on both sides it would also be possible to
> always take the input timestamps and return this as the position of the sinkpad
> and return this minus the latency as the position of the srcpad. Do you want to
> provide a patch for that?

That sounds sensible to me.  I'm a little bit bogged down right now; it might be up to a week until I can put forth a new patch.  I don't mind if someone else does it before then.
Comment 4 Tim-Philipp Müller 2010-10-08 16:18:15 UTC
Setting NEEDINFO, awaiting updated patch.
Comment 5 Sebastian Dröge (slomo) 2010-10-10 16:19:22 UTC
Too bad that we can't really get the latency from inside basetransform and instead should do the doubled timestamp tracking


commit 14023fff89d073b5758ce8c46abf15951f6607f7
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Sun Oct 10 18:14:40 2010 +0200

    basetransform: Report the output position on POSITION queries on the srcpad
    
    There can be a difference between input and output last_stop.
    
    Fixes bug #629410.
Comment 6 Jonathan Matthew 2010-10-28 13:10:49 UTC
This breaks rhythmbox a tiny bit.  When playback is paused, we get the playback position by doing a position query on a volume element which is part of a bin that (when paused) is not linked to the audio sink.

Immediately after pausing, we do a flushing seek back about half a second to compensate for the fade out duration, which now results in last_stop_out being set to GST_CLOCK_TIME_NONE, so the position queries return that rather than the actual position.

Using segment.last_stop when last_stop_out is GST_CLOCK_TIME_NONE makes things work again for me.
Comment 7 Jonathan Matthew 2010-10-28 13:31:35 UTC
Created attachment 173397 [details] [review]
use input position instead of reporting GST_CLOCK_TIME_NONE

I think I misread the previous condition in three separate ways while trying to fix this, so it's entirely possible this isn't right.
Comment 8 Sebastian Dröge (slomo) 2010-10-28 16:53:51 UTC
Comment on attachment 173397 [details] [review]
use input position instead of reporting GST_CLOCK_TIME_NONE

Makes sense... let's get this into the next pre-release.
Comment 9 Wim Taymans 2010-10-29 13:33:56 UTC
sounds like a good idea.
Comment 10 Tim-Philipp Müller 2010-10-29 13:37:27 UTC
Comment on attachment 173397 [details] [review]
use input position instead of reporting GST_CLOCK_TIME_NONE

Ok, let's get this in then.

It's not ideal though, basetransform should always have 1:1 input-output timestamp mapping, there shuouldn't really be any lantency (says wim)..
Comment 11 Sebastian Dröge (slomo) 2010-10-29 13:45:50 UTC
Well, even for something simple like audioresample you have latency... or any other FIR filter.


commit 754c3038be87e182b7c71b1b402ed430a877ad58
Author: Jonathan Matthew <jonathan@d14n.org>
Date:   Thu Oct 28 23:28:15 2010 +1000

    basetransform: use input position for queries if we have no output position