Hardly creative, but for the moment I do not seem to arrive at anything better than coming up with some sort of (start/stop) event sent downstream to convince (base)sink to:
* note start event, and perform modified position reporting in paused based on that start (to avoid advancing time)
* note stop event and the amount of time that it reports, and subtract that amount of that time to determine subsequent running_time (used for subsequent sync handling and position reporting)

The latter mechanism would then replace the offset adding (to buffer timestamps) currently done by jitterbuffer.  Disadvantage obviously is involving basesink, but the advantage is that ts retain original meaning (e.g. without twisting position reporting all along the pipeline).

Created attachment 190514 [details] [review]
jitterbuffer: send custom event to inform downstream of live buffering

Patch introduces a custom downstream GstLiveBuffering event that informs downstream of the offset that is being applied (or of current ongoing buffering).

Created attachment 190515 [details] [review]
basesink: account for live buffering in stream position reporting

Patch makes basesink aware of possible upstream live buffering, and makes it compensate for the applied offsets in position reporting (or falling back to last reported position if buffering is ongoing).

The foregoing patches implement sort-of the first option given in comment #1.

However, with some further reflection, the current jitterbuffer approach actually has more problems than wrong position reporting; since it directly manipulates (increases) buffer timestamps, it is possible for those timestamps to fall outside the current segment (roughly determined by npt-start and npt-stop).  So not only position reporting will be off, but material may also be clipped.

As such, the second option in comment #2 may be in order.  Specifically, rather than having jitterbuffer apply an offset to buffer timestamps, use the event in these patches to have downstream apply an offset when needed, which is basically in basesink's sync handling (where it can treat the received offset as additional latency).  As mentioned, disadvantage is involving basesink in some additional event stuff (though not unlike -base depayloader using -good jitterbuffer lost packet event), but the advantage is cleaner timestamps with less exception handling along the way (including in basesink).

(In reply to comment #4)
> 
> As such, the second option in comment #2 may be in order.  Specifically, rather
> than having jitterbuffer apply an offset to buffer timestamps, use the event in
> these patches to have downstream apply an offset when needed, which is
> basically in basesink's sync handling (where it can treat the received offset
> as additional latency).

Yes, I've been considering that too, all this buffer timestamp changing is not such a good idea in the end. It would be nicer if the jitterbuffer could notify it's offset with a nice event somehow.

Created attachment 190735 [details] [review]
jitterbuffer: send custom event to inform downstream of live buffering

Created attachment 190736 [details] [review]
basesink: account for live buffering and reported buffering offset

Created attachment 190737 [details] [review]
baseaudiosink: also consider buffering offset in determining sync time

So, in summary, above patches implement the alternative approach where jitterbuffer does not modify the buffer timestamp, but the (custom) event it sends is used by downstream sink (basesink and baseaudiosink) to determine sync time (in addition to latency and ts_offset).  As such, stream time can be determined normally and when buffering is ongoing last reported stream position can be used instead.  It should also prevent undue segment clipping.

Disadvantage is that it's "spread out" a bit, and don't know if the event is "nice enough" and/or needs a bit more standardization.

Unfortunately segment info (even in 0.11) might not suffice for all this, since OTOH depayloader tends to intercept these for its own purposes, and even some segment event could not prevent the sink from reporting increasing position while buffering (since based on increasing running time).

Created attachment 190930 [details] [review]
basesink: account for live buffering and reported buffering offset

Updated patch should apply cleaner than previous one.

Review of attachment 190930 [details] [review]:

In gst_base_sink_get_position, you are not initializing the new boolean variable "buffering". I have no idea why the compiler isn't warning about this. On -O2 I could understand (since it gets optimized away), but in -O0 I can clearly step through with gdb, which shows in my case buffering is 42 initially.

I guess you want to set this to FALSE initially, just as "in_paused".

Created attachment 191271 [details] [review]
basesink: account for live buffering and reported buffering offset

Oops, newer version with minor fix as mentioned.

I think you attached the old patch again :)

Created attachment 191284 [details] [review]
basesink: account for live buffering and reported buffering offset

Next (attaching) attempt ...

Created attachment 203058 [details] [review]
rtpmanager: modify live buffering

Still with custom event(s) as before, though slightly different:
* there is (also) an OOB event which allows the sink to halt/resume rendering at once when buffering starts/stops (rather than having it go on for some queued amount which is rather confusing)
* the offset that is conveyed now simply represents the (running) time spent buffering, and is appropriately accumulated by the sink (and used as a sort of variable latency to determine subsequent data's proper sync time)

Created attachment 203059 [details] [review]
basesink: account for live buffering and reported buffering offset

Sort of as before, i.e. with custom events, though with modified semantics as mentioned above.

So finally got around to this latest version, which seems to make it all Just Work without messing with buffer timestamps (and corresponding disadvantages) and has mostly advantages (e.g. rendering starts/stops along with buffering, and position reporting is kept properly).

As before, disadvantage is some event'ing, custom for now but could be made more standard of course.  Only real disadvantage appears to be that "live aware" elements can no longer assume that a passing buffer will be rendered at its running_time, but that assumption was already broken when latency was introduced (and this adds a variable latency to the setting, which buffering and/or underlying network delays basically are).

As such, if needed, or for completeness, the "variable latency" could also be reported upstream (as is latency currently) with some event (such as extending the current latency event with some field or whatever) for the benefit of whatever interested upstream element.

All in all, while maybe not the very cleanest, it is still pretty clean and likely simplier than e.g. trying to squeeze in some BUFFERING state (between PAUSED and PLAYING) and playing around with (re)distributing updated base_time in between transitions.

0.11 might be a good time to standardize some of this, though maybe already possible in 0.10 ...

Wim, anybody? What's the status here?

AFAICS situation is still as before.

Could have a look at bringing up-to-date if there is consensus on how to proceed.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/40.