GNOME Bugzilla – Bug 797303
rtpbasepayload: Update seqnum property correctly
Last modified: 2018-11-03 12:11:08 UTC
Created attachment 373961 [details] [review] Update seqnum for buffer lists If a buffer list is pushed, the seqnum property will have the sequence number from the first buffer in the list, but it should be the last one.
Review of attachment 373961 [details] [review]: ::: gst-libs/gst/rtp/gstrtpbasepayload.c @@ +1308,3 @@ gst_buffer_list_foreach (GST_BUFFER_LIST_CAST (obj), filter_meta, NULL); + /* sequence number has increased more if this was a buffer list */ + payload->seqnum = data.seqnum - 1; Why -1?
(In reply to Sebastian Dröge (slomo) from comment #1) > Review of attachment 373961 [details] [review] [review]: > > ::: gst-libs/gst/rtp/gstrtpbasepayload.c > @@ +1308,3 @@ > gst_buffer_list_foreach (GST_BUFFER_LIST_CAST (obj), filter_meta, NULL); > + /* sequence number has increased more if this was a buffer list */ > + payload->seqnum = data.seqnum - 1; > > Why -1? data.seqnum is currently used to keep track of the next sequence number to use. It's set to priv->next_seqnum in the beginning, and every call to set_headers (in the loop) will end with an increment. priv->next_seqnum is set to data.seqnum at the end of _prepare_push().
This might cause a problem when the RTP info is generated in gst-rtsp-server. I have to investigate. There is code in gst-rtsp-server that reads the current sequence number from all payloaders and set a new offset when a media is paused. That will not work if the last push was a buffer list. (do_set_seqnum() in rtsp-media.c). Anyway, don't merge until I have figured out if the RTP info was affected by this.
This does indeed cause a problem when creating the RTP info. gst_rtsp_stream_get_rtpinfo() will read the "stats" property of the payloader to get the sequence number. The payloader may have produced more then one RTP packet in that case. If the seqnum property has the value of the last produced RTP packet (which makes sense to me if it does), the current way of generating the RTP info will not work. There is still an issue with RTSP PAUSE when the server uses GST_RTSP_SUSPEND_MODE_RESET. More work has to be done to solve this, then. This patch does not work (at least without other changes), sorry for the upload. Can I mark the attachment as obsolete?
The patch is actually fine, the property should give the seqnum of the last processed packet. It's a bug elsewhere (RTSP server, probably others) that they assume that they can get the very first seqnum from there on preroll. The real solution would be to change RTSP server (and whatever else breaks) to use the seqnum-offset from the RTP stats instead. That's always going to be the very first seqnum.
-- 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-base/issues/495.