Skip to content

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

Merged
merged 2 commits into from
Jul 13, 2017
Merged

Fix 'animation' unable to detect AVConv. #8743

merged 2 commits into from
Jul 13, 2017

Conversation

ElieGouzien
Copy link
Contributor

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

@tacaswell tacaswell added this to the 2.0.3 (next bug fix release) milestone Jun 11, 2017
@tacaswell
Copy link
Member

Sorry for breaking that and thank you for the fix!

I have a slight preference to re-override the _handle_subprocess in AVConvBase instead of adding the string test against the class name (which would also fix this in the case of a user sub-class of AVConvBase which does not include 'AVConv' in the class name).

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.0.3 (next bug fix release) Jun 11, 2017
@anntzer
Copy link
Contributor

anntzer commented Jun 11, 2017

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.

@ElieGouzien
Copy link
Contributor Author

ElieGouzien commented Jun 11, 2017

@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).

@anntzer
Copy link
Contributor

anntzer commented Jun 11, 2017

There are still people using Windows XP, it is up to us whether we want to support them.

@tacaswell
Copy link
Member

Have ffmpeg and avconv sorted out their issues an re-merged? My understanding was that avconv is a hostile fork of ffmpeg with a diverging CLI.

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?).

@ElieGouzien
Copy link
Contributor Author

Hi,
I've moved the fix as an overwriting in AVConvBase.
Since the commit that introduced the bug is quite recent I totally agree on just applying my patch and letting as it is for few years.

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 13, 2017
@dopplershift dopplershift merged commit 8e517d8 into matplotlib:master Jul 13, 2017
@ElieGouzien ElieGouzien deleted the fix-anim-avconv branch October 14, 2017 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants