-
Notifications
You must be signed in to change notification settings - Fork 181
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
FURY Streaming System Proposal #437
FURY Streaming System Proposal #437
Conversation
related with fury-gl#435
Hello @devmessias! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-04-13 14:53:03 UTC |
Co-authored-by: devmessias <messias.physics@gmail.com> Co-authored-by: filipinascimento <filipinascimento@gmail.com>
Doing that way because the FuryStreamClient will have a lot of more behaviors, like user interaction.
This can be used for example to store/read user interactions
This uses a circular queue as proposed by @filipinascimento
It seems that vtk uses left-bottom coordinates
macOS dosen't use fork to achieve multiprocessing. Therefore, this create some issues for specific python versions. The solution it's to pass just the buffers to the childreen process
For example, using ngrok ./ngrok http --subdomain=something 8000
Fix zoom factor for throttling
iframe mode, background color and interaction can be defined through url parameters Example: http://localhost:8000/?interaction=1&iframe=0&background=black
…uffers This allows you to start a webrtc server in the main process
This will avoid the first rendering to be just a blank screen inside the browser
cff02af
to
eb85624
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first round of my reviews. More asap.
Linux | ||
|
||
|
||
`apt install libavdevice-dev libavfilter-dev libopus-dev libvpx-dev pkg-config` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see ffmpeg here. Is that okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I forgot. I'll address that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you address it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that for mac, ffmpeg, opus or libvpx are not needed. I've just pipinstall aiortc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that for mac, ffmpeg, opus or libvpx are not needed. I've just pipinstall aiortc
None, | ||
stream.img_manager.info_buffer, | ||
None, | ||
stream_interaction.circular_queue.head_tail_buffer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The args can be simplified very much by just providing as input stream
and stream_interaction
. Then you can grab the other properties from the inside your function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work on MacOs. Unfortunately, MacOs seems to have some problems sending a pickle to a child process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, create two classes one for shared memory and one for raw array.
self.ms_jpeg = ms_jpeg | ||
self._host = host | ||
if port is None: | ||
port = np.random.randint(7000, 8888) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so. @filipinascimento do you have any considerations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This do not check for existing open ports, it is safe, but can fail if the selected port is already open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can check this. But this operation takes some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @devmessias,
See below for some comments, I still need to spend time on fury.stream.tools
to improve readability
To improve the code readability and design
e1390da
to
051d42f
Compare
fury/stream/tools.py
Outdated
def fix_register(name, rtype): | ||
if rtype == "shared_memory": | ||
return | ||
return resource_tracker._resource_tracker.register( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self
is undefined here. Results in a silent error when the server is running in another process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how we can do monkey patching on python. This is an open bug in CPython implementation regarding shared memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alto this is a silent ERROR or silent Warning? What is the msg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this results in a warning caused by an unraisable exception, which I'm not sure if it is unraisable because it is running during garbage collector, destroy or because it runs in another process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you can do is call ls -ls /dev/shm
and see if some resource was not properly released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've tested this, and the patch works on mac. The problem is that, for some reason, pytest checks this self and gives an NameError. On macs, /dev/shm does not exist, but I've checked for memory usage and everything seems to work nicely on Python 3.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a NameError try catch, as you suggested, gets rid of the warning for when running pytest. Memory is reclaimed. so everything should be fine. The problem does not happen on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done at 25b3979
Maybe is good to describe this issue on cypthon repo. python/cpython#82300
Also, do not forget to address all the pep8 listed above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the PR is ready (may need a pass from Serge and Elef to make sure everything is ok with coding style). We can improve the documentation incrementally later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @devmessias Please see comments and also address pep 8 issues. Keep pushing this is getting super close to be merged! FURY streaming is on the way! Go go team!
It dosen't provides real improvements
python bug related to python/cpython#82300
4a8f49f
to
25b3979
Compare
b58807f
to
3fbaf24
Compare
Congratulations @devmessias This important PR is finally merged. Great job! |
Great news! Thanks @Garyfallidis , @skoudoro and @filipinascimento |
Next step @devmessias is the creation of furyspeed package that will accelerate streaming speed @skoudoro |
related with #435