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

Fix ToxAv's use of struct Tox. #1064

Merged
merged 1 commit into from
Aug 13, 2018
Merged

Fix ToxAv's use of struct Tox. #1064

merged 1 commit into from
Aug 13, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 11, 2018

This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Aug 11, 2018
@iphydf iphydf force-pushed the toxav-get-tox branch 3 times, most recently from a1a9b19 to 71f66d8 Compare August 12, 2018 10:37
@iphydf iphydf changed the title Fix toxav_get_tox to return tox, not messenger. Fix ToxAv's use of struct Tox. Aug 12, 2018
@iphydf iphydf requested a review from zoff99 August 12, 2018 10:39
@iphydf iphydf force-pushed the toxav-get-tox branch 2 times, most recently from ddc3b60 to 30d6976 Compare August 12, 2018 11:05
zoff99
zoff99 previously requested changes Aug 12, 2018
Copy link

@zoff99 zoff99 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained (waiting on @zoff99)


toxav/video.c, line 348 at r1 (raw file):

    for (vpx_image_t *dest = vpx_codec_get_frame(vc->decoder, &iter);
            dest != nullptr;
            dest = vpx_codec_get_frame(vc->decoder, &iter)) {

be very careful with this loop. it works against common logic. i ran into problems here before. tbh i would not change this one for the moment, and just mark it TODO.
this needs to be tested with Video tests, to see if u are getting double or empty frames.

@iphydf iphydf force-pushed the toxav-get-tox branch 14 times, most recently from d07c736 to 01e2eac Compare August 12, 2018 23:34
* Fix `toxav_get_tox` to return tox, not messenger.
* Fix the casts from Tox* to Messenger* in toxav_old.c.
* Pass Tox instead of Messenger to public group AV callbacks.
Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zoff99)


toxav/video.c, line 348 at r1 (raw file):

Previously, zoff99 (Zoff) wrote…

be very careful with this loop. it works against common logic. i ran into problems here before. tbh i would not change this one for the moment, and just mark it TODO.
this needs to be tested with Video tests, to see if u are getting double or empty frames.

Moved it out to a different PR.

@iphydf iphydf dismissed zoff99’s stale review August 13, 2018 22:17

The code this comment was about has moved to a different PR.

@iphydf iphydf merged commit 2d84681 into TokTok:master Aug 13, 2018
@iphydf iphydf deleted the toxav-get-tox branch August 13, 2018 22:22
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.6 Aug 14, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants