-
Notifications
You must be signed in to change notification settings - Fork 907
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
support rtp/rtcp over tcp #2457
Conversation
3a54fd3
to
56f1dc1
Compare
regarding /* NDPI_PROTOCOL_RTP */
u_int32_t rtp_stage:2;
u_int8_t rtp_seq_set[2];
u_int16_t rtp_seq[2];
/* NDPI_PROTOCOL_RTCP */
u_int32_t rtcp_stage:2; I'm not so sure if declaring them direct inside ndpi_flow struct is the right place to define them if(flow->protos.tls_quic.server_names)
ndpi_free(flow->protos.tls_quic.server_names); which caused segfault. also, i'm not completely sure if that was the reason for trying to free an object that wasn't allocated, the above explanation is my analysis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it is quite good!
Just some minor issues below.
Furthermore:
- try to merge together the 2 RTP traces, if possible
- rebase
You got it right: you can write to |
4c5e83c
to
3cc92ec
Compare
for merging 2 rtp traces, here there are 3 traces 2 of them have the same rtp flow and the third one has fragmented packets (that are not covered in this PR) |
@mmaatuq , sorry, I have not been clearer: I was asking to merge "rtp_tcp.pcapng" that you added to the PR and the existing "rtp.pcapng" already in the repository. This way we have only one RTP trace in the tests directory... |
* support rtp/rtcp over tcp as per rfc4571. Signed-off-by: mmaatuq <[email protected]>
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmaatuq, thanks for this work
Please sign (check) the below before submitting the Pull Request:
Link to the related issue:
Describe changes: