GNOME Bugzilla – Bug 758183
gst-libav-1.6 broke compatibility with libav
Last modified: 2015-12-04 20:38:56 UTC
We build against system ffmpeg/libav and libav-11.3 users noticed a breakage with 1.6 version: https://bugs.gentoo.org/show_bug.cgi?id=565668 % gst-inspect-1.0 -b (gst-plugin-scanner:6030): GStreamer-WARNING **: Failed to load plugin '/usr/lib64/gstreamer-1.0/libgstlibav.so': /usr/lib64/gstreamer-1.0/libgstlibav.so: undefined symbol: av_frame_get_sample_rate Blacklisted files: libgstlibav.so The culprit looks to be this commits: http://cgit.freedesktop.org/gstreamer/gst-libav/commit/?id=7afaf5c02050fcac18006abfdc01287f8dba16eb http://cgit.freedesktop.org/gstreamer/gst-libav/commit/ext/libav/gstavauddec.c?id=3b6c656e2552c936a17ff6bb9a0e753fcc42e4ce Thanks a lot
libav is not officially supported anymore, but if you want to provide a patch for making it work with both that would be acceptable unless it's too intrusive. Note that there were also many other changes necessary to work properly with ffmpeg, which might break more things with libav.
I'm not sure it's worth trying to support both. I'd say, as long as this does not create a ifdef spaghetti, ok, otherwise, we should just support one properly (it's already hard enough).
Right, if it's #ifdef madness we shouldn't do that. It's annoying that ffmpeg and libav use the same sonames and almost same API, but are incompatible in the details unless you use only the very smallest parts of the API.
Maybe we should add some check and just refuse libav (either at runtime or compile time) ?
Yes that would definitely be a good idea until we implemented compatibility
Created attachment 316141 [details] [review] Require libav provided by FFmpeg at build time Additionally a check at run time can be made using the same _VERSION_MICRO trick, not sure its needed (nor desired) after this though.
Comment on attachment 316141 [details] [review] Require libav provided by FFmpeg at build time sooner or later libav will also update its micro version, no? is there no better way for checking this? and imho it should also be a runtime check, not just compile (it should be both)
Micro version gets rolled back with for each minor increment, rationale behind FFmpeg using 100+ was exactly this one, to differentiate: https://libav.org/documentation/developer.html See "3" on 1.6 New codecs or formats checklist for the rollback requirement Will add a runtime check patch tomorrow. Thanks for taking a look
Created attachment 316736 [details] [review] Require libav provided by FFmpeg at run-time Run-time check. Same rationale
Created attachment 316738 [details] [review] Require libav provided by FFmpeg at run-time
Review of attachment 316738 [details] [review]: In general looks good, just some minor comments :) Also please squash together with the other patch ::: ext/libav/gstav.c @@ +51,3 @@ + + /* FFmpeg *_MICRO versions start at 100 and Libav's at 0 */ + if ((av_version & 0xff) < 100) Doesn't it have macros to extract major/minor/micro from the version integer? @@ +142,3 @@ + /* Bail if not FFmpeg. We can no longer ensure operation with Libav */ + if (!gst_ffmpeg_avcodec_is_ffmpeg ()) { Do that as the very first thing in plugin_init() before e.g. setting the log callback
Thanks for taking a look. No, we don't have macros for that. I will likely add some but in the mean time, this would have to do. Pushing after moving the check up as you suggested (OKyed on IRC).
Review of attachment 316141 [details] [review]: commit d9dec6893a0c66870d08bdfbee5b0ad9fcb66674 Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com> Date: Mon Nov 23 23:45:38 2015 -0800 Require libav provided by FFmpeg at build-time Libav-incompatible changes were introduced to support FFmpeg and we can no longer properly support Libav. [..]
Review of attachment 316738 [details] [review]: Pushed with suggested mods commit 193b0d81d1fc85eb2d51847e90b0098e8a111988 Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com> Date: Wed Dec 2 12:27:08 2015 -0800 Require libav provided by FFmpeg at run-time [..]
Setting to resolved - wontfix from the original patch title and scope. Feel free to change if considered wrong somehow