Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RtpPacket::SetPayloadLength get wrong payloadLength #1379

Closed
Lynnworld opened this issue Apr 21, 2024 · 2 comments · Fixed by #1380
Closed

RtpPacket::SetPayloadLength get wrong payloadLength #1379

Lynnworld opened this issue Apr 21, 2024 · 2 comments · Fixed by #1380
Labels

Comments

@Lynnworld
Copy link
Contributor

Hi,
this method padding length to 4 bytes, however it change real payloadLength and remove padding flags , is that the correct ?
I thing we should check padding length is the same as orig length, if padding happen then set padding flag to true and set padding byte to padding length.

void RtpPacket::SetPayloadLength(size_t length)
	{
		MS_TRACE();

		// Pad desired length to 4 bytes.
		length = static_cast<size_t>(Utils::Byte::PadTo4Bytes(static_cast<uint16_t>(length)));

		this->size -= this->payloadLength;
		this->size -= size_t{ this->payloadPadding };
		this->payloadLength  = length;
		this->payloadPadding = 0u;
		this->size += length;

		SetPayloadPaddingFlag(false);
}

https://github.com/versatica/mediasoup/blob/d6373d540751c06b7fb96f0e7e26ad56f50f25bd/worker/src/RTC/RtpPacket.cpp#L653

@Lynnworld Lynnworld added the bug label Apr 21, 2024
ibc added a commit that referenced this issue Apr 22, 2024
Fixes #1379

### Details

- Make `RtpPacket::SetPayloadLength()` automatically adjust padding so padding+payload is padded to 4 bytes.
@ibc
Copy link
Member

ibc commented Apr 22, 2024

Yes, the API is a bit unfortunate. Improving it here: #1380

@ibc
Copy link
Member

ibc commented Apr 22, 2024

Finally this is not gonna done that way. All methods in RtpPacket will remove padding if present. Rationale in the PR.

@ibc ibc closed this as completed in #1380 Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants