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

Split toxav_bit_rate_set() into two functions to hold the maximum bitrates libvpx supports #578

Merged
merged 1 commit into from Jan 25, 2018

Conversation

xhebox
Copy link

@xhebox xhebox commented Aug 13, 2017

following #572


This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Aug 13, 2017

CLA assistant check
All committers have signed the CLA.

@robinlinden robinlinden added this to the v0.2.0 milestone Aug 22, 2017
@Diadlo
Copy link

Diadlo commented Aug 27, 2017

@xhebox You forget to update toxav/toxav.c

@xhebox
Copy link
Author

xhebox commented Aug 27, 2017

@diablo oh my, sorry, i am so hurried

@SkyzohKey SkyzohKey added CAT:code_cleanup toxav Audio/video P1 High priority labels Nov 3, 2017
@iphydf
Copy link
Member

iphydf commented Jan 13, 2018

@xhebox: Can you rebase this on master and check the checkbox on the right to allow contributors to push to your PR branch? Also, the CI fails, likely because you didn't update toxav.api.h.

@xhebox
Copy link
Author

xhebox commented Jan 13, 2018

@iphydf sorry for the late reply, but there are still some controversial problems, that's why i stop working.. see #572 (comment) , i am actually waiting for a reply about this comment, how do we deal with the '-1' flag? i've commented two options in the next comment, tell me if you have a better idea. i'm willing to continue my work if you give me a clear statement about it.

@nurupo
Copy link
Member

nurupo commented Jan 14, 2018

libvpx not specifying max bitrate and using integers that have implementation dependent widths complicates things a lot. No, this can't be. There must be max birate specified somewhere. Otherwise how is this even supposed to be portable? On one machine unsigned int might be 16 bits, on another 32-bit, yet on another 64 bits. Imagine a machine with 64-bit unsinged int sending video with UINT64_MAX bitrate to a machine with 32-bit unsigned int. It would just overflow. There must be a bitrate limit specified somewhere in VPX video stream protocol definition. Given that unsigned int is specified to hold at least 16 bit integers, my guess would be that the max bitrate would be less or equal to that of UINT16_MAX, but guessing is bad, we should really find the actual bitrate specification rather than do the guessing.

@iphydf
Copy link
Member

iphydf commented Jan 14, 2018

@xhebox I see, thanks. I've replied on that issue.

@xhebox xhebox changed the title Correct the type of toxav_bit_rate_set() Split toxav_bit_rate_set() into two functions to hold the maxium bitrates libvpx supports Jan 14, 2018
@xhebox
Copy link
Author

xhebox commented Jan 14, 2018

@iphydf new commits pushed
@nurupo i've never thought so.👍

@iphydf iphydf modified the milestones: v0.2.0-RC1, v0.2.0 Jan 14, 2018
@iphydf
Copy link
Member

iphydf commented Jan 14, 2018

@xhebox code needs reformatting.

@nurupo
Copy link
Member

nurupo commented Jan 14, 2018

Well, my last comment is somewhat irrelevant to this PR or the issue in question in general, it was more of a reaction on zoff's comment in the issue thread #572 (comment). Should have replied in there instead of here.

@nurupo
Copy link
Member

nurupo commented Jan 14, 2018

@xhebox run astyle on the code to format it, you can find astyle rule file and the command to use at https://github.com/TokTok/c-toxcore/tree/master/other/astyle.

@nurupo
Copy link
Member

nurupo commented Jan 14, 2018

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

because we also use uint

We use uint32_t, not unsigned int, these are different things, they can have different ranges.

I'd say that this function needs to return bit_rate <= UINT_MAX, to check if we would overflow unsigned int with our uint32_t value.


Comments from Reviewable

@iphydf iphydf changed the title Split toxav_bit_rate_set() into two functions to hold the maxium bitrates libvpx supports Split toxav_bit_rate_set() into two functions to hold the maximum bitrates libvpx supports Jan 14, 2018
@xhebox
Copy link
Author

xhebox commented Jan 15, 2018

@nurupo @iphydf OK, new commits pushed, all tests passed, looks good :)

* we may want to prevent from passing overflowed bitrates to libvpx
* more in detail, it's the case where bit_rate is larger than uint, but smaller than uint32_t
*/
return bit_rate > UINT_MAX;
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, I just want to note that it may cause compiler warnings on ILP64 systems (we have never tested any of those, so we have no idea if anything in toxcore works on them, anyway).

Copy link
Author

@xhebox xhebox Jan 17, 2018

Choose a reason for hiding this comment

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

@iphydf I have idea about how to detect if it's ILP64 or LP64(sizeof?). But i think it's fine since most computers are still using int of 4 bytes. And if it's ILP64, uint32_t is not large enough at all. In my view, we could sleep on it for now.

@iphydf iphydf assigned nurupo and Diadlo and unassigned nurupo Jan 16, 2018
@Diadlo
Copy link

Diadlo commented Jan 17, 2018

:lgtm_strong:


Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Jan 21, 2018

Please enable the checkbox "Allow edits from maintainers." on the bottom right. This allows us to squash the commits and rebase on master before merge.

@nurupo can you review this?

@xhebox
Copy link
Author

xhebox commented Jan 21, 2018

@iphydf enabled :)

@zoff99
Copy link

zoff99 commented Jan 25, 2018

@iphydf can you keep the old toxav_bit_rate_set() as a wrapper that just calls the 2 new functions internally? no need to break api "just because we can".

iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 26, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 26, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 26, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 26, 2018
iphydf added a commit to iphydf/go-toxcore-c that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-api that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/qTox that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-api that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-api that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/go-toxcore-c that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/jvm-toxcore-c that referenced this pull request Jan 28, 2018
iphydf added a commit to iphydf/py-toxcore-c that referenced this pull request Jan 28, 2018
@iphydf iphydf added refactor Refactoring production code, eg. renaming a variable, not affecting semantics and removed code cleanup labels May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High priority refactor Refactoring production code, eg. renaming a variable, not affecting semantics toxav Audio/video
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants