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

Don't throw away rtp packets from old Toxcore #831

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

robinlinden
Copy link
Member

@robinlinden robinlinden commented Mar 5, 2018

Resolves #812.
PR from @zoff99's code.


This change is Reviewable

@zoff99
Copy link

zoff99 commented Mar 6, 2018

:lgtm_strong:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@zoff99
Copy link

zoff99 commented Mar 6, 2018

@iphydf @robinlinden can we release just this as 0.2.1 (after review) ?
so that new clients will be able to video call with old clients

@zoff99 zoff99 added bug Bug fix for the user, not a fix to a build script P0 Critical priority labels Mar 6, 2018
@@ -467,7 +467,8 @@ static int handle_rtp_packet(Messenger *m, uint32_t friendnumber, const uint8_t
return -1;
}

if (header.offset_full >= header.data_length_full) {
if (header.offset_full >= header.data_length_full
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about: if (header.data_length_full != 0 && header.offset_full >= header.data_length_full)? I.e. either we don't have a full data length, or the offset can't be greater-equal it.

@iphydf iphydf added this to the v0.2.1 milestone Mar 9, 2018
@iphydf iphydf merged commit 5da8c8d into TokTok:master Mar 9, 2018
@robinlinden robinlinden deleted the old-new-rtp-fix branch August 13, 2018 19:17
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fix for the user, not a fix to a build script P0 Critical priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants