-
Notifications
You must be signed in to change notification settings - Fork 848
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
Update inconsistent header size calculation #2662
Conversation
Updated inconsistent header size calculation
The constant you have changed here refers to the UDP header size and was never intended for anything else. |
Did you add documentation explicitly stating why it's using only the UDP header and not including the SRT header?? If not, then why did you close this?? It seems like you didn't even read what I put in the description of the PR before closing it... |
Ok, I admit the initial view of the changes has tricked me. Still, if this is saying it's UDP header, then it includes only IPv4 and UDP header, so I think it should be fixed a little bit differently. |
Ah, line 92/95 should get a similar fix. I could not propose it unfortunately because git doesn't allow to do this in a "non-changed" area. |
Hi @aaron-jencks // Maximum SRT payload size. m_config.iMSS = SRTO_MSS.
m_iMaxSRTPayloadSize = m_config.iMSS - CPacket::UDP_HDR_SIZE - CPacket::HDR_SIZE;
// The RCV queue is initialized with s->core().maxPayloadSize() = m_iMaxSRTPayloadSize .
// Internally a CUnit structure is created with CPacket holding SRT header + SRT payload.
m.m_pRcvQueue->init(128, s->core().maxPayloadSize(), m.m_iIPversion, 1024, m.m_pChannel, m.m_pTimer);
m_pUnitQueue = new CUnitQueue(qsize, (int)payload = s->core().maxPayloadSize()); The RCV buffer then stores |
Ok, let me be clear on one thing: The MSS (that is, size of the MTU) is only symbolically used to comply with the system value of MTU, but this value as such is not used in any internally defined size, only in intermediate calculations. The total number of bytes for the sender and receiver buffer is the number of bytes that the application is going to use. So as per a single packet treated as MTU: it does include the payload part AND the SRT header, but NOT the IPv4+UDP header. The actualy size, however, depends on the MSS value. This means that: bytes per packet = MSS - UDP_HEADER; default: 1500 - 28. And this is for IPv4 only, of course. This part of the code defines in short how the receiver buffer size in packets is defined:
Or, in short: the size of the buffers is not the number of bytes allowed to carry out the user's payload, but the size of the memory - additionally aligned to mutliplicity of the UDP payload size - that the application will allocate for the buffer. |
Co-authored-by: Sektor van Skijlen <ethouris@gmail.com>
Co-authored-by: Sektor van Skijlen <ethouris@gmail.com>
I think I've remedied this, take a look |
Okay, I think I've fixed everything, it should be ready for review again |
@maxsharabayko please review if this fix is ok because I personally have doubt. Not sure what this whole document is all about, so once again:
|
The document advises how to calculate a value to be set in |
I don't actually know what the correct thing is here, but when I read the documentation, it was unclear if UDPHDR_SIZE was supposed to be 28, or 44 and the reasoning for the difference between the two wasn't well defined. I'm sure what you guys have isn't wrong, it's just unclear |
Ok, I was also a bit confused. I submitted a fix for the description - this should look clearer now. If you have any further suggestions you're welcome to add them there. |
Updated inconsistent header size calculation
Not sure why the 16 SRT header bytes aren't included here, but if they really shouldn't be included in the calculation, then an explanation as to why should be included in the document as currently it is unclear and directly contradicts the use of the number 44 in lines 63-66.