If we were guaranteed that ffmpeg:
* doesn't change API all the time
* doesn't get ABI incompatible between *versions*
* doesn't break codecs/formats for some svn checkouts
* actually made *stable* releases so we know which version of ffmpeg is being used with the gstreamer plugin
... yes we could do that. Until then, we won't do that. It's less work to update the ffmpeg mirror than to handle all those issues.

If a security problem occurs in ffmpeg, a simple request to update the ffmpeg mirror can be done to fix that.

You have to live with the API and ABI changes anyway, and I don't question the layout of having your local ffmpeg tree, however it would certainly be nice if people could opt out of the default build using an obsolete ffmpeg tree; obviously these people would have to face the API and ABI changes, but might also help you towards supporting a fresher API.

I might have been bold with the previous message, but I never said we'd refuse patches to gst-ffmpeg in order to make it build with outside ffmpeg :)

It's just that it won't be the default behaviour.

Hi,

During discussion of the removal of GStreamer 0.8, the topic was brought up again and Josselin Mouette proposed a patch which I'm going to attach:

http://lists.debian.org/debian-devel/2006/12/msg00140.html
========================================
From: Josselin Mouette <joss@debian.org>
Date: Wed, 06 Dec 2006 22:47:59 +0100
Subject: Re: Dropping GStreamer 0.8 for etch


Le mercredi 06 décembre 2006 à 21:48 +0100, Loïc Minier a écrit :
> On Wed, Dec 06, 2006, Josselin Mouette wrote:
> > Maybe it is also not too late to rework the gstreamer0.10-ffmpeg package
> > to link against the Debian libavcodec/libavformat packages. This would
> > save a lot of trouble to the security team.
>
>  This is a separate issue, and the short status on the subject is that
>  upstream thinks this is not possible, but they would accept help on
>  this topic:
>     <http://bugzilla.gnome.org/show_bug.cgi?id=363363>

The attached patch should be at least enough for Debian. Finally working
h264 videos with totem-gstreamer, hear hear!

That's just a hack, and for upstream, this requires of course some
integration in the configure script, but that's definitely not
impossible.
--
Josselin Mouette                /\./\

Created attachment 77878 [details] [review]
Link with system's ffmpeg

Several points:

I mentioned in comment #3 that patches are welcome but they won't be the default behaviour. The proposed patch plainly discards the included ffmpeg tree (gst-libs/...). Such a patch is NOT acceptable. An acceptable patch would be one with a configure option to use system-wide ffmpeg and that option would be disabled by default.

As for h264 support... it works with our mirror of ffmpeg already. Works nicely. gst-ffmpeg should be released anytime soon.

I also forgot to mention that the patch shouldn't modify the code of the plugins as it is doing (regarding the codecid).
Maybe you'll start to understand why we don't link against system-wide installed version....

Created attachment 77937 [details] [review]
Proposed implementation

Loïc forgot to mention this was a quick and dirty patch proposed for the Debian package and not worth forwarding here.

Here is a hopefully better implementation.

I also agree that this shouldn't be made the default. You don't have the same constraints as us, because:
* there is only one copy of ffmpeg in the whole GNOME source tree;
* you have to deal with many distributors which all use different ffmpeg versions.

It is not necessary that you change the codec maps either. If it is possible in your release to build gst-ffmpeg using an external source, then we'll deal with a small patch of the like or some adaptation of our own ffmpeg packages (which have e.g. their own ABI versioning scheme). The point of this request is to have a point of entry which makes building against the system ffmpeg straightforward.

Could you please make the patch against controlled files ?
cvs -z5 diff -up > name.patch

The patch you gave is full of non-version controlled files.
Currently I can't tell if the patch is correct or not because of that.

In fact, I managed to see a little bit what's going on.

You should try to keep the same define as what's currently there (HAVE_FFMPEG_UNINSTALLED), and once again, the code shouldn't be modified. all the #ifdef HAVE_FFMPEG_UNINSTALLED codes are correct.

The only thing you will have to modify is to define HAVE_FFMPEG_UNINSTALLED as 0 or 1, and replace the #ifdef by #if.

As I already explained, the code changes are related to our own ffmpeg version and are not necessarily suitable for GNOME. We can live with such a patch (or probably a more carefully crafted one) in our packages.

I disagree with the HAVE_FFMPEG_UNINSTALLED defines, though. When ffmpeg is installed on the system, the compiler must be passed -I$(somewhere) to find the includes, and the appropriate include line is '#include <avformat.h>'.

The proposed patch was made against the tarball, I'll make one against the latest CVS version.

Josselin, (In reply to comment #12)
> As I already explained, the code changes are related to our own ffmpeg version
> and are not necessarily suitable for GNOME. We can live with such a patch (or
> probably a more carefully crafted one) in our packages.

  Fine by me, then for the proposed patch, do not include those distro-ffmpeg-specific modifications.

Created attachment 78240 [details] [review]
Patch against CVS

Here is a completely cleaned up patch against the CVS version. I've just built both versions of the plugin using it, and they work.

Hrmn, I should have chimed in on this earlier:

* This patch is too late to make it into the 0.10.2 release of gst-ffmpeg (which should be out tonight), but we'll do a 0.10.3 much sooner and try and get it in then.

* I'd like a patch which also AC_SUBSTS a constant like FFMPEG_SOURCE="System install" vs "Local snapshot" and modify the element wrappers to include that in the plugin description, so that when we get a bug report we can easily tell whether it's likely in our local snapshot, or if we should send the user elsewhere.

Created attachment 78315 [details] [review]
Patch with ffmpeg origin description

Here we go. I've added a FFMPEG_SOURCE macro describing where ffmpeg comes from.

Looks good, ta.

The 0.10.2 release is out now, so one of us can look at applying this, probably in the morning

Looks fine for me, please commit.

Doesn't build off the system installed one here, which is not heartening.

In file included from /usr/include/ffmpeg/avutil.h:24,
                 from /usr/include/ffmpeg/avcodec.h:14,
                 from gstffmpeg.c:29:
/usr/include/ffmpeg/common.h:175:1: error: "ABS" redefined
In file included from /usr/lib/glib-2.0/include/glibconfig.h:9,
                 from /usr/include/glib-2.0/glib/gtypes.h:30,
                 from /usr/include/glib-2.0/glib/galloca.h:30,
                 from /usr/include/glib-2.0/glib.h:30,
                 from /home/jan/.install/include/gstreamer-0.10/gst/gst.h:27,
                 from gstffmpeg.c:27:
/usr/include/glib-2.0/glib/gmacros.h:173:1: error: this is the location of the previous definition

@Jan: This is a problem I've already noticed with the system ffmpeg, and it should be fixed in the system ffmpeg. Nothing wrong in gst-ffmpeg here. 

Furthermore, this is only a warning that goes away when building from the tarball (i.e. without -Werror). I have also checked that both definitions are strictly equivalent, meaning this is harmless anyway.

Even with ERROR_CFLAGS="" I still can't get it to build, but the system install in Ubuntu Edgy is older than our current snapshot.

I'm still very dubious about this functionality - every time we update FFMpeg it costs someone a week or so to make sure everything is still working, and I get the feeling that this is just going to make our bug reports harder to manage. I know we rely on plenty of other external libraries, but none of those are as unstable in their API/ABI as FFMpeg (except maybe bloody faad). 

Be warned, I'm planning to close every bug report that mentions "System Install" with a WONTFIX until it's demonstrated that it also occurs against our local snapshot.

Committed:

        * Makefile.am:
        * configure.ac:
        * ext/ffmpeg/Makefile.am:
        * ext/ffmpeg/gstffmpeg.c:
        * ext/ffmpeg/gstffmpegdemux.c:
        * ext/libpostproc/Makefile.am:
        * ext/libpostproc/gstpostproc.c:

        Allow building against an external FFMpeg install. Fixes: #363363
        Patch by: Josselin Mouette <joss at debian dot org>
        When built against an external install, the plugin description will
        say "system install" instead of "local snapshot"