rtpbasepayload: Update seqnum property correctly

Submitted by Linus Svensson

Link to original bug (#797303)

Description

Created attachment 373961
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.

Patch 373961, "Update seqnum for buffer lists":
0001-gstrtpbasepayload-Update-current-seqnum-for-buffer-l.patch

🔍 Sebastian Dröge @slomo reviewed patch 373961:

::: 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?

💬 Linus Svensson said:

(In reply to Sebastian Dröge (slomo) from comment 1)

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?

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().

💬 Linus Svensson said:

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.

💬 Linus Svensson said:

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?

💬 Sebastian Dröge @slomo said:

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.

mentioned in merge request !17 (merged)

mentioned in merge request gst-rtsp-server!3 (merged)

closed via merge request !17 (merged)