Rtp header padding #1079

buptlsp · 2023-05-12T10:20:55Z

what I have done:

npm run test:worker, the result as follow, all test passed.
test in online process. the result as follow:

the bandwidth has decreased by 5% -10%
the memory of machine has decreased by about 10%
the CPU consumption of machine seems to have remained unchanged

make fuzzer . I haven't used fuzzer before, I couldn't run it successfully. Can you help me try this? Thank you

ibc

I'll review next week in depth but overall code doesn't respect existing code guidelines. Please run "make lint" and "make format" in worker/ folder. Also leave blank space after logs and use auto or auto* or const auto or const auto* where possible as in the whole code. In addition, comments must start with capital letter and end with a dot.

ibc · 2023-05-12T10:38:56Z

worker/fuzzer/src/RTC/FuzzerRtpPacket.cpp

@@ -165,8 +165,9 @@ void Fuzzer::RTC::RtpPacket::Fuzz(const uint8_t* data, size_t len)
 	packet->HasExtension(14);
 	packet->GetExtension(14, extenLen);
 	packet->ReadTransportWideCc01(wideSeqNumber);
-	packet->UpdateTransportWideCc01(12345u);
-	packet->SetExtensionLength(14, 2);
+	//packet->UpdateTransportWideCc01(12345u);


Why are those tests commented? We cannot leave commented tests. Do they no longer work or what?

this two line can be replace by setExtensionValue(). should I just delete the 2 line?

We should test as much as possible

We should test as much as possible

Extremely agree. This is my first time writing C++, and I have been writing PHP before. So the more tests, the better

buptlsp · 2023-05-12T10:51:37Z

I'll review next week in depth but overall code doesn't respect existing code guidelines. Please run "make lint" and "make format" in worker/ folder. Also leave blank space after logs and use auto or auto* or const auto or const auto* where possible as in the whole code. In addition, comments must start with capital letter and end with a dot.

It's my first time to write pr of mediasoup. I will run make lint and make format. and I will check the format problem

buptlsp · 2023-05-12T11:57:24Z

I have fix the format. if there is any problem, notify me any time. the header padding is as follow:

btw, I found other two problems about the rtp packet when i use wireshark to capture the packet. I think is not related to this pr because it also occured before my code change. I will fix them next time.

worker/src/RTC/RtpPacket.cpp

jmillan · 2023-05-23T14:15:20Z

@buptlsp, since you've based the new SetExtensionValue() on the existing ParseExtensions() method do please reuse the code as much as possible for a better readability. Reuse the structure, the variable names, the comments.., So we can clearly see the difference between both methods at a first glance.

And please write an explanatory comment at least on top of every condition (most of them are already present from original code). The code needs to be understood now and in the coming years, and having to go through all code lines to understand it makes it time consuming in the short and long term.

buptlsp · 2023-05-24T09:16:24Z

@buptlsp, since you've based the new SetExtensionValue() on the existing ParseExtensions() method do please reuse the code as much as possible for a better readability. Reuse the structure, the variable names, the comments.., So we can clearly see the difference between both methods at a first glance.

And please write an explanatory comment at least on top of every condition (most of them are already present from original code). The code needs to be understood now and in the coming years, and having to go through all code lines to understand it makes it time consuming in the short and long term.

in fact, I have reuse the code as much as I can. the method is differ to parseExtensions, I copy from two method mainly : ParseExtensions, setExtensions。 most structure and variable names cames from this two methods. but the method setExtensionValue is more complicated because the padding can be everywhere and the data may override.

I can add some more comments to explain the code.

jmillan

I'll review this properly next week.

jmillan · 2023-05-25T09:41:51Z

worker/src/RTC/RtpPacket.cpp

-
-			if (len != currentLen)
+
+			// If len == currentLen, just memcpy


Comments start with Uppercase and end with dot (.).

jmillan · 2023-05-29T10:45:22Z

@buptlsp, few comments:

SetExtensionValue() does not need to consider padding bits between header values because we generate such buffer and only add padding at the end of the header extensions buffer (see RtpPacket::SetExtensions).

The previous point allows for ptr1 not to be needed in SetExtensionValue() which will make the code cleaner. Also would require a single memmove operation to be done, just for the header we are setting.

In pseudo code, the new extensions would be rewritten as:

			// If len != currentLen, means we may need to shift extension or payload.
			else
			{
				uint8_t* extensionStart      = reinterpret_cast<uint8_t*>(this->headerExtension) + 4;
				uint8_t* extensionEnd        = extensionStart + GetHeaderExtensionLength();
				uint8_t* ptr                          = extensionStart;
				size_t extensionsTotalSize = 0;

				while (ptr < extensionEnd)
				{
					const uint8_t tempExtensionId = (*ptr & 0xF0) >> 4;
					const size_t tempExtensionLen = static_cast<size_t>(*ptr & 0x0F) + 1;

					// tempExtensionId=15 in One-Byte extensions means "stop parsing here".
					if (tempExtensionId == 15u)
                                          {
						break;
                                          }
                                          // Extension id 0. We already reached the end of the extension header.
                                          // NOTE: Padding only exists at the end of the extension header for outgoing RTP packet.
					if (tempExtensionId == 0u)
                                          {
                                               break:
                                          }
					// Valid extension id.
					else
					{
                                                // This is the extension which length and value is being set. 
						if (tempExtensionId == id)
						{
							std::memmove(ptr, value.c_str(), len + 1);
                                                        ptr += len + 1;
            						extensionsTotalSize += len + 1;
						}
                                                  else
						{
							ptr += tempExtensionLen + 1;
            						extensionsTotalSize += tempExtensionLen + 1;
						}
             
                  				// Store the One-Byte extension element.
						// `-1` because we have 14 elements total 0..13 and `id` is in the range 1..14.
						this->oneByteExtensions[tempExtensionId - 1] =
					             reinterpret_cast<OneByteExtension*>(ptr);
					}
				}

This is much cleaner approach IMHO and reduces the number of memmove() calls to just the header being set.

buptlsp · 2023-05-30T09:05:33Z

@jmillan Have you read the issue #1073 I mentioned? I don't think your code will solve the problem. the padding indeed existed between headers.

jmillan · 2023-05-30T11:03:54Z

the padding indeed existed between headers.

It's an accidental padding in MID extension:

We are defining a RTC::MidMaxLength of 8 bytes.
We are providing RtpPacket::SetExtensions() with a buffer of size 8 and exten len 8 for the MID extension.
Then we set the mid extension value (if the Consumer RtpParameters do define a mid value).
When setting the mid value, it's usually less than 8 bytes, so the exten len is modified and the remaining bits are left to 0, thus creating padding.

IMHO we should:

In Producer::MangleRtpPacket() create the MID extension with length of 1 byte and assign it a value of 0.
- This way if the Consumer does not define mid we waste the less possible bandwidth.
- This way we don't create artificial padding.
- NOTE: We already set default values for other extensions (see the indicated method).
Later on, RtpPacket::UpdateMid() needs to check if the provided value is less than RTC::MidMaxLength and otherwise log error.
Then my proposed approach will work (no padding between extension headers) and the header resize will be more clean and efficient.

Do you agree @ibc?

buptlsp · 2023-05-30T11:50:00Z

I don't think it‘s a good idea. Your method may be cleaner, but there are a lot of prerequisites. My method does not need to have prerequisites and assumptions like yours. One day, we may add other extensions, or we may introduce other padding, and at that time my method will still be valid.

jmillan · 2023-05-30T12:21:15Z

Your method may be cleaner, but there are a lot of prerequisites

What prerequisites?

The fact that every outgoing extension header should have a default value? That's indeed something desirable to have the minimum possible overhead for unset headers.
The fact that we don't create artificial padding between header extensions? That's indeed desirable too, and it's not a prerequisite but an implementation detail, which is positive and BTW is already done if the previous point is satisfied.

ibc · 2023-05-30T13:08:37Z

While @jmillan's proposal makes sense I would also like to know about those "prerequisites" and those potential problems if we add more extensions in the future.

jmillan · 2023-05-30T13:58:21Z

While @jmillan's proposal makes sense I would also like to know about those "prerequisites" and those potential problems if we add more extensions in the future.

There are no prerequisites, but a single contract:

Producer::MangleRtpPacket() always needs to set a default value for every extension.

We already to that for every extension but for MID.

My proposal is that we set a default value of 0, with a length of 1 for MID. It will optionally be rewritten with a new value/length. In the future, we need to follow the same policy: always set a default value for new extensions. If that extension is optional, like MID, then use the smallest possible value. (value 0, length 1).

buptlsp · 2023-05-31T03:24:42Z

Well, let's go along with your idea that the mid length is now 1 byte. The mid value can appear anywhere in the header. (It's the first header now, but you can't assume it's forever). Suppose the new mid value's length is 3. What do we need to do when updateMid()?

there is not enough memory to put the value. we need to move the header to make sure it has enough memory. all headers after mid should change their place.
Note: If you move the header backwards, you have to be careful to overwrite the payload.
we need to recalculate the header length. If there is other padding before, we may not need to increase the length again.
we may need to move the payload backwards due to the increased length.

You've done a bunch of operations like setting the default value and setting it to the minimum, which just reduces the situation where you think the length will only increase and the payload will not move forward. and after all operation you have done, your code is not as clean as your think.

now you have change the mid length to 3. but the Router has lots of consumers, this packet will update other mid after you have change it to length 3. not the new length is 1, what should you do now?

My method is called setExtensionValue, and we may make an extension longer or shorter, and the payload may move forward or backward. Your method is entirely for setExtensions, not a method that can be used anywhere.

jmillan · 2023-05-31T09:27:31Z

@buptlsp

Your method is entirely for setExtensions, not a method that can be used anywhere.

No, my change was indeed for SetExtensionValue(). It's behaviour is identical to yours with the exception that it just moves memory on the position where the extension being set resides. All extensions lie on the same memory buffer so such move will affect all extensions that come after it.

Then total extension length and padding needs to be recalculated, just as in your implementation.

I can directly implement it when I get the time so we can simply compare it.

buptlsp · 2023-05-31T10:01:16Z

I have already put my code online in my environment. the cpu, memory and bandwidth result I have give in the pr. So, I am not in a hurry to solve this problem． after all, I have already saved money for my company.

I don't know your pseudo well. maybe when you complete the code ,we can compare and discuss later.

jmillan

After re-reviewing I'm fine with the implementation.

I've added few change requests.

jmillan · 2023-06-02T14:46:22Z

worker/src/RTC/RtpPacket.cpp

+				uint8_t* extensionEnd      = extensionStart + GetHeaderExtensionLength();
+				uint8_t* ptr               = extensionStart;
+				size_t extensionsTotalSize = static_cast<size_t>(len + 1);
+				uint8_t* ptr1              = ptr;


Please, rename to lastExtension, since it points exactly to the last extension that has been evaluated, while ptr keeps moving forward among the 0s.

jmillan · 2023-06-02T14:48:31Z

worker/src/RTC/RtpPacket.cpp

+						*ptr1 = 0u;
+						ptr1++;


can we add here a single memset in order to fill with zeros at once?

jmillan · 2023-06-02T14:49:06Z

worker/src/RTC/RtpPacket.cpp

+				uint8_t* extensionEnd      = extensionStart + GetHeaderExtensionLength();
+				uint8_t* ptr               = extensionStart;
+				size_t extensionsTotalSize = static_cast<size_t>(len + 2);
+				uint8_t* ptr1              = ptr;


same naming changes please.

jmillan · 2023-06-02T14:49:17Z

worker/src/RTC/RtpPacket.cpp

+						*ptr1 = 0u;
+						ptr1++;


single memset here.

jmillan

After re-reviewing I'm fine with the implementation.

I've added few change requests.

jmillan · 2023-06-02T15:15:59Z

worker/src/RTC/RtpPacket.cpp

-			// In One-Byte extensions value length 0 means 1.
-			extension->len = len - 1;
+				std::memmove(this->payload + shift, this->payload, this->payloadLength + this->payloadPadding);
+				// Clear the shift place, if shift > currentLen, may create some strange extension.


Suggested change

// Clear the shift place, if shift > currentLen, may create some strange extension.

// Fill padding with zeroes.

jmillan · 2023-06-02T15:23:19Z

worker/src/RTC/RtpPacket.cpp


-			// In One-Byte extensions value length 0 means 1.
-			extension->len = len - 1;
+				std::memmove(this->payload + shift, this->payload, this->payloadLength + this->payloadPadding);


Is payloadPadding needed here? it's only valid to point the padding for parsed packets, but has no relevance otherwise.

jmillan · 2023-06-02T15:26:04Z

worker/src/RTC/RtpPacket.cpp

+				{
+					// Means move has end. Fill the rest buffer to 0u.
+					if (ptr >= extensionEnd)
+					{


In which case are we entering this condition?

jmillan

Just some cosmetic changes.

The PR is good to go for me. I'd like @ibc to review and approve it before merging.

jmillan · 2023-06-13T09:54:55Z

worker/src/RTC/RtpPacket.cpp

+	 *  This method only can set exists extension. if len of value changed, this method will
+	 * recaculate and fill padding.


Suggested change

* This method only can set exists extension. if len of value changed, this method will

* recaculate and fill padding.

* This method set the value for an existing extension. If the length of the current value changed,

* it will recalculate it and regenerate the header extension accordingly.

jmillan · 2023-06-13T09:55:33Z

worker/src/RTC/RtpPacket.cpp

+			  currentLen,
+			  value.c_str());
+
+			// If len == currentLen, just memcpy.


Suggested change

// If len == currentLen, just memcpy.

// If len == currentLen, just copy the new value.

jmillan · 2023-06-13T09:56:26Z

worker/src/RTC/RtpPacket.cpp

+						if (lastExtensionPtr < ptr)
+						{
+							std::memmove(lastExtensionPtr, ptr, tempExtensionLen + 1);
+							// the extension has move from ptr to lastExtensionPtr, store the One-Byte extension


Suggested change

// the extension has move from ptr to lastExtensionPtr, store the One-Byte extension

// The extension has moved from ptr to lastExtensionPtr, store the One-Byte extension

jmillan · 2023-06-13T09:57:02Z

worker/src/RTC/RtpPacket.cpp

+					}
+				}
+
+				// Means move has end. Fill the rest buffer to 0u.


Suggested change

// Means move has end. Fill the rest buffer to 0u.

// Fill the rest buffer to 0u.

jmillan · 2023-06-13T09:57:57Z

worker/src/RTC/RtpPacket.cpp

@@ -618,12 +705,82 @@ namespace RTC

 			auto* extension = it->second;
 			auto currentLen = extension->len;
+			// If len == currentLen, just memcpy.


Suggested change

// If len == currentLen, just memcpy.

// If len == currentLen, just copy the new value.

jmillan · 2023-06-13T09:58:16Z

worker/src/RTC/RtpPacket.cpp

+				uint8_t* ptr               = extensionStart;
+				size_t extensionsTotalSize = static_cast<size_t>(len + 2);
+				uint8_t* lastExtensionPtr  = ptr;
+				// Clear current extension, valueLen + 2 byteheader. we will rewrite after move all


Suggested change

// Clear current extension, valueLen + 2 byteheader. we will rewrite after move all

// Clear current extension, valueLen + 2 byteheader. We will rewrite after moving all

jmillan · 2023-06-13T09:58:35Z

worker/src/RTC/RtpPacket.cpp

+				size_t extensionsTotalSize = static_cast<size_t>(len + 2);
+				uint8_t* lastExtensionPtr  = ptr;
+				// Clear current extension, valueLen + 2 byteheader. we will rewrite after move all
+				// extension in right place.


Suggested change

// extension in right place.

// extensions to the right place.

jmillan · 2023-06-13T09:58:53Z

worker/src/RTC/RtpPacket.cpp

-			// Fill with 0's if new length is minor.
-			if (len < currentLen)
-				std::memset(extension->value + len, 0, currentLen - len);
+				// Means move has end. Fill the rest buffer to 0u.


Suggested change

// Means move has end. Fill the rest buffer to 0u.

// Fill the rest buffer to 0u.

ibc · 2023-06-13T10:18:24Z

I'm on vacation so give some days before reviewing this. For now just some concerns:

Does this PR include proper C++ tests for the new behavior/feature? I'd like to see tests that create a RTP packets, add RTP extensions and asserts that the total RTP packet size matches the expected size.
Does this PR include the new API into fuzzer code so it's guaranteed that nothing can crash mediasoup?

buptlsp · 2023-06-14T07:17:08Z

@jmillan I have already changed the comment.
@ibc I have add some complex test cases. the result is as follow:

I don't know how to use fuzzer, can you help to solve this problem please?

ibc · 2023-06-27T16:39:13Z

@buptlsp:

@jmillan and me have had a long call today about this PR. We entirely understand the motivation and the goal, plus the code obviously works as expected. However we have decided to go another way and do it even better, so we are gonna close this PR.

Please check this story in which I've documented what we are gonna do next:

#1107

buptlsp added 4 commits May 6, 2023 11:30

fix strang rtp header padding

e50bfad

comment SetExtenstionLength

3794264

fix bug and complete test code

8f04206

fix bug

aa2ebe8

ibc requested changes May 12, 2023

View reviewed changes

fix code format

f4feadf

buptlsp added 3 commits May 19, 2023 18:38

fix ptr1++ first brings bug

b3136a3

fix ptr1++ first brings bug

d5aa6eb

fix while condition

250280b

jmillan reviewed May 22, 2023

View reviewed changes

jmillan reviewed May 23, 2023

View reviewed changes

worker/src/RTC/RtpPacket.cpp Show resolved Hide resolved

Add comments and change format

d411a31

jmillan reviewed May 25, 2023

View reviewed changes

add comment dot

34cf193

jmillan reviewed Jun 2, 2023

View reviewed changes

single Memset and change ptr1 to lastExtensionPtr

b35933b

jmillan reviewed Jun 13, 2023

View reviewed changes

add Test cases and modify comment

0d50648

add test cases for Two-Byte header

627a95a

ibc mentioned this pull request Jun 27, 2023

Set RTP extensions on sending transport side #1107

Open

ibc closed this Jun 27, 2023

	// Clear the shift place, if shift > currentLen, may create some strange extension.
	// Fill padding with zeroes.

		* This method only can set exists extension. if len of value changed, this method will
		* recaculate and fill padding.

	// If len == currentLen, just memcpy.
	// If len == currentLen, just copy the new value.

	// the extension has move from ptr to lastExtensionPtr, store the One-Byte extension
	// The extension has moved from ptr to lastExtensionPtr, store the One-Byte extension

	// Means move has end. Fill the rest buffer to 0u.
	// Fill the rest buffer to 0u.

	// Clear current extension, valueLen + 2 byteheader. we will rewrite after move all
	// Clear current extension, valueLen + 2 byteheader. We will rewrite after moving all

	// extension in right place.
	// extensions to the right place.

Rtp header padding #1079

Rtp header padding #1079

Conversation

buptlsp commented May 12, 2023

ibc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buptlsp May 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buptlsp commented May 12, 2023 • edited

buptlsp commented May 12, 2023 • edited

jmillan commented May 23, 2023

buptlsp commented May 24, 2023 • edited

jmillan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillan commented May 29, 2023 • edited

buptlsp commented May 30, 2023 • edited

jmillan commented May 30, 2023 • edited

buptlsp commented May 30, 2023

jmillan commented May 30, 2023

ibc commented May 30, 2023

jmillan commented May 30, 2023 • edited

buptlsp commented May 31, 2023 • edited

jmillan commented May 31, 2023 • edited

buptlsp commented May 31, 2023

jmillan 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

jmillan left a comment

Choose a reason for hiding this comment

jmillan Jun 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillan 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

ibc commented Jun 13, 2023

buptlsp commented Jun 14, 2023

ibc commented Jun 27, 2023

buptlsp May 12, 2023 •

edited

buptlsp commented May 12, 2023 •

edited

buptlsp commented May 12, 2023 •

edited

buptlsp commented May 24, 2023 •

edited

jmillan commented May 29, 2023 •

edited

buptlsp commented May 30, 2023 •

edited

jmillan commented May 30, 2023 •

edited

jmillan commented May 30, 2023 •

edited

buptlsp commented May 31, 2023 •

edited

jmillan commented May 31, 2023 •

edited

jmillan Jun 2, 2023 •

edited