-
Notifications
You must be signed in to change notification settings - Fork 285
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 high quality video sending. #629
Conversation
it would be good that this gets tested first, to see if this solution does actually work as intended |
The compiler complains about some things. For example
Undefined behaviour is very bad and it allows the compiler to pretty much treat the code as if it will never be reached and optimize it with this in mind, which can result in very bizarre optimizations. I would suggest fixing the errors first before testing the PR, as they might affect your testing. |
@nurupo what compiler flags are you using? what are the minimum flags to use? |
also, has anybody ever used toxcore on big-endian? i am not sure that all the already existing code is endianness safe |
The default ones on CMake: Lines 53 to 112 in 0fce3fc
|
You can see what cmake command Travis executes in the build log. It's
You would probably want to disable You might want to additionally set
Someone did a year or two ago and there were indeed a couple of issues, but I believe they got fixed. The ones that I remember were about the When toxcore was initially written there was little regard for portability and there weren't many code reviews done. Code quality wasn't very good (but it wasn't very bad either). Things changed after the TokTok fork, code quality requirements became more strict and code reviews became required, CI is testing for a lot more of things now, and we ourselves have learned a great deal of stuff in the process. Most of the improvements were done in toxcore, not toxav, toxencryptsave or others, so that's why you might see not as good code in some places, especially in non-toxcore subdirs. |
i am pretty sure video will not work correctly on bigendian. does anybody have a bigendian system with toxcore to test video? |
@zoff99 Travis still doesn't like it: https://travis-ci.org/TokTok/c-toxcore/jobs/312062622#L2205 |
@robinlinden i will check this. |
includes:
|
Do you consider this ready for review? |
@sudden6 in terms of functionality yes. before reviewing, it would be best if a few people could test this if it works as expected? |
@Diadlo i am sure bigendian does not work with current release version also. so it will not work with my fix aswell. |
Ok, I went over this huge PR and marked a lot of the stuff I think absolutely needs changes. It took 1,5h... To accelerate this the next time this PR should be split into at least 3 parts
Not sure if it makes sense to do that over git or just copy the state of the relevant to new branches each. Some general stuff:
Review status: 0 of 15 files reviewed at latest revision, 57 unresolved discussions, some commit checks broke. circle.yml, line 124 at r2 (raw file):
Should this be part of this PR or has this PR a dependency on something else? toxav/audio.c, line 60 at r2 (raw file):
line length toxav/audio.c, line 76 at r2 (raw file):
line length toxav/audio.c, line 134 at r2 (raw file):
still relevant? toxav/audio.c, line 136 at r2 (raw file):
comment out of sync with the code? toxav/audio.h, line 32 at r2 (raw file):
A comment explaining what this setting does would be good, does it buffer samples, packets? toxav/audio.h, line 38 at r2 (raw file):
Are If separate settings are really needed, please make the naming convention more uniform, I suggest toxav/bwcontroller.c, line 35 at r2 (raw file):
Is there any reasoning behind the chosen values and can you put that in the comments? toxav/bwcontroller.c, line 42 at r2 (raw file):
Remove empty comment toxav/bwcontroller.c, line 98 at r2 (raw file):
C99 allows to do toxav/bwcontroller.c, line 123 at r2 (raw file):
delete instead of disabling toxav/bwcontroller.c, line 145 at r2 (raw file):
delete instead of disabling toxav/bwcontroller.c, line 197 at r2 (raw file):
remove debugging comment toxav/bwcontroller.c, line 208 at r2 (raw file):
delete instead of disabling toxav/bwcontroller.c, line 220 at r2 (raw file):
remove testing code toxav/bwcontroller.c, line 228 at r2 (raw file):
outer float cast is unneeded because the calculation is already in float toxav/bwcontroller.c, line 265 at r2 (raw file):
why removed? toxav/bwcontroller.c, line 270 at r2 (raw file):
outer float cast unneeded toxav/ring_buffer.c, line 28 at r2 (raw file):
Why does a generic ring buffer need a byte for the data type? toxav/ring_buffer.c, line 48 at r2 (raw file):
fill out toxav/rtp.c, line 103 at r2 (raw file):
If without effect toxav/rtp.c, line 165 at r2 (raw file):
does that mean keyframes allways have the highest bit set and others are guaranteed to not have it set? toxav/rtp.c, line 169 at r2 (raw file):
Are you sure you don't want to compare against toxav/rtp.c, line 172 at r2 (raw file):
It's needed to do the cast like this toxav/rtp.c, line 217 at r2 (raw file):
why do you cast toxav/rtp.c, line 261 at r2 (raw file):
remove unneccessary comments toxav/rtp.c, line 339 at r2 (raw file):
why uncommented? toxav/rtp.c, line 343 at r2 (raw file):
formatting toxav/rtp.c, line 368 at r2 (raw file):
Could use some documentation toxav/rtp.c, line 376 at r2 (raw file):
This function could use some documentation what it's doing, because it's really complex toxav/rtp.c, line 385 at r2 (raw file):
use C99 for toxav/rtp.c, line 393 at r2 (raw file):
It seems to me, from here on, this toxav/rtp.c, line 410 at r2 (raw file):
debug message needed? toxav/rtp.c, line 509 at r2 (raw file):
change to toxav/rtp.c, line 594 at r2 (raw file):
This comment makes no sense to me toxav/rtp.c, line 620 at r2 (raw file):
A reason "why" this should never happen would be very helpful toxav/rtp.c, line 661 at r2 (raw file):
remove toxav/rtp.c, line 683 at r2 (raw file):
you can merge toxav/rtp.c, line 698 at r2 (raw file):
remove ifdefed out code if not needed toxav/rtp.c, line 746 at r2 (raw file):
remove toxav/rtp.c, line 825 at r2 (raw file):
one time is probably enough toxav/rtp.h, line 35 at r2 (raw file):
a comment with the literal value of an enum is pretty useless, either the value is explicit by the protocl, then use toxav/rtp.h, line 41 at r2 (raw file):
same toxav/rtp.h, line 82 at r2 (raw file):
I think we should use toxav/toxav.c, line 41 at r2 (raw file):
IMO this should be higher by default because a normal desktop can handle probably more than that. toxav/toxav.c, line 799 at r2 (raw file):
remove useles toxav/toxav.c, line 876 at r2 (raw file):
use toxav/toxav.c, line 879 at r2 (raw file):
use toxav/toxav.c, line 904 at r2 (raw file):
remove empty else toxav/video.c, line 138 at r2 (raw file):
still needed? toxav/video.c, line 147 at r2 (raw file):
why disabled? toxav/video.c, line 245 at r2 (raw file): Waaay to much whitespace here toxav/video.c, line 274 at r2 (raw file): same toxav/video.c, line 729 at r2 (raw file):
???? toxcore/LAN_discovery.c, line 359 at r2 (raw file):
should probably not be part of this PR toxcore/logger.c, line 70 at r2 (raw file):
same toxcore/logger.h, line 34 at r2 (raw file):
same Comments from Reviewable |
I did not test this PR yet |
Review status: 5 of 12 files reviewed at latest revision, 48 unresolved discussions, some commit checks failed. toxav/bwcontroller.c, line 92 at r6 (raw file): Previously, zoff99 (Zoff) wrote…
*sigh* reluctantly OK. toxav/video.c, line 41 at r6 (raw file): Previously, zoff99 (Zoff) wrote…
OK toxav/video.c, line 459 at r6 (raw file): Previously, zoff99 (Zoff) wrote…
ok Comments from Reviewable |
Reviewed 5 of 7 files at r8, 2 of 2 files at r9. Comments from Reviewable |
toxav/rtp.c, line 343 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
I don't know Comments from Reviewable |
f606e7f
to
c91a421
Compare
c91a421
to
a6a1770
Compare
Review status: all files reviewed at latest revision, 35 unresolved discussions. circle.yml, line 124 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
probably easier to merge this in a separate PR then. toxav/bwcontroller.c, line 35 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
then please add a comment like toxav/bwcontroller.c, line 92 at r6 (raw file): Previously, zoff99 (Zoff) wrote…
@zoff99 toxav/ring_buffer.c, line 28 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
The usual way to solve this problem is, to put the ring buffer in a struct with a description field. This way the ring buffer api can stay generic and you still have only one extra byte. toxav/ring_buffer.c, line 98 at r8 (raw file):
extra spaces toxav/rtp.c, line 165 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
I think the logic in the whole block can be simplified like this:
which is the standard way to check for set bits in C toxav/rtp.c, line 169 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
Where do you get the value toxav/rtp.c, line 594 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
maybe explain why these functions are listed here? toxav/rtp.c, line 620 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
If you can't figure out why, add a todo or fixme comment. toxav/rtp.c, line 362 at r8 (raw file):
is toxav/rtp.h, line 82 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
What's the TODO: about? Remove the commented out piece of code toxav/toxav.c, line 876 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
inverted the condition to toxav/toxav.c, line 887 at r10 (raw file):
Is it important/correct to unlock the mutex before the logger call and setting toxav/video.c, line 138 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
Ok toxav/video.c, line 147 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
Ok toxav/audio.h, line 32 at r2 (raw file): Previously, zoff99 (Zoff) wrote…
I still see no comment? Comments from Reviewable |
7263d33
to
b438010
Compare
Reviewed 1 of 2 files at r5, 2 of 9 files at r6, 2 of 7 files at r7, 4 of 7 files at r8, 1 of 2 files at r9, 3 of 3 files at r11, 1 of 3 files at r12. testing/av_test.c, line 210 at r12 (raw file):
rb_write can retrun NULL toxav/rtp.c, line 169 at r2 (raw file): Previously, sudden6 wrote…
Create a constant for toxav/rtp.c, line 281 at r11 (raw file):
Make sure memory is allocated for toxav/rtp.c, line 303 at r11 (raw file):
Add a check to make sure toxav/rtp.c, line 496 at r11 (raw file):
Make sure memory is allocated. toxav/rtp.c, line 560 at r11 (raw file):
Possible double free, toxav/rtp.c, line 621 at r11 (raw file):
Possible double free, toxav/video.c, line 536 at r11 (raw file):
rb_write can return NULL. toxav/video.c, line 538 at r11 (raw file):
rb_write can return NULL. Comments from Reviewable |
Reviewed 2 of 3 files at r12. Comments from Reviewable |
b438010
to
d9a0148
Compare
Extracted from TokTok#629.
6b63bff
to
53250c0
Compare
Review status: 7 of 10 files reviewed at latest revision, 51 unresolved discussions. toxav/bwcontroller.c, line 36 at r11 (raw file):
BWC_REFRESH_INTERVAL_MS is no longer used; delete? toxav/bwcontroller.c, line 37 at r12 (raw file):
BWC_AVG_PKT_COUNT is no longer used, nor is rcvpkt. Delete them? toxav/bwcontroller.c, line 166 at r12 (raw file):
Should be "Peer"? toxav/rtp.c, line 64 at r13 (raw file):
Already NULL, from calloc toxav/rtp.c, line 148 at r13 (raw file):
This is a pretty nasty hack! Generally, keeping track of whether a frame is a keyframe seems to be leading to a lot of unpleasantness. Is it really necessary? Could it make sense to just treat unusually large frames specially, rather than keyframes in particular? toxav/rtp.c, line 210 at r13 (raw file):
Will deleting this break bwc for v2 packets? toxav/rtp.h, line 88 at r12 (raw file):
This makes no sense in terms of the RTP standard. We could stay within the RTP spec by putting the extra information after the csrcs in a "header extension". I don't know if there's actually any value to sticking to the RTP spec, but it seems perverse to do so partially but not fully. toxav/rtp.h, line 109 at r12 (raw file):
Again, these 3 should be in csrc according to the RTP spec. Comments from Reviewable |
Closing this in favour of the WIP patch series I'm building. We'll work on those branches together and I'll manage the git rebases. |
This change is