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

[core] make sure TTL will not drop packets over last block #2005

Merged

Conversation

gou4shi1
Copy link
Contributor

The msg seq of m_pLastBlock is undefined, right?

@gou4shi1 gou4shi1 changed the title [weride] make sure TTL will not drop packets over last block [core] make sure TTL will not drop packets over last block May 16, 2021
@gou4shi1 gou4shi1 force-pushed the make_sure_ttl_drop_not_overflow branch from f21a45f to eee207d Compare May 16, 2021 06:10
@codecov-commenter

This comment has been minimized.

@gou4shi1
Copy link
Contributor Author

any comment?

@maxsharabayko
Copy link
Collaborator

Your improvement looks good.
However, seems like another loop at the beginning of the function is also not very bullet-proof: no checks for p == NULL and for m_pLastBlock.

    for (int i = 0; i < offset; ++i)
        p = p->m_pNext;

@gou4shi1
Copy link
Contributor Author

I have check for this loop, although return -2 is ugly :lol:

@ethouris
Copy link
Collaborator

As long as you add the explanation of the return values in the function's documentation, there's no problem with it.

srtcore/buffer.cpp Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko added this to the v1.4.4 milestone May 27, 2021
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels May 27, 2021
@gou4shi1
Copy link
Contributor Author

I have just checked the code in buffer.cpp/h, p should never be NULL, it's a ring buffer, right?

@maxsharabayko
Copy link
Collaborator

I have just checked the code in buffer.cpp/h, p should never be NULL, it's a ring buffer, right?

Yes. Packets are added sequentially to the buffer, and there should be no gaps.
See CUDT::sendmsg2.

@ethouris
Copy link
Collaborator

Originally they didn't even have sequence numbers until they were sent out for the first time. This has changed only with the group code, and it's only for synchronization. There shouldn't ever happen a case of existing a gap in the sender buffer, even in the group code, in which case there may happen an "initial shift" due to link activation that causes a difference between ISN and the first packet sequence, but this is resolved by wiping out this initial part of the buffer and causing an artificial loss that is ignored at the receiver side.

@ethouris
Copy link
Collaborator

Something's wrong. Can you please merge against the latest master ?

@gou4shi1
Copy link
Contributor Author

I don't known why add a log will make the unit test failed... it success in my local

srtcore/buffer.cpp Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko merged commit 17fee15 into Haivision:master May 31, 2021
@gou4shi1
Copy link
Contributor Author

thx for the merge :)

@gou4shi1 gou4shi1 deleted the make_sure_ttl_drop_not_overflow branch May 31, 2021 11:51
@maxsharabayko
Copy link
Collaborator

@gou4shi1 Thank you for the contribution! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants