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 video payload into multiple RTP messages when too big to fit into one #623

Merged
merged 1 commit into from Dec 17, 2017

Conversation

@mannol
Copy link

@mannol mannol commented Nov 25, 2017

Fixes #620

This is the implementation of the proposed fix for this issue.

Haven't tried it myself yet but, it's simple enough to work. NOTE: this fix does not break the protocol.


This change is Reviewable

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Nov 25, 2017

CLA assistant check
All committers have signed the CLA.

@mannol mannol requested a review from zoff99 Nov 25, 2017
@nurupo
Copy link
Member

@nurupo nurupo commented Nov 25, 2017

I'd use UINT16_MAX constant instead of 65000.

What about the audio frame, will it always fit?

@mannol mannol force-pushed the mannol-patch-1 branch from 71f5224 to 4df8a69 Nov 25, 2017
@mannol
Copy link
Author

@mannol mannol commented Nov 25, 2017

@nurupo ok, changed to UINT16_MAX. IIRC, yes, audio frames should always fit.

@nurupo
Copy link
Member

@nurupo nurupo commented Nov 25, 2017

Did you make sure UINT16_MAX is not treated as a special value by anything?

@mannol
Copy link
Author

@mannol mannol commented Nov 25, 2017

Yup.

@nurupo
Copy link
Member

@nurupo nurupo commented Nov 25, 2017

I'm building Windows qTox v1.12.1 with your patch for anyone wanting to test this, will upload once they are done. You can build it yourself by following this and changing --branch v0.1.10 \ to --branch mannol-patch-1 \ in build.sh file.

@mannol
Copy link
Author

@mannol mannol commented Nov 25, 2017

Great! I hope it works!

@mannol mannol force-pushed the mannol-patch-1 branch from 4df8a69 to 8fe44ae Nov 25, 2017
@nurupo
Copy link
Member

@nurupo nurupo commented Nov 25, 2017

Here are the qTox Windows builds for testing

(Apparently GitHub doesn't allow uploading 7z archives)

@zoff99 zoff99 removed their request for review Nov 25, 2017
@zoff99 zoff99 removed their assignment Nov 25, 2017
@zoff99
Copy link

@zoff99 zoff99 commented Nov 25, 2017

its written in a most complex way. hard for me to understand

@robinlinden robinlinden force-pushed the mannol-patch-1 branch from 8fe44ae to 56dcbf1 Nov 25, 2017
@zoff99
Copy link

@zoff99 zoff99 commented Nov 25, 2017

this may not work because of:

session->sequnum ++;

also this complex solution has the risk of intruducing new problems not yet found.

@mannol
Copy link
Author

@mannol mannol commented Nov 25, 2017

No, that's how standard RTP works, increasing sequnum to monitor packet order. Also, it's super simple: split large payload into smaller chunks. The decoder then takes those chunks and creates an image out of them.

@zoff99
Copy link

@zoff99 zoff99 commented Nov 25, 2017

its hard to say. it opens up for many new, not found bug.
sequence number is used all over the place

if (session->mp->header.sequnum == net_ntohs(header->sequnum) &&

@mannol
Copy link
Author

@mannol mannol commented Nov 25, 2017

You do realize that sequence number has nothing to do with the issue in the first place? You do realize that sequence number is used for ordering packets and is defined by the standard to be 16-bit value? Why are you talking nonsense about this proposed fix that's infinitely better (does not all of the existing clients) than your solution?

@sudden6
Copy link

@sudden6 sudden6 commented Nov 25, 2017

Did not test, but code :lgtm_strong:


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


Comments from Reviewable

@Diadlo
Copy link

@Diadlo Diadlo commented Nov 25, 2017

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@nurupo
Copy link
Member

@nurupo nurupo commented Nov 25, 2017

Would be nice if someone could test it. If someone who has video issues in the unpatched qTox 1.12.1 (the latest qTox stable release) could say if it works better in the patched qTox.

@Diadlo
Copy link

@Diadlo Diadlo commented Nov 25, 2017

If someone will have time tomorrow at ~20:00 - 22:00 UTC+0, we can test this patch over network (ToxId in GitHub bio)

@zugz
Copy link

@zugz zugz commented Nov 26, 2017

At zoff99's request, I just tested this branch and found that it's broken in
the same way that master is. Testing with some non-backwards-compatible by
zoff99, meanwhile, it worked perfectly.

I tested with https://github.com/zoff99/ToxCam/ branch '_video_test_01' to
send the video, using the test slides
https://github.com/zoff99/ToxCam/blob/_video_test_01_/toxcam/frame_1.yuv
https://github.com/zoff99/ToxCam/blob/_video_test_01_/toxcam/frame_2.yuv
, and using utox running locally to receive the video. With master and with
this PR, I get nothing at all. With zoff99's branch
'_0.1.10_2017_runtime_val_changes' on https://github.com/zoff99/c-toxcore/ ,
I receive the slides correctly.

I haven't really looked into this issue and I don't know what to recommend,
I'm just reporting test findings.

@nurupo
Copy link
Member

@nurupo nurupo commented Nov 26, 2017

Glad that someone tested it. Sounds like this PR doesn't fix things then, more debugging/investigation is needed.

@sudden6
Copy link

@sudden6 sudden6 commented Nov 26, 2017

@zugz Did you use the patched c-toxcore from this PR only as receiver? Because this fix can only work at the sender side.

@zugz
Copy link

@zugz zugz commented Nov 26, 2017

@mannol
Copy link
Author

@mannol mannol commented Nov 26, 2017

Trying it out myself. I also used toxcam branch '_video_test_01' compiled with this patch to send the video. And this is the result: https://streamable.com/215y8

@zugz Is that the expected result or?

NOTE: In the video, there are some frames lost due to the screen recorder.

@zugz
Copy link

@zugz zugz commented Nov 26, 2017

@mannol mannol changed the title Proposed fix for #620 Fix for #620 Nov 26, 2017
@robinlinden
Copy link
Member

@robinlinden robinlinden commented Nov 27, 2017

:lgtm_strong:


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


Comments from Reviewable

@zoff99
Copy link

@zoff99 zoff99 commented Nov 28, 2017

@mannol test again with _video_test_01_ but this time copy "frame_3.yuv" over both frame_1 and frame_2 so that it will only use frame_3.
also what CPU does your sending system have? test outcome largely depend on that. it needs to be powerful for the test to show the bug.

also dont use qTox for the test, the received video window is to small to see if its crunched down or really full res

@SkyzohKey SkyzohKey added this to the v0.1.11 milestone Nov 28, 2017
@mannol mannol removed the api break label Dec 1, 2017
@mannol
Copy link
Author

@mannol mannol commented Dec 1, 2017

@nurupo network test is failing. What do?

@nurupo
Copy link
Member

@nurupo nurupo commented Dec 1, 2017

No idea, you are asking the wrong person, I haven't been following toxcore development for at least a month or two now and back then everything was passing.

@nurupo
Copy link
Member

@nurupo nurupo commented Dec 1, 2017

There were some network refactors in toxcore recently, could be something to do with that, or could also be some issue on Travis's side. Ideally someone would debug why it's failing and fix it. I have also noticed it failing in the zoff's PR and have bugged @robinlinden about it since he has been keeping an eye on toxcore and merging PRs, but it seems like he doesn't know either. Out if curiosity, does the test fail locally for you?

@zoff99
Copy link

@zoff99 zoff99 commented Dec 1, 2017

@mannol @nurupo let me just check that quick ...

@nurupo
Copy link
Member

@nurupo nurupo commented Dec 1, 2017

The network test passes locally for me, using the same flags that the clang travis does but with turned off format test:
cmake -DASAN=ON -DDEBUG=ON -DSTRICT_ABI=ON -DTEST_TIMEOUT_SECONDS=120 -DTRACE=ON -DUSE_IPV6=no -DERROR_ON_WARNING=ON -DBUILD_NTOX=ON -DFORMAT_TEST=OFF ..

@zoff99
Copy link

@zoff99 zoff99 commented Dec 1, 2017

i changed the settings in circleCI

image

somebody set it so that not all commits were built. so you dont know which commit introduced the bug.

@Diadlo
Copy link

@Diadlo Diadlo commented Dec 1, 2017

@zoff99 Probably, bug introduced by Travis

@zoff99
Copy link

@zoff99 zoff99 commented Dec 1, 2017

its working now in circleCI. please don't change circleCI settings without first checking.

@zoff99
Copy link

@zoff99 zoff99 commented Dec 8, 2017

@robinlinden @Diadlo @sudden6 wow 3 times LGTM without 1 single test.
i actually tested it myself and with zugz and it does not work

@zoff99
Copy link

@zoff99 zoff99 commented Dec 8, 2017

@mannol
Copy link
Author

@mannol mannol commented Dec 8, 2017

@zoff99 I presented an example of unpatched and patched versions on which there is a clear evidence of quality improvement.
You said:

also what CPU does your sending system have? test outcome largely depend on that. it needs to be powerful for the test to show the bug.

also dont use qTox for the test, the received video window is to small to see if its crunched down or really full res

And I'll say it again: there is a clear difference between unpatched and patched versions; the unpatched version is so bad that you can't see anything while the patched version is ok.

@sudden6
Copy link

@sudden6 sudden6 commented Dec 9, 2017

I got around to testing this now too.

Setup

  • PC1, qTox tag v1.13.0, c-toxcore master
  • PC2, qTox qTox/qTox@9ac5f88, c-toxcore master, later switched to this PR

Both PCs are connected over WLAN. The WLAN has a few percent packet loss, so I think this is a good simulation of a longer distance internet connection.

Test1

PC2 shares the screen (2960 x 1050) to PC1, PC2 is on c-toxcore master

  • No picture ever from PC2 on PC1, webcam (320p) displays fine on PC2

Test2

PC2 shares the screen (2960 x 1050) to PC1, PC2 is on c-toxcore with this PR

  • desktop is shared to PC1, webcam displays fine on PC2
  • sometimes the lower part of the desktop is grainy/not sharp on PC1
  • rarely the desktop is displayed as pink on PC1 for a few seconds

Test3

PC2 shares the screen (2960 x 1050) to PC1, PC2 is on c-toxcore with #622

  • desktop is shared to PC1, webcam displays fine on PC2
  • only the top 25% of the desktop are displayed correctly on PC1, the rest is always corrupted.

Conclusion

This PR gives better results than #622, but does not completely fix the problem. I think this PR should still be merged since it improves the situation without breaking the protocol and the complete fix most likely needs this anyway.

@robinlinden
Copy link
Member

@robinlinden robinlinden commented Dec 9, 2017

@zoff99 I tested this PR before LGTMing it and it does work better than the current video sending code.

@robinlinden robinlinden force-pushed the mannol-patch-1 branch 2 times, most recently from ade444b to b15d809 Dec 14, 2017
@robinlinden robinlinden changed the title Fix for #620 Split video payload into multiple RTP messages when too big to fit into one Dec 17, 2017
This is the implementation of the [proposed fix](#620 (comment)) for [this issue](#620).
@robinlinden robinlinden force-pushed the mannol-patch-1 branch from b15d809 to e996a03 Dec 17, 2017
@robinlinden robinlinden merged commit e996a03 into master Dec 17, 2017
5 checks passed
5 checks passed
ci/circleci Your tests passed on CircleCI!
Details
code-review/reviewable 3/1 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@robinlinden robinlinden deleted the mannol-patch-1 branch Dec 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants
You can’t perform that action at this time.