TWCCMan evaluates recovery pct for FEC #15
Conversation
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
2 times, most recently
from
0b04a8b to
b8bd262
Compare
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
from
b8bd262 to
373ba36
Compare
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
from
373ba36 to
bb19396
Compare
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
from
bb19396 to
c475446
Compare
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
from
6599840 to
c484c7e
Compare
|
We are also missing some tests here. I am thinking some rtx tests that checks for the presence of the new meta and confirms the right things are set given various conditions. And also some rtpsession/twcc tests (we really should split out the twcc tests from rtpsession at some point) that shows clearly some interactions between the new meta and how that will affects statistics. |
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
from
a2ebbe2 to
6b3aafa
Compare
| @@ -0,0 +1,159 @@ | |||
| /* GStreamer | |||
There was a problem hiding this comment.
this needs gst-indent run on it
| gboolean gst_buffer_get_repair_seqnums(GstBuffer *buffer, guint32 * ssrc, | ||
| GArray ** seqnums); | ||
|
|
||
| /* If this packet is a FEC/RTX packet, what is it sequential number a block */ |
There was a problem hiding this comment.
Also, GStreamer typically puts comments like this in the .c file, not the header.
|
|
||
| GST_RTP_API | ||
| GstRTPRepairMeta *gst_buffer_add_rtp_repair_meta(GstBuffer *buffer, | ||
| const guint16 idx_red_packets, const guint16 num_red_packets, |
There was a problem hiding this comment.
I am wondering about the "red" here, since "red" already has a very concrete meaning to rtp resilience. (https://getstream.io/resources/projects/webrtc/advanced/red/).
Also on the topic of naming, I think GStreamer often leans towards keeping the variable names in API fairly short, like here: https://github.com/pexip/gstreamer/blob/main/subprojects/gst-plugins-base/gst-libs/gst/rtp/gstrtpmeta.h
So could we think of some shorter names, maybe just "packet_idx" and "packet_count"?
| pinfo->rtx_ssrc = | ||
| GPOINTER_TO_UINT (g_hash_table_lookup (sess->rtx_ssrc_to_ssrc, | ||
| GUINT_TO_POINTER (pinfo->ssrc))); | ||
|
|
||
| return res; | ||
| } |
There was a problem hiding this comment.
The change here need to clean up a bit more, since rtx_ssrc_to_ssrc is no longer in use, so I believe that includes a property and some other stuff around this?
| if (GST_CLOCK_TIME_IS_VALID (pkt->local_ts)) { | ||
| GstClockTimeDiff offset = GST_CLOCK_DIFF (pkt->local_ts, start_time); | ||
| for (i = 0; i < array_length; i++) { | ||
| SentPacket *pkt = ((StatsPktPtr*)gst_queue_array_peek_nth_struct (array, i)) |
There was a problem hiding this comment.
Here it would be nice with a API for doing this, so that instead of having to cast and use lots of (), we could call _sent_packet_get () or something similar.
| gst_queue_array_push_tail_struct (twcc->stats_ctx->pt_packets, | ||
| (StatsPktPtr*)&pkt); | ||
| TWCCStatsCtx *ctx = _get_ctx_for_pt (twcc, pkt->pt); | ||
| gst_queue_array_push_tail_struct (ctx->pt_packets, (StatsPktPtr*)&pkt); |
There was a problem hiding this comment.
Similarly here. Hide away using the underlying implementation with some nice API that also makes it more clear what is happening. _stats_packet_add() or something.
| g_array_unref (twcc->parsed_packets); | ||
| g_queue_free_full (twcc->rtcp_buffers, (GDestroyNotify) gst_buffer_unref); | ||
| g_mutex_clear (&twcc->recv_lock); | ||
| g_mutex_clear (&twcc->send_lock); | ||
| g_mutex_clear (&twcc->sent_packets_feedback_lock); |
There was a problem hiding this comment.
seeing two new locks makes me a bit skeptical, since we don't currently have any concurrency issues with the old code, and I don't think any new threads have been created, so why would this be necessary?
| g_array_append_val (twcc->sent_packets, packet); | ||
| _prune_old_sent_packets (twcc); | ||
| rtp_twcc_manager_register_seqnum (twcc, pinfo->ssrc, pinfo->seqnum, seqnum); | ||
|
|
There was a problem hiding this comment.
The old code here was more in line with how I would structure this.
_prune_old_sent_packets()
rtp_twcc_manager_register_seqnum()
If you could split logical bits of code into smaller functions, then we avoid having big blocks like this one, and with the {} it almost begs to be its own function.
| GstClockTimeDiff rtt = GST_CLOCK_STIME_NONE; | ||
| SentPacket * found; |
There was a problem hiding this comment.
run gst-indent here as well
amberik1
force-pushed
the
main
branch
2 times, most recently
from
a910da4 to
d9a6a3e
Compare
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
from
1ab0597 to
49507e7
Compare
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
2 times, most recently
from
e0660e7 to
00b5bf0
Compare
Major rework of twcc manager internals caused by need to evaluate recovery percentage in case of FEC. Sent Packet structs are stored GstQueueArray to keep their pointers persistent. TWCC feedback parser creates a list of packets updated by this feedback, which then is passed into statistic gathering context. Then the remaining work is done in get_windowed_stats call.
MishaBaranov
force-pushed
the
misha/twcc_fec_stats
branch
from
b62f556 to
733856f
Compare
Major rework of twcc manager internals caused by need to evaluate recovery percentage in case of FEC.
RTX and FEC packets get RepairMeta metadata, containing an array of data packets seqnums and SSRC this particular packet covers.
Then TWCC Manager tracks Sent Packet structs by storing them in GstQueueArray to keep pointers persistent. TWCC feedback parser creates a list of packets updated by this feedback, which then is passed into statistic gathering context.
Then the remaining work is done in get_windowed_stats call:
original seqnum→twcc seqnum