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

moviepy returns empty frames if frames are not accessed strictly sequentially #2044

Open
pgunn opened this issue Sep 28, 2023 · 5 comments
Open
Labels
bug Issues that report (apparent) bugs. lib-FFmpeg Issues pertaining to dependency FFmpeg.

Comments

@pgunn
Copy link

pgunn commented Sep 28, 2023

Expected Behavior

See below - the handle to a video file should not start returning zeros. The video file we're using with this is here:
https://caiman.flatironinstitute.org/~neuro/caiman_downloadables/msCam13.avi

We found this while spotting the error in pims, using the moviepy backend, and eventually tracked it down to moviepy when considering directly using moviepy to avoid what we believed was a bug in pims, instead to find that it's actually a bug in moviepy.

Oddly, accessing frame 0 of the movie fixes the handle to the movie

Actual Behavior

mph = moviepy.editor.VideoFileClip(fn)
mph.size
[752, 480]
mph.duration
50.0
mph.fps
20.0
np.array(mph.get_frame(0)).max()
173
np.array(mph.get_frame(2)).max()
206
np.array(mph.get_frame(1)).max()
0
np.array(mph.get_frame(0)).max()
173
np.array(mph.get_frame(1)).max()
188

Steps to Reproduce the Problem

Specifications

  • Python Version: 3.11.5
  • MoviePy Version: 1.0.3
  • Platform Name: Linux (tested on Windows too)
@pgunn pgunn added the bug Issues that report (apparent) bugs. label Sep 28, 2023
@pgunn
Copy link
Author

pgunn commented Sep 28, 2023

Following it down into the backend:

mph = moviepy.video.io.ffmpeg_reader.FFMPEG_VideoReader(fn)
>>> mph.get_frame(0).max()
173
>>> mph.get_frame(1).max()
188
>>> mph.get_frame(2).max()
206
>>> mph.get_frame(1).max()
0
>>> mph.get_frame(0).max()
173
>>> mph.get_frame(1).max()
188

I'm looking at the logic here:
https://github.com/Zulko/moviepy/blob/master/moviepy/video/io/ffmpeg_reader.py#L218

But I find the existence of self.last_read and self.lastread as distinct variables to be puzzling. I'm guessing the bug is somewhere around there.

@pgunn
Copy link
Author

pgunn commented Sep 30, 2023

It looks like, at least with our datafiles, any use of -ss before -i in the call to ffmpeg causes ffmpeg itself to return empty frames, while the second call to -ss (while not performant) returns data correctly.

Found this by modifying moviepy/video/io/ffmpeg_reader.py to print out the commandline its building with various calls, and looking over the if start_time != 0 block in initialize().

Something like:

ffmpeg -ss 10 -i /home/pgunn/caiman_data/example_movies/msCam13.avi -ss 10 -loglevel info -vf scale=752:480 -sws_flags bicubic -pix_fmt rgb24 /tmp/foo_noprelinear.avi

(really providing anything at all, float or integer, with a first -ss, gives us a blank movie.

Without that initial -ss, we get movies that work.

I'm wondering if it might be better to remove the first -ss from these calls, or at least provide an option to.

@pgunn
Copy link
Author

pgunn commented Sep 30, 2023

I will follow up with the ffmpeg developers.

@pgunn
Copy link
Author

pgunn commented Sep 30, 2023

@keikoro keikoro added the lib-FFmpeg Issues pertaining to dependency FFmpeg. label Feb 10, 2024
@Breakthrough
Copy link
Contributor

Breakthrough commented Nov 23, 2024

But I find the existence of self.last_read and self.lastread as distinct variables to be puzzling. I'm guessing the bug is somewhere around there.

This seems like an oversight in #1170, I tested locally and remaing the fields to all have the same name works correctly. There's also some cases where you can make the variables get out of sync depending on what order you call get_frames/read_frames.

These variables were previously not underscore-prefixed, so one of my projects relies on the presence of lastread. I would request that both names are kept in sync to avoid breaking code that relied on this field. That could be done by using last_read everywhere, and providing a property that aliases it. Submitted PR #2257 to resolve the discrepancy there.

Breakthrough added a commit to Breakthrough/moviepy that referenced this issue Nov 23, 2024
The property `lastread` was renamed to `last_read` in Zulko#1170 however
not all instances were changed. This leads to some cases where only
one of the variables is updated, which can make detection of end-of-file
more complicated than it used to be in MoviePy 1.x.

Additionally, there are third-party projects which relied on the lastread
property as it was never declared as private (e.g. no underscore prefix).
To avoid breaking projects relying on this, we also provide a property
with the old name for backwards compatibility.

Related to Zulko#2044
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that report (apparent) bugs. lib-FFmpeg Issues pertaining to dependency FFmpeg.
Projects
None yet
Development

No branches or pull requests

3 participants