jeertmans / manim-slides

Tool for live presentations using manim
https://manim-slides.eertmans.be
MIT License
489 stars 49 forks source link

[BUG] `utils.reverse_video_file` causes excessive RAM usage #434

Open jungerm2 opened 6 months ago

jungerm2 commented 6 months ago

Description

EDIT: This issue actually comes from reverse_video_file. See below.

I'm aware this is likely an av bug, but I'm filling it here because others might encounter it.

It seems manim-slides' concatenate_video_files eats up too much RAM (it crashes my 16GB laptop) and can cause severe OS crashes. I've tried to fix this bug on my end by monkey-patching it to use manimCE's equivalent of concatenate_video_files like so:

import manim_slides
from manim_slides.logger import logger

# Yet another bad idea... but it works **shrug**
def _concatenate_video_files(files: list[Path], dest: Path) -> None:
    logger.warn("Warning: Using monkey-patched `concatenate_video_files`")
    renderer = CairoRenderer()
    writer = SceneFileWriter(renderer, "NotARealScene")
    writer.combine_files(files, dest)
manim_slides.slide.base.concatenate_video_files = _concatenate_video_files

The above works fine most of the time (which raises the question: why does manim-slides have its own version of combine files then?), but also causes OOMs just like the original version.

A long-term fix would be great, but I understand this might be out of scope. In the meanwhile, I'm trying to render out my slides in HQ because I've a presentation coming up in a few days, and, while they render fine with -ql (which I was using to prototype the slides), I can't seem to render them in HQ. Any temporary fixes I could do?

Version

Latest at time of writing (manim-slides, version 5.1.7)

Platform

Linux, Fedora 40

Screenshots

No response

Additional information

No response

jeertmans commented 6 months ago

Hello @jungerm2, thank you for reporting this bug!

I highly suspect that your bug is caused by a previous fix I deployed to avoid concatenating empty video files. This was needed prior to Manim v18.1 (fixed by https://github.com/ManimCommunity/manim/pull/3491) because Manim could produce empty video files.

This fix is in def _filter:

https://github.com/jeertmans/manim-slides/blob/b08073983b8ac8b871c34c2a422c150e097e3be1/manim_slides/utils.py#L12-L62

Can you try monkey-patching this function by replacing the _filter function with:

_filter = lambda files: files

I suspect that the context manager does not correctly close the files, and this might explain why we have an increase in memory.

jungerm2 commented 6 months ago

I thought this might have been an ffmpeg bug at first, so I updated from 5.11 to latest and the problem persists. There's some similar bugs that have been reported in the concat muxer, but I don't know if they are related, i.e: https://trac.ffmpeg.org/ticket/10550

I'll try your above suggestion, but since I've monkey-patched concatenate_video_files to use manim's version, and it still doesn't work, I do not think this is the issue. I'm starting to think it's the reverse_video_file function that causes issues and that I had misdiagnosed this. FFmpeg is notorious for large memory consumption during reversal: https://trac.ffmpeg.org/ticket/9073

Thanks for the quick reply and I'll keep you updated.

jungerm2 commented 6 months ago

It's absolutely the reverse_video_file function! Sorry for getting it wrong. I monkey patched the following (which just skips the reversal step):

manim_slides.slide.base.reverse_video_file = shutil.copy2

And the whole rendering process uses minimal RAM now.

jeertmans commented 6 months ago

Ok then reversing indeed requires to load the whole video into memory, which then can be an issue for large videos and I am afraid there is not real solutions. Some people have suggested to reverse a video by splitting it into parts and then concatenate them.

Can you share a minimal example when memory is an issue?

jungerm2 commented 6 months ago

Seems like the recommended way to reverse large videos is to split it into chunks, reverse those and concat back (here, and here). This should be pretty straightforward to implement -- we're only missing the split part.

To avoid dealing with ffmpeg, or pyAV, I used moviepy to reverse the video (as I have a deadline coming up!) as it reverses the video by extracting one frame at a time thus not loading everything into memory. It is also much slower because of this:

def _reverse_video_file(src: Path, dest: Path) -> None:
    from moviepy.editor import VideoFileClip, vfx
    clip = VideoFileClip(str(src)).fx(vfx.time_mirror)
    clip.write_videofile(str(dest))

if literal_eval(os.environ.get("SKIP_REVERSE", "False")):
    logger.warn("Warning: Skipping video reversal because the `SKIP_REVERSE` environment variable was set.")
    manim_slides.slide.base.reverse_video_file = shutil.copy2
else:
    manim_slides.slide.base.reverse_video_file = _reverse_video_file

And finally, this works as intended!

For reference, the video I'm reversing is only about 5mb when encoded, but it's 4k60, and is relatively simple so it compresses very well, so I suspect its full decoded size is very large. Reversing it with the above takes ~1 hour though...

This is not a viable solution long-term, and a more efficient reversal method will be needed. But for now, it means I can do my presentation.

I think adding a cli flag to skip reverse video generation might be a good idea too. It's not a feature I've ever used (the V in the GUI), and it's likely not needed during prototyping, although maybe it complicates other things such as exports...

jungerm2 commented 6 months ago

Looks like you answered while I was writing a reply...

Here's a minimal example that breaks on my system:

On my system, the 720p version is more than enough to cause havoc.

jeertmans commented 6 months ago

But do you have a MWE that is self-contained, where files are generated with Manim?

jungerm2 commented 6 months ago

Sure, here's a simple one (which you might recongnize):

from manim import *  # or: from manimlib import *

from manim_slides import Slide

class BasicExample(Slide):
    def construct(self):
        circle = Circle(radius=3, color=BLUE)
        dot = Dot()

        self.play(GrowFromCenter(circle))
        self.next_slide()  # Waits user to press continue to go to the next slide

        self.next_slide(loop=True)  # Start loop
        for _ in range(100):
            self.play(MoveAlongPath(dot, circle), run_time=2, rate_func=linear)
        self.next_slide()  # This will start a new non-looping slide

        self.play(dot.animate.move_to(ORIGIN))

Rendering with default settings (i.e: manim example.py) uses huge amounts of memory.

jungerm2 commented 6 months ago

The above can probably be addressed easily by reversing individual animations (or partial-movie-files) and then concatenating them together, this could be done in this loop: https://github.com/jeertmans/manim-slides/blob/b08073983b8ac8b871c34c2a422c150e097e3be1/manim_slides/slide/base.py#L476-L495

But here's another MWE that causes issues on my machine, when running with -qh or equivalently if the runtime is longer, which would not be fixed by the above:

from manim import *  # or: from manimlib import *
from manim_slides import Slide

class BasicExample(Slide):
    def construct(self):
        circle = Circle(radius=3, color=BLUE)
        dot = Dot()

        self.play(GrowFromCenter(circle))
        self.next_slide()  # Waits user to press continue to go to the next slide

        self.next_slide(loop=True)  # Start loop
        self.play(MoveAlongPath(dot, circle), run_time=30, rate_func=linear)
        self.next_slide()  # This will start a new non-looping slide

        self.play(dot.animate.move_to(ORIGIN))

I don't think it's unrealistic to render 30 second animations in high quality.

I see two solutions:

jeertmans commented 6 months ago

I see, this is a non-trivial problem because it happens for both: many animations and one very long animations. I actually had the same issue before in presentations.

Regarding your 2nd solution, I am afraid that submitting a PR to Manim is not a good solution, because they do even use reversed animations, so why would they accept that?

I could "easily" implement a mix of both "reversing individual partial movies files and them concatenating the reversed list", and, in reverse_video_file, split the video if it is too large and concatenate it back. I could even leverage parallel threads for that. The only question I still have is "when" to split (what criterion?) and "how" (I know there is a split filter, but I have never used it).

jeertmans commented 6 months ago

See #439