-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix 'animation' unable to detect AVConv. #8743
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
Conversation
Sorry for breaking that and thank you for the fix! I have a slight preference to re-override the |
Or we could just remove the method override, which was only added in #8253 so that tests pass on systems where ffmpeg is a shim for avconv (i.e. oldish ubuntus, in particular Travis before the switch to trusty); but as far as I can guess things should still work in that case as long as one is not using the h264 codec. |
@tacaswell That's exactly what I've done initially but I realized it makes the code less readable. The issue was with a specific version of ffmpeg but the test broader than it should be. So rather than overriding the method back to MovieWriter's version I found it better to restrain the test. That way we also keep the fact that AVConv is just FFMPEG with a different path. @anntzer I didn't notice but ubuntu 12.0.4 ended is life April 28, 2017. So removing the override seems a good idea. Is there a way to know if there is still users of this OS ? Whatever the decision for long-term I really believe this fix should be released as soon as possible. Now the animation module is unable to save anything with avconv (which is the only one available within debian stable). |
There are still people using Windows XP, it is up to us whether we want to support them. |
Have I think that restoring the non-libav rejecting version in the sub-class is clearer, bit more code, but less coupling. We should probably leave filter for ffmpeg-shims-on-top-of-avconv in place until we are sure no main-line distros are shipping the shims (and it sounds like ffmpeg is not available on debian stable, do they ship the shims in it's place?). |
Hi, |
Hi,
The commit 926a59f introduced a major bug : animation became unable to load Avconv.
Basically the issue came from the fact that this commit was unaware that AVConvBase inherit from FFMpegBase.
This pull request fix it (it was easy).
I haven't seen any open issue concerning it.
Maybe it could be useful to have a test to check that it doesn't happen again.
But I'm not sure if it's really relevant since the same would happen for someone that didn't install Avconv.
Best,
Élie