-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
[GSoC'21] Livestreaming v2 #387
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #387 +/- ##
==========================================
+ Coverage 96.25% 96.26% +0.01%
==========================================
Files 35 35
Lines 1521 1528 +7
==========================================
+ Hits 1464 1471 +7
Misses 57 57
Continue to review full report at Codecov.
|
@TheCedarPrince @Wikunia I think this is ready for review |
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.
To be honest I expected something quite different with #345
Currently this does still precomputes all image frames before starting to stream.
Live streaming should mean that it starts streaming as soon as the first frame got computed.
@@ -1,6 +1,8 @@ | |||
# Javis.jl - Changelog | |||
|
|||
## Unreleased | |||
- Make livestreaming faster |
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 do you mean by faster? Is it live? 😉
src/javis_viewer.jl
Outdated
stream_filecounter = 1 | ||
for frame in stream_frames | ||
frame_image = convert.(RGB, get_javis_frame(video, objects, frame; layers = layers)) | ||
if !isempty(tempdirectory) |
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 happens if tempdirectory is empty? It just generates the frames but can't use 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.
No tempdir is created before if it's empty (as before)
https://github.com/Wikunia/Javis.jl/pull/387/files/56156371a5657544c632a3cb6faa2b39681e1fe7#diff-763ffddd0fe1ffb533564007035f5947812905676655f155ce1ee4357eb53c5eR275
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.
Yeah what I mean is: it should never be empty so we don't need the if. The problem is if we have an mp4 as the pathname without a temp directory but want to stream it will fail, right?
I'm unsure what broke when merging it with master. Do you have an idea @Sov-trotter ? Feel free to revert the changes I made. |
Yeah. I understand. Just taking small steps here. Will get to that too. :) |
I think we cna talk about how to do this in a cleaner fashion. Because subsequent frames are generated in a serial fashion in a for loop. I wonder how to have ffmpeg read that. We can obviously overwrite one image again and again but that might cause a race condition. https://newbedev.com/can-you-stream-images-to-ffmpeg-to-construct-a-video-instead-of-saving-them-to-disk might be of help |
Hey @Wikunia @TheCedarPrince after some more experimentation here's what I found:
What we can do is remove the images from temp directory when |
PR Checklist
If you are contributing to
Javis.jl
, please make sure you are able to check off each item on this list:CHANGELOG.md
with whatever changes/features I added with this PR?Project.toml
+ set an upper bound of the dependency (if applicable)?test
directory (if applicable)?Closes #345
As discussed this makes livestreaming more live, this time by streaming the frames rather than a gif.
Also allows to stream specific frames by passing in a
UnitRange