Review of attachment 366674 [details] [review]:

This actually looks correct to me and corresponds to what has been defined in the design doc[0] now I wonder why and how it has not been detected before!

I would rather have someone else review it to confirm.

[0] https://gstreamer.freedesktop.org/documentation/design/synchronisation.html#stream-time

It should basically be like gst_segment_to_stream_time(), which also does not seem to use the offset. Which IIRC also makes sense because offset is related to running time, not stream time.

Why do you think this is correct? Note that the calculations in the design docs seem to be all about running time, and how to get from there to the stream time.

As the doc mention, offset is in the same scale as the buffer timestamp. But it still need to be scaled according tot he applied_rate.

"The amount (in buffer timestamps) that has already been elapsed in the segment"

(In reply to Sebastian Dröge (slomo) from comment #2)
> It should basically be like gst_segment_to_stream_time(), which also does
> not seem to use the offset. Which IIRC also makes sense because offset is
> related to running time, not stream time.

> Why do you think this is correct? Note that the calculations in the design docs seem to be all about running time, and how to get from there to the stream time.

My first understanding was also that `segment.offset` only applies to the running_time but then I read the design doc again and the final formula with the following statement: "This last formula is typically used in sinks to report the current position in an accurate and efficient way.". The formula:

    stream_time = (S.offset + (absolute_time - base_time - S.base) * ABS (S.rate)) * ABS (S.applied_rate) + S.time

From my reading and understanding of the code in `gst_basesink_get_position` we are using this exact formula modulo the addition of `S.offset`, thus my statement that this patch looks correct to me. Now afaiu, in `gst_segment_to_stream_time()` we use:

    stream_time = (position - S.start) * ABS (S.applied_rate) + S.time

which indeed does not take `S.offset` into account.

Afaiu from the design doc those formulas are equivalent.

Now I am confused and might be mistaking or have misunderstood the code :-)

Yes the offset seems to have to be taken into account when calculating the stream time from the running time like basesink does. I'm not sure why it does not cause problems nowadays when using a segment with offset!=0 (e.g. from pad offsets).

Can someone create a unit test for this that fails with the current behaviour and succeeds with the new patch?

(In reply to Sebastian Dröge (slomo) from comment #5)
> I'm not sure why it does not cause problems nowadays when using a segment with offset!=0 (e.g. from pad offsets).

Maybe it was too small to be noticed.

Created attachment 368325 [details] [review]
gstbasesink: Include segment.offset in the computation of position

Position queries with GST_FORMAT_TIME are supposed to return stream
time.

gst_base_sink_get_position() estimates the current stream time on its
own instead of using gst_segment_to_stream_time(), but the algorithm
used was not taking segment.offset into account, resulting in invalid
values when this field was set to a non-zero value.

(In reply to Sebastian Dröge (slomo) from comment #5)
>
> Can someone create a unit test for this that fails with the current
> behaviour and succeeds with the new patch?

You can find a unit test in the patch I just uploaded.

Review of attachment 368325 [details] [review]:

lgtm

Attachment 368325 [details] pushed as 65dcb2a - gstbasesink: Include segment.offset in the computation of position