OCP-6005 rtph266pay: Add config-interval and AP #42

carlosfg-flu · 2024-11-19T07:24:30Z

continuation of #37

carlosfg-flu · 2024-11-25T14:01:27Z

When using jitterbuffer, the playback does not work well. I think it might be because the encoder is not setting the timestamps properly 😩

cfoch

I will review tomorrow AP implementation.

cfoch · 2024-11-27T12:56:45Z

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

@@ -205,12 +266,36 @@ static void
 _set_property (GObject * object, guint prop_id,
    const GValue * value, GParamSpec * pspec)
 {
+  GstRtpH266Pay *rtph266pay = GST_RTP_H266_PAY (object);
+
+  GST_OBJECT_LOCK (rtph266pay);


I wouldn't protect with GST_OBJECT_LOCK/UNLOCK the default case: G_OBJECT_WARN_INVALID_PROPERTY_ID.

Do you think it could lead to some kind of error or wrong behavior? I agree it isn't necessary, but if it's just because of that, I'd rather keep it as is. The code is simpler this way.

No, just no need to warn inside a protected section.

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

cfoch · 2024-11-27T14:39:30Z

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

+  for (GList * l = head; l != NULL; l = l->next) {
+    Nalu *current_nalu = l->data;
+
+    if (!_can_insert_xps (rtph266pay, current_nalu))


What about renaming to something like _nalus_insert_xps_nalu?

🤔 I'd like to keep the can verb, as this method is not inserting, but checking if the insertion is possible. I don't mind adding the nalu particle. What do you think about _can_insert_xps_nalu()?

I had a typo: _nalus_can_insert_ps. Just a preference. Nitpick.

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

cfoch · 2024-11-28T09:37:57Z

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

@@ -323,6 +413,9 @@ _sink_event (GstRTPBasePayload * rtpbasepay, GstEvent * event)
      GST_DEBUG_OBJECT (rtph266pay, "EOS: Draining");
      ret = _push_pending_data (rtph266pay, TRUE);
      break;
+    case GST_EVENT_STREAM_START:
+      _clear_xps_cache (rtph266pay);


Why do you decide to clera the xps cache here and not in GST_EVENT_FLUSH_STOP?

I'm not sure 😕 I was imitating what rtph265pay does:

case GST_EVENT_STREAM_START: GST_DEBUG_OBJECT (rtph265pay, "New stream detected => Clear VPS, SPS and PPS"); gst_rtp_h265_pay_clear_vps_sps_pps (rtph265pay); break;

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

cfoch · 2024-11-28T09:58:37Z

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

+  const AggregatedNalus *aggregated_nalus = &rtph266pay->aggregated_nalus;
+  guint mtu = GST_RTP_BASE_PAYLOAD_MTU (rtph266pay);
+  guint num_nalus = g_queue_get_length ((GQueue *) & aggregated_nalus->nalus);
+  gsize nalu_size_sum = aggregated_nalus->nalu_size_sum;


Remove nalu_size_sum definition. We can directly use aggregated_nalus->nalu_size_sum.

I like the way it is. It makes this statement fit in ~~one~~two line and makes it easier to read:

guint payload_size = NALU_HDR_LEN + nalu_size_field_sum + nalu_size_sum + nalu->size;

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

cfoch · 2024-11-28T11:03:17Z

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

+    }
+  }
+
+  if (_aggregated_nalus_have_au (aggregated_nalus))


Is this part even reachable? There was a condition above if (!_aggregated_nalus_is_empty (aggregated_nalus)) {.

Yes, it is. Let's say a complete AU comes in and fits into an AP. What will happen is:

The for-each-nalu loop will aggregate all nalus into the AP: _aggregated_nalus_add(); -> continue;. The Can't aggregate anymore part of the loop won't be executed.

Finally, if we have aggregated a complete AU, we push it.

It seems this loop is not easy to understand 😞 Do you think it'd be good to add a commentary summarizing what it does?

Yes, add a comment. I don't understand yet, though.

So, if an AU is ensured to be completed after this while loop terminates, what is pushed here?

// vvv Can't aggregate anymore vvv if (!_aggregated_nalus_is_empty (aggregated_nalus)) { g_queue_push_head (nalus, nalu); // Keep this one for later ret = _push_aggregated (rtph266pay); // Push what we have right now if (ret != GST_FLOW_OK) return ret;

An AP containing an incomplete AU? Can you explain in what case this piece of code called?

I've added even more commentaries and reworded some of them. Let me know what you think, please.

So, if an AU is ensured to be completed after this while loop terminates, what is pushed here?

No, an AU is not ensured to be complete at this point. At this point, we just know we've tried to aggregate this NALU, be we couldn't. In this situation we might have to do one of these two things:

If we already have something aggregated, send it. We'll deal later with the current NALU that couldn't fit inside this AP.

If we have nothing aggregated, then it means the current NALU can't be aggregated and it has be processed by other means (ie: FU or UP)

cfoch · 2024-11-28T11:06:57Z

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

+    // vvv Can't aggregate anymore vvv
+    if (!_aggregated_nalus_is_empty (aggregated_nalus)) {
+      g_queue_push_head (nalus, nalu);  // Keep this one for later
+      ret = _push_aggregated (rtph266pay);      // Push what we have right now


Why not check here with _aggregated_nalus_have_au?

It doesn't matter whether we have a complete AU or not; as we can no longer aggregate, we need to push the AP we have right now.

Let's say an AU has Nalu1, Nalu2... Nalu100. Can an AP have Nalu1..50 and the next AP have Nalu51... Nalu100?

Yes! Exactly! An AP can have less than one AU, but it can not have more than one AU.

An AP aggregates NAL units of one access unit, and it MUST NOT contain NAL units from more than one AU.

subprojects/gst-plugins-good/gst/rtp/gstrtph266pay.c

OCP-5966 rtph266pay

Keep a cache of all Parameter Sets received, then, send them when it's appropriated according with the `config-interval` parameter. Issue: OCP_6005

Add support for two types of aggregation: * zero-latency: Only aggregate Parameter Set NALUs * max: Try to aggregate entires NALUs Issue: OCP_6005

UNKOWN -> UNKNOWN Issue: OCP_6005

Remove X from XPS. Now it's only Package Set. Issue: OCP_6005

Explain the more complex part of the algorithm, as it was proveen to be hard to understand. Issue: OCP_6005

Add missing Nalu fields to these parameters. Issue: OCP_6005

carlosfg-flu changed the title ~~rtph266pay: WIP: Add XPS cache~~ OCP-6005 rtph266pay: WIP: Add XPS cache Nov 19, 2024

carlosfg-flu mentioned this pull request Nov 19, 2024

OCP-5966 rtph266pay #37

Merged

carlosfg-flu force-pushed the rtph266pay_xps branch from 4be0a88 to 27d4f51 Compare November 19, 2024 08:00

carlosfg-flu force-pushed the rtph266pay branch from 22320a2 to fc9cf86 Compare November 19, 2024 08:00

carlosfg-flu force-pushed the rtph266pay_xps branch from 27d4f51 to 3919204 Compare November 19, 2024 09:41

carlosfg-flu force-pushed the rtph266pay branch from fc9cf86 to 4e2fde9 Compare November 19, 2024 09:41

carlosfg-flu force-pushed the rtph266pay_xps branch 5 times, most recently from a5bb6e8 to 1bcc68c Compare November 22, 2024 17:14

carlosfg-flu changed the title ~~OCP-6005 rtph266pay: WIP: Add XPS cache~~ OCP-6005 rtph266pay: Add config-interval and AP Nov 25, 2024

carlosfg-flu marked this pull request as ready for review November 25, 2024 13:52

Base automatically changed from rtph266pay to main November 25, 2024 13:54

carlosfg-flu requested a review from cfoch November 26, 2024 06:36

cfoch requested changes Nov 27, 2024

View reviewed changes

cfoch requested changes Nov 28, 2024

View reviewed changes

carlosfg-flu and others added 12 commits December 2, 2024 07:39

Merge pull request #37 from fluendo/rtph266pay

28da6cf

OCP-5966 rtph266pay

rtph266pay: Add config-interval property

9e87a52

Keep a cache of all Parameter Sets received, then, send them when it's appropriated according with the `config-interval` parameter. Issue: OCP_6005

rtph266pay: Add support for Aggregation Packages

d6bafdf

Add support for two types of aggregation: * zero-latency: Only aggregate Parameter Set NALUs * max: Try to aggregate entires NALUs Issue: OCP_6005

rtph266pay: Amend errata

41c6474

UNKOWN -> UNKNOWN Issue: OCP_6005

fixup! rtph266pay: Add config-interval property

73cc55d

fixup! rtph266pay: Add config-interval property

fe34314

fixup! rtph266pay: Add support for Aggregation Packages

49b25bd

fixup! rtph266pay: Add support for Aggregation Packages

b56c79d

fixup! rtph266pay: Add support for Aggregation Packages

a502bc0

fixup! rtph266pay: Amend errata

ef61da2

fixup! rtph266pay: Add support for Aggregation Packages

2f95f50

rtph266pay: XPS -> PS refactor

7a6bef7

Remove X from XPS. Now it's only Package Set. Issue: OCP_6005

carlosfg-flu added 2 commits December 2, 2024 08:15

rtph266pay: Add explanatory commentaries

db2e363

Explain the more complex part of the algorithm, as it was proveen to be hard to understand. Issue: OCP_6005

rtph266pay: Update NALU_PTR_FORMAT and NALU_ARGS

85c171a

Add missing Nalu fields to these parameters. Issue: OCP_6005

carlosfg-flu force-pushed the rtph266pay_xps branch from 1bcc68c to 85c171a Compare December 2, 2024 07:19

carlosfg-flu requested a review from cfoch December 2, 2024 07:20

cfoch changed the base branch from main to rtph266depay December 2, 2024 11:35

fixup! rtph266pay: Add explanatory commentaries

a1ee00b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCP-6005 rtph266pay: Add config-interval and AP #42

OCP-6005 rtph266pay: Add config-interval and AP #42

carlosfg-flu commented Nov 19, 2024 •

edited

Loading

carlosfg-flu commented Nov 25, 2024

cfoch left a comment

cfoch Nov 27, 2024

carlosfg-flu Nov 29, 2024

cfoch Dec 2, 2024

cfoch Nov 27, 2024

carlosfg-flu Nov 29, 2024

cfoch Dec 2, 2024

cfoch Nov 28, 2024

carlosfg-flu Nov 29, 2024

cfoch Nov 28, 2024

carlosfg-flu Nov 29, 2024 •

edited

Loading

cfoch Nov 28, 2024

carlosfg-flu Nov 29, 2024

carlosfg-flu Nov 29, 2024

cfoch Dec 2, 2024 •

edited

Loading

carlosfg-flu Dec 3, 2024

cfoch Nov 28, 2024

carlosfg-flu Nov 29, 2024

cfoch Dec 2, 2024

carlosfg-flu Dec 3, 2024

OCP-6005 rtph266pay: Add config-interval and AP #42

Are you sure you want to change the base?

OCP-6005 rtph266pay: Add config-interval and AP #42

Conversation

carlosfg-flu commented Nov 19, 2024 • edited Loading

carlosfg-flu commented Nov 25, 2024

cfoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosfg-flu Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfoch Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosfg-flu commented Nov 19, 2024 •

edited

Loading

carlosfg-flu Nov 29, 2024 •

edited

Loading

cfoch Dec 2, 2024 •

edited

Loading