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

[WIP]: Add dev mode for faster rendering #323

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

codejaeger
Copy link
Contributor

PR Checklist

If you are contributing to Javis.jl, please make sure you are able to check off each item on this list:

  • Did I update CHANGELOG.md with whatever changes/features I added with this PR?
  • Did I make sure to only change the part of the file where I introduced a new change/feature?
  • Did I cover all corner cases to be close to 100% test coverage (if applicable)?
  • Did I properly add Javis dependencies to the Project.toml + set an upper bound of the dependency (if applicable)?
  • Did I properly add test dependencies to the test directory (if applicable)?
  • Did I check relevant tutorials that may be affected by changes in this PR?
  • Did I clearly articulate why this PR was made the way it was and how it was made?

Link to relevant issue(s)
Attempt to close #166

How did you address these issues with this PR? What methods did you use?

The idea is to automatically track the updated frames in case the dev_mode is specified while creating the videos. This definitely has its limitations and cannot always track every unchanged frame i.e. if the image of the frame is left unchanged but somehow that frame was referred to or used since last render it will still get created in the next render.

Currently, tests need to be added to benchmark the improvements (if any). @Wikunia @TheCedarPrince could you give a initial review if possible?
Thanks!

@Wikunia
Copy link
Member

Wikunia commented Mar 26, 2021

Will have a look later this weekend. Probably Sunday. Do you mind giving some code to go into dev mode and a bit more explanation to what should happen?
Thanks in advance m testing with morphing might fail based on your explanation so far.

@codejaeger
Copy link
Contributor Author

codejaeger commented Mar 26, 2021

Yes sure @Wikunia.

using Javis

function ground(args...)
    background("white") # canvas background
    sethue("black") # pen color
end

myvideo = Video(500, 500; dev_mode=true)
Background(1:70, ground)
red_ball = Object(1:70, (args...)->object(O, "red"), Point(100,0))
render(myvideo; pathname="tutorial_1.gif")
# To skip re-drawing frames use the compute_frames argument
render(myvideo; pathname="tutorial_1.gif", compute_frames=:latest)

This is the way one should use it. I am still on the benchmarking part, trying to see what is the outcome. I did not find a test so far where not re-drawing the frames had any significant speed up. I will try to find one and add it as a test in a subsequent commit as test. Thanks.

@codejaeger codejaeger force-pushed the codejaeger-feature-animationdevmode branch from b933f60 to 52ca40a Compare March 26, 2021 17:55
@TheCedarPrince
Copy link
Member

Sure @codejaeger - I could probably do a cursory review today to give some feedback. 😄

@TheCedarPrince TheCedarPrince self-requested a review March 26, 2021 17:57
@codejaeger codejaeger changed the title [WIP]: Add dev mode for faster animation [WIP]: Add dev mode for faster rendering Mar 26, 2021
@Wikunia
Copy link
Member

Wikunia commented Mar 26, 2021

Thanks! What's the :latest part? Can one specify the frames one want to recompute? And does this work for gif and mp4?

@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #323 (52ca40a) into master (af4f664) will decrease coverage by 0.47%.
The diff coverage is 73.91%.

❗ Current head 52ca40a differs from pull request most recent head d05d310. Consider uploading reports for the commit d05d310 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
- Coverage   96.31%   95.84%   -0.48%     
==========================================
  Files          21       21              
  Lines        1113     1130      +17     
==========================================
+ Hits         1072     1083      +11     
- Misses         41       47       +6     
Impacted Files Coverage Δ
src/Javis.jl 94.61% <63.63%> (-2.93%) ⬇️
src/structs/Video.jl 87.50% <77.77%> (-12.50%) ⬇️
src/structs/Object.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af4f664...d05d310. Read the comment docs.

@codejaeger
Copy link
Contributor Author

codejaeger commented Mar 26, 2021

:latest is to signify to re-compute/draw only the frames which are thought to be different. If not specified, it should work just like it was doing before. I tried it with gif, I will do the same with mp4 and let you know. No, currently there is no way to take in the frame range to compute as input. I will add that after this. This seemed to me a simpler way to provide a speed up.

I also mentioned a reason why specifying the frame range could be a difficult option in this comment.

@Sov-trotter
Copy link
Member

Sov-trotter commented Mar 28, 2021

@codejaeger, Sorry for poking my nose in here, but I totally understand the Image.load() latency issue here as I had faced something similar while trying to reduce the lag in Jupyter Viewer. One approach you could try is replace frame_image = Images.load("$(tempdirectory)/$(lpad(filecounter, 10, "0")).png") with something "in-memory" eg, fetch that particular frame from an array of frames(stored previously). Again that might lead to increased allocations but it's worth giving a try.

@Wikunia
Copy link
Member

Wikunia commented Mar 28, 2021

Lol we had the same thought @Sov-trotter just in a different issue #166

@codejaeger
Copy link
Contributor Author

codejaeger commented Mar 28, 2021

Will try that @Sov-trotter, thanks for the suggestion 😃

@codejaeger codejaeger force-pushed the codejaeger-feature-animationdevmode branch from ea3b1ec to d05d310 Compare April 1, 2021 11:28
@codejaeger
Copy link
Contributor Author

@Wikunia I tried the approach of storing the frames in memory. In a crude way, there is a performance speed up of 25% when I try with examples generating 300 and 500 frames. I will add tests for these. I wanted to ask does this speed up seem reasonable against the increased memory consumption?

@Wikunia
Copy link
Member

Wikunia commented May 22, 2021

I think it makes sense to provide the option with bigger memory consumption but for faster rerendering.

@Wikunia Wikunia changed the base branch from master to main February 25, 2022 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Javis Animation Dev Mode
4 participants