GNOME Bugzilla – Bug 719587
rtpgstpay: delay events until first buffer so they can be timestamped
Last modified: 2018-11-03 14:50:31 UTC
While developing a patch (concerning RTP-Info header) for gst-rtsp-server I stumbled upon an rtspserver test using rtpgstpay where my newly introduced code expected the first payloaded buffer in each media stream to have a timestamp. Unfortunately rtpgstpay sent an non-timestamped event as its first RTP packet so my code failed. Now, there is no reason to _not_ timestamp the events packaged into RTP packets so therefore I opted to delay the events until the first buffer arrives and give both events and buffer the same timestamp. Finally there was no unit test for rtpgstpay so I created a silly one that at least exercise the rtpgstpay a little bit.
Created attachment 263188 [details] [review] Proposed patch adding new unit test
Created attachment 263189 [details] [review] Proposed patch adding new unit test
Created attachment 263190 [details] [review] Proposed patch delaying events until buffer arrives
In set_timestamp() you do the same check twice.
Created attachment 263200 [details] [review] Proposed patch delaying events until buffer arrives Fixed repeated code.
After a brief a discussion with Wim Taymans on #gstreamer I believe that I should update this patch. I will hopefully get to this soon.
Ping.
Ping again
Don't know if Sebras is currently doing GStreamer work. Would've been good if he had mentioned what needs fixing :)
I'd like to be, but my spare time is unexpectedly short at the moment. Yes, I agree. Sadly I never pasted the discussion with wtay. :-/ I have a vague memory that we talked about GAP events though. Could it be that payloader ought to send a GAP event before sending any of the other events since these will have a delayed timestamp?
Here is what I have: Dec 04 01:46:54 <wtay> 719587 (delay events until buffer) should maybe let some events through (like GAP) Dec 04 01:47:33 <wtay> because you want some events to go through quickly Dec 04 01:47:36 <sebras> wtay: why? as I see it this would create havoc with the timestamping..? Dec 04 01:48:10 <wtay> the GAP is explicitly to fill a void (no buffers) Dec 04 01:48:30 <wtay> to keep the other end busy and processing, delaying it would not help Dec 04 01:48:51 <wtay> but I think you want to bundle the segment and caps with the first buffer Dec 04 01:49:31 <sebras> wtay: wait... does the original rtpgstpay even handle GAP..? Dec 04 01:49:52 <wtay> I don't know.. Dec 04 01:50:14 <sebras> wtay: if I were do to what you propose, then what rtptime would I use for the very first event which appears without any timestamp before the first buffer..? Dec 04 01:51:35 <sebras> wtay: this is the problem I'm seeing. the only events that I can se that rtpgstpay handles are FLUSH_STOP, TAG, CUSTOM_DOWNSTREAM, CUSTOM_BOTH and STREAM_START, any other event is not propagated through to the src pad of the payloader. Dec 04 01:51:42 <wtay> sebras, it would be the same as the rtptime of the next buffer Dec 04 01:52:09 <sebras> wtay: so basically ts_base then. Dec 04 01:52:16 <wtay> ok, so only the custom events are problematic Dec 04 01:52:32 <wtay> and flush I guess too Dec 04 01:52:37 <sebras> wtay: well, STREAM_START was the one that threw me actually. Dec 04 01:52:57 <wtay> you can easily bundle stream-start with the next buffer Dec 04 01:53:31 <sebras> wtay: because I ended up with an RTP packet in a gstbuffer in the payloader and then I queried the payloader for the last processed buffers running time (which was the RTP packet buffer) and got GST_CLOCK_TIME_NONE back. Dec 04 01:53:47 <wtay> and even if you get a GAP event after the segment-start, you could bundle the GAP and SEGMENT_START and give it the rtptime of the start position in the segment event Dec 04 01:55:39 <sebras> wtay: ok, so then between SEGMENT_START and the first buffer the running time shoudl be the start position of the segment event..? Dec 04 01:56:00 <sebras> wtay: and before SEGMENT_START it would be GST_CLOCK_TIME_NONE. Dec 04 01:56:58 <wtay> sebras, segment-start is delayed until you get the first buffer or GAP event, if you have a buffer you place the RTPtime of the buffer, if you have a GAP you place the RTPtime of the segment start Dec 04 01:58:34 <wtay> you always need stream-start, segment, (caps) (,buffer | GAP) Dec 04 02:03:35 <sebras> wtay: but what about the running-time (introduced i 719415)? I have only two sources of info there, segment event start position and the timestamp of the buffer. if the basepay have neither the event nor the buffer, then I guess GST_CLOCK_TIME_NONE is ok, if it has the segment event, then I expect that the start position of that event is the current running time, and whenever basepay receives the first buffer of course the timestamp of this buffe Dec 04 02:05:08 <wtay> sebras, yes, so I think your patch does good, collect events until you either see a buffer or a GAP event, then timestamp and push them Dec 04 02:06:11 <wtay> sebras, while you see the segment you update your 'current-best-timestamp-guess', when you see the buffer you know the running-time else with the GAP you take the best guess and push Dec 04 02:06:14 <sebras> wtay: well, rtpgstpay doesn't handle the SEGMENT_START event and use the start position to set the running time. Dec 04 02:07:07 <sebras> wtay: I can add that of course, and I think you are asking me to..? :) Dec 04 02:07:11 <wtay> sebras, it does not look right Dec 04 02:07:54 <wtay> sebras, ah ok, yes it just passes the timestamp to the base class, which uses the segment to do the running-time and rtptime then Dec 04 02:09:52 <wtay> sebras, so you need to somehow update the timestamps on those queued packets Dec 04 02:10:36 <wtay> your patch does that too Dec 04 02:11:30 <wtay> so the trigger to release the packets should either be a valid buffer timestamps or a GAP event Dec 04 02:12:29 <wtay> or event the custom-downstream event Dec 04 02:18:29 * wtay has quit (Read error: Connection reset by peer) Dec 04 02:22:20 <sebras> wtay: actually it is currently only a valid buffer, no GAP no CUSTOM_DOWNSTREAM
Thank you! :) However, even after reading through that discussion I'm a bit confused (it seems I was really smart and knowledgeable in 2013 though!). I need to refresh my knowledge about the rtp payloading a bit more and look at the patch I proposed before fixing it.
-- 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/98.