GNOME Bugzilla – Bug 751026
basesink: Properly handle buffer lists for the last-sample property
Last modified: 2015-06-22 11:35:05 UTC
In case that rtsp server sends range info in play response msg as below (start not 0), rtpjitterbuffer is mis-calculating elapsed time. RTSP response message 0xb63f8030 status line: code: '200' reason: 'OK' version: '1.0' headers: key: 'CSeq', value: '5' key: 'RTP-Info', value: 'url=rtsp://127.0.0.1:8554/test/stream=0;seq=29781;rtptime=2982392158, url=rtsp://127.0.0.1:8554/test/stream=1;seq=2244;rtptime=3012487835' key: 'Range', value: 'npt=29.375027216-62.95' key: 'Server', value: 'GStreamer RTSP server' key: 'Session', value: 'eyblXfy01$zjO-+Z' body: length 0 This causes EOS event finally, and pipeline is shutdown very soon. Because mis-calculated elapsed time leads to adding EOS timer in update_estimated_eos and do_eos_timeout creates EOS event. IMHO, elapsed time needs to be calculated based on clock base time and npt start time.
Created attachment 305357 [details] [review] rtpjitterbuffer: calculate elapsed time based on clock base time and npt-start time In case that npt start time is non-0, elapsed time needs to be calculated based on clock base and npt start time.
it does not seem right.. elapsed time is calculated as time since start (clock_base). total time is calculated as npt_stop - npt_start. As long as elapsed < total, we are still good. Your patch tries to add npt_start to the elapsed time, which does not make sense to me.
Related, see http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=62a7bcb3959613bcf52a8f6f92c3ed090b1c57a3
(In reply to Wim Taymans from comment #2) > As long as elapsed < total, we are still good. Your patch tries to add > npt_start to the elapsed time, which does not make sense to me. But in the case I reported, elapsed is always over total. IMHO, if my approach doesn't make sense, it means that clock_base is invalid in this case. Could you give guide to fix this issue?
(In reply to Sebastian Dröge (slomo) from comment #3) > Related, see > http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/ > ?id=62a7bcb3959613bcf52a8f6f92c3ed090b1c57a3 I think this patch doesn't fix this issue. ext_time = gst_rtp_buffer_ext_timestamp (&priv->ext_timestamp, rtp_time); if (ext_time < priv->ext_timestamp) { ext_time = priv->ext_timestamp; } In gst_rtp_buffer_ext_timestamp, return value is assigned to the first param. which means ext_time and priv->ext_timestamp is always same.
Indeed, thanks: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=e9902430daaf283a28789e33c6e00f070a09594d I don't think this solves your problem here, it's just something else wrong in that code that I noticed while looking at your patch :) I don't know what the correct fix for your problem would be.
(In reply to Sebastian Dröge (slomo) from comment #6) > I don't think this solves your problem here, it's just something else wrong > in that code that I noticed while looking at your patch :) I don't know what > the correct fix for your problem would be. I'm digging around somewhere else more. Probably, this would be gst-rtsp-server's issue. Thanks for response! :-)
For the completion of this bug. Is it reproducible using specific steps ? Are you testing on current git master as the bug says ?
I'm testing on git master. Reproduce step. 1. run test-mp4 in gst-rtsp-server with shared 'TRUE' 2. connect to this server with gst-launch playbin 3. After passing through half of streaming, another client connect to this server in the same way. 4. it quits by EOS event as soon as it starts. I'm investigating into rtsp server. I think RTP-Info is wrong in the second PLAY response msg from the server.
In gst-rtsp-server, during making play response, sometimes it gets invalid seqnum from last-sample in multiudpsink.(in gst_rtsp_stream_get_rtpinfo) Case : 1. Server's streaming shared media to a client. 2. Another client requests same media. 3. Server and the client start to negotiate. 4. Finally, server sends PLAY response to client. seqnum of RTP-Info in this play response is exactly same as previous PLAY response's seqnum. This is an example, which you can reproduce very easily. ---------- PLAY response to the first client ------------ RTSP response message 0xb705e030 status line: code: '200' reason: 'OK' version: '1.0' headers: key: 'CSeq', value: '7' key: 'RTP-Info', value: 'url=rtsp://127.0.0.1:8554/test/stream=0;seq=22808;rtptime=1168841023, url=rtsp://127.0.0.1:8554/test/stream=1;seq=15530;rtptime=1679833381' key: 'Range', value: 'npt=0-62.95' key: 'Server', value: 'GStreamer RTSP server' key: 'Session', value: 'zZPFVRWjP85yuYzh' body: length 0 ---------- PLAY response to the second client ------------ RTSP response message 0xb705e030 status line: code: '200' reason: 'OK' version: '1.0' headers: key: 'CSeq', value: '5' key: 'RTP-Info', value: 'url=rtsp://127.0.0.1:8554/test/stream=0;seq=22808;rtptime=1168841023, url=rtsp://127.0.0.1:8554/test/stream=1;seq=16338;rtptime=1679833381' key: 'Range', value: 'npt=28.747845721-62.95' key: 'Server', value: 'GStreamer RTSP server' key: 'Session', value: 'pd9ib.qQhHopBJLl' body: length 0 You can see same seqnum(22808) of the first stream, while another stream's seqnum(15530, 16338) is different. It's because basesink doesn't call gst_base_sink_set_last_buffer every render in case of BUFFER_LIST. Actually, in this example, rtph264pay sends BUFFER_LIST. To be confirmed, if I set large mtu size to rtph264pay to force to send a BUFFER in test-mp4, it's working fine. I'm wondering if this issue should be fixed or not and what part should I fix it? Instead of getting last-sample from sink, get information from payloader? (As description says, it's more accurate to get information from sink) Or somewhere else could be fixed?
Very interesting finding. Would a probe on the sink sinkpad be accurate enough ?
Hah. I think GstBaseSink should probably call set_last_buffer() also in case of a buffer list, with the last buffer in the list. You'd have to check if it has timestamps set, and if not set the timestamps from the first buffer/sync_buf.
Created attachment 305516 [details] [review] basesink: in case of buffer-list rendering, set the last buffer in the list as last buffer (In reply to Tim-Philipp Müller from comment #12) > Hah. I think GstBaseSink should probably call set_last_buffer() also in case > of a buffer list, with the last buffer in the list. You'd have to check if > it has timestamps set, and if not set the timestamps from the first > buffer/sync_buf. Thanks for advice. Following tim's advice. I'm not sure that I understood exactly what you mean about timestamp thing.
(In reply to Nicolas Dufresne (stormer) from comment #11) > Very interesting finding. Would a probe on the sink sinkpad be accurate > enough ? Thanks for advice. Using a probe on the sinkpad is better option than payloader, I think. Probably, this is needed unless basesink is changed.
basesink should be changed like you did, but I think in this specific case gst-rtsp-server would prefer to have the *first* buffer of the bufferlist. Not the last one. And I think it would make generally sense to provide the first buffer of a buffer list here, as very often all buffers but the first contain no useful metadata. Alternatively, what about adding a gst_sample_set/get_buffer_list()?
(In reply to Sebastian Dröge (slomo) from comment #15) > Alternatively, what about adding a gst_sample_set/get_buffer_list()? I think it's good idea. You mean once a user get last-sample from sink, the user can choose a buffer from the buffer_list in GstSample, don't you?
Yes, and maybe the buffer in the sample should still be set to the *first* buffer of the buffer list. So that by default you have the *first* buffer in the sample, but if you need more you can still look at the buffer list.
Created attachment 305548 [details] [review] sample: add gst_sample_set/get_buffer_list apis Allowed to set/get buffer list to sample if needed
Created attachment 305549 [details] [review] basesink: enable to get last sample including buffer list if needed In case of a buffer list rendering, last-sample is not updated. It needs to be updated and enable to get buffer list from last-sample.
Created attachment 305817 [details] [review] sample: add gst_sample_set/get_buffer_list apis Add Since 1.6 to description.
Created attachment 305818 [details] [review] asesink: enable to get last sample including buffer list Add gst_base_sink_set_last_buffer also in each render.
commit 7c34b4ed0f66bc764fcbc10d0e3612cffdbd85a5 Author: Hyunjun <zzoon.ko@samsung.com> Date: Mon Jun 22 20:02:55 2015 +0900 basesink: enable to get last sample including buffer list if needed In case of a buffer list rendering, last-sample is not updated. It needs to be updated and enable to get buffer list from last-sample. https://bugzilla.gnome.org/show_bug.cgi?id=751026 commit e8db96b033b3459691bb201711eb72a712ef6327 Author: Hyunjun <zzoon.ko@samsung.com> Date: Mon Jun 22 19:35:40 2015 +0900 sample: add gst_sample_set/get_buffer_list apis Allowed to set/get buffer list to sample if needed https://bugzilla.gnome.org/show_bug.cgi?id=751026