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

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

Open
jungerm2 opened this issue May 20, 2024 · 11 comments · May be fixed by #439
Open

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

jungerm2 opened this issue May 20, 2024 · 11 comments · May be fixed by #439
Labels
bug Something isn't working lib Related to the library (a.k.a. module)
Milestone

Comments

@jungerm2
Copy link

jungerm2 commented May 20, 2024

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

@jungerm2 jungerm2 added the bug Something isn't working label May 20, 2024
@jeertmans
Copy link
Owner

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 ManimCommunity/manim#3491) because Manim could produce empty video files.

This fix is in def _filter:

def concatenate_video_files(files: list[Path], dest: Path) -> None:
"""Concatenate multiple video files into one."""
def _filter(files: list[Path]) -> Iterator[Path]:
"""Patch possibly empty video files."""
for file in files:
with av.open(str(file)) as container:
if len(container.streams.video) > 0:
yield file
else:
logger.warn(
f"Skipping video file {file} because it does "
"not contain any video stream. "
"This is probably caused by Manim, see: "
"https://github.com/jeertmans/manim-slides/issues/390."
)
with tempfile.NamedTemporaryFile(mode="w", suffix=".txt", delete=False) as f:
f.writelines(f"file '{file}'\n" for file in _filter(files))
tmp_file = f.name
with (
av.open(tmp_file, format="concat", options={"safe": "0"}) as input_container,
av.open(str(dest), mode="w") as output_container,
):
input_video_stream = input_container.streams.video[0]
output_video_stream = output_container.add_stream(
template=input_video_stream,
)
if len(input_container.streams.audio) > 0:
input_audio_stream = input_container.streams.audio[0]
output_audio_stream = output_container.add_stream(
template=input_audio_stream,
)
for packet in input_container.demux():
if packet.dts is None:
continue
ptype = packet.stream.type
if ptype == "video":
packet.stream = output_video_stream
elif ptype == "audio":
packet.stream = output_audio_stream
else:
continue # We don't support subtitles
output_container.mux(packet)
os.unlink(tmp_file) # https://stackoverflow.com/a/54768241

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.

@jeertmans jeertmans added the lib Related to the library (a.k.a. module) label May 21, 2024
@jeertmans jeertmans added this to the v6 milestone May 21, 2024
@jungerm2
Copy link
Author

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
Copy link
Author

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.

@jungerm2 jungerm2 changed the title [BUG] utils.concatenate_video_files seems to load all videos into memory before concatenating [BUG] utils.reverse_video_file causes excessive RAM usage May 21, 2024
@jeertmans
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

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

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

from manim_slides.utils import reverse_video_file
reverse_video_file("big_buck_bunny_720p_h264.mov", "assets/big_buck_bunny_720p_h264_reversed.mov")

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

@jeertmans
Copy link
Owner

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

@jungerm2
Copy link
Author

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
Copy link
Author

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:

for pre_slide_config in tqdm(
self._slides,
desc=f"Concatenating animation files to '{scene_files_folder}' and generating reversed animations",
leave=self._leave_progress_bar,
ascii=True if platform.system() == "Windows" else None,
disable=not self._show_progress_bar,
):
slide_files = files[pre_slide_config.slides_slice]
file = merge_basenames(slide_files)
dst_file = scene_files_folder / file.name
rev_file = scene_files_folder / f"{file.stem}_reversed{file.suffix}"
# We only concat animations if it was not present
if not use_cache or not dst_file.exists():
concatenate_video_files(slide_files, dst_file)
# We only reverse video if it was not present
if not use_cache or not rev_file.exists():
reverse_video_file(dst_file, rev_file)

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:

  • Implement a split, reverse & concat method (short-term)
  • Hook into (or submit a PR) manim's rendering pipeline so that is directly saves forward and backwards videos. There's already a --save_sections flag for manimCE which we could use such that manim takes care of concatenating animations instead of re-implementing it here.

@jeertmans
Copy link
Owner

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 added a commit that referenced this issue May 22, 2024
Implement a smarter generation of reversed files by splitting the video into smaller segments.

Closes #434
@jeertmans jeertmans linked a pull request May 22, 2024 that will close this issue
@jeertmans
Copy link
Owner

See #439

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lib Related to the library (a.k.a. module)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants