After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 758183 - gst-libav-1.6 broke compatibility with libav
gst-libav-1.6 broke compatibility with libav
Status: RESOLVED WONTFIX
Product: GStreamer
Classification: Platform
Component: gst-libav
1.6.1
Other Linux
: Normal normal
: git master
Assigned To: Reynaldo H. Verdejo Pinochet
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-16 15:39 UTC by Pacho Ramos
Modified: 2015-12-04 20:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Require libav provided by FFmpeg at build time (1.54 KB, patch)
2015-11-24 08:00 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
Require libav provided by FFmpeg at run-time (1.66 KB, patch)
2015-12-03 19:46 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
Require libav provided by FFmpeg at run-time (1.66 KB, patch)
2015-12-03 19:54 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review

Description Pacho Ramos 2015-11-16 15:39:50 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
Comment 1 Sebastian Dröge (slomo) 2015-11-16 15:42:03 UTC
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.
Comment 2 Nicolas Dufresne (ndufresne) 2015-11-16 16:15:09 UTC
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).
Comment 3 Sebastian Dröge (slomo) 2015-11-18 07:49:27 UTC
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.
Comment 4 Olivier Crête 2015-11-18 19:16:17 UTC
Maybe we should add some check and just refuse libav (either at runtime or compile time) ?
Comment 5 Sebastian Dröge (slomo) 2015-11-19 08:14:13 UTC
Yes that would definitely be a good idea until we implemented compatibility
Comment 6 Reynaldo H. Verdejo Pinochet 2015-11-24 08:00:22 UTC
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 7 Sebastian Dröge (slomo) 2015-12-02 08:23:17 UTC
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)
Comment 8 Reynaldo H. Verdejo Pinochet 2015-12-02 08:40:34 UTC
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
Comment 9 Reynaldo H. Verdejo Pinochet 2015-12-03 19:46:28 UTC
Created attachment 316736 [details] [review]
Require libav provided by FFmpeg at run-time

Run-time check. Same rationale
Comment 10 Reynaldo H. Verdejo Pinochet 2015-12-03 19:54:25 UTC
Created attachment 316738 [details] [review]
Require libav provided by FFmpeg at run-time
Comment 11 Sebastian Dröge (slomo) 2015-12-04 08:07:30 UTC
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
Comment 12 Reynaldo H. Verdejo Pinochet 2015-12-04 20:29:05 UTC
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).
Comment 13 Reynaldo H. Verdejo Pinochet 2015-12-04 20:35:52 UTC
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.
    
[..]
Comment 14 Reynaldo H. Verdejo Pinochet 2015-12-04 20:36:54 UTC
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
    
[..]
Comment 15 Reynaldo H. Verdejo Pinochet 2015-12-04 20:38:56 UTC
Setting to resolved - wontfix from the original
patch title and scope. Feel free to change if
considered wrong somehow