[PATCH] disable filters that depend on dsputil.h
Hi, last months we added dsputil.h to the install target of libavcodec; it was a mistake because that file is not supposed to be used outside of libavcodec. This patch disables the inclusion of filters that depend on that header when libavcodec is not imported in main/; if applied I'll post a patch to ffmpeg-devel to restore the previous behaviour (not install dsputil.h). Please, review. Nico
On Tue, Mar 14, 2006 at 12:46:18AM +0100, Nico Sabbi wrote:
Please, review.
--- vf.c 9 Feb 2006 14:07:55 -0000 1.123 +++ vf.c 13 Mar 2006 23:44:23 -0000 @@ -130,7 +130,7 @@ -#ifdef HAVE_ZR +#if defined(HAVE_ZR) && defined(USE_LIBAVCODEC) && ! defined(USE_LIBAVCODEC_SO) &vf_info_zrmjpeg,
This looks like an unrelated bugfix to me..
-#ifdef USE_LIBAVCODEC +#if defined(USE_LIBAVCODEC) && ! defined(USE_LIBAVCODEC_SO)
Hmm, we should create different variables for LIBAVCODEC_STATIC, LIBAVCODEC_SHARED and LIBAVCODEC_EITHER or something similar. I'll try to sit down and implement this later today, it's been on my ToDo for ages. Diego
Nico Sabbi wrote:
Hi, last months we added dsputil.h to the install target of libavcodec; it was a mistake because that file is not supposed to be used outside of libavcodec.
This patch disables the inclusion of filters that depend on that header when libavcodec is not imported in main/; if applied I'll post a patch to ffmpeg-devel to restore the previous behaviour (not install dsputil.h).
Please, review.
Nico -#ifdef HAVE_ZR +#if defined(HAVE_ZR) && defined(USE_LIBAVCODEC) && ! defined(USE_LIBAVCODEC_SO) &vf_info_zrmjpeg, #endif &vf_info_dvbscale,
maybe the above is not necessary -- Email.it, the professional e-mail, gratis per te: http://www.email.it/f Sponsor: Telefona con Email.it Phone Card, tanti minuti di conversazione con il massimo del risparmio, clicca qui Clicca qui: http://adv.email.it/cgi-bin/foclick.cgi?mid=2687&d=14-3
Nico Sabbi wrote:
Nico Sabbi wrote:
Nico -#ifdef HAVE_ZR +#if defined(HAVE_ZR) && defined(USE_LIBAVCODEC) && ! defined(USE_LIBAVCODEC_SO) &vf_info_zrmjpeg, #endif &vf_info_dvbscale,
maybe the above is not necessary
it is, HAVE_ZR is set only if libavcodec (static) is set. Patch updated
2006/3/14, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Hi, last months we added dsputil.h to the install target of libavcodec; it was a mistake because that file is not supposed to be used outside of libavcodec.
This patch disables the inclusion of filters that depend on that header when libavcodec is not imported in main/; if applied I'll post a patch to ffmpeg-devel to restore the previous behaviour (not install dsputil.h).
Please, review.
I'm afraid this could be considered feature removal. The reason (interface purity) doesn't justify the result. However I am not against conditional compilation of these filters. This includes ./configure detection of dsputil.h presence and check for called functions availability.
Ivan Kalvachev wrote:
2006/3/14, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Hi, last months we added dsputil.h to the install target of libavcodec; it was a mistake because that file is not supposed to be used outside of libavcodec.
This patch disables the inclusion of filters that depend on that header when libavcodec is not imported in main/; if applied I'll post a patch to ffmpeg-devel to restore the previous behaviour (not install dsputil.h).
Please, review.
I'm afraid this could be considered feature removal.
it's not, it's the correction of a mistake: as I wrote Michael prefers to keep that file private to libavcodec
The reason (interface purity) doesn't justify the result.
rather than interface purity I consider it interface presence :)
However I am not against conditional compilation of these filters. This includes ./configure detection of dsputil.h presence and check for called functions availability.
static libavcodec involves the presence of dsputil.h Nico
2006/3/15, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Ivan Kalvachev wrote: [...]
However I am not against conditional compilation of these filters. This includes ./configure detection of dsputil.h presence and check for called functions availability.
static libavcodec involves the presence of dsputil.h
So both solutions are equal? Let's say it otherwise. Use: #ifdef HAVE_LAVC_DSP instead of these messy long #if defined(USE_LIBAVCODEC) && ! defined(USE_LIBAVCODEC_SO) fair enough?
Ivan Kalvachev wrote:
2006/3/15, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Ivan Kalvachev wrote:
[...]
However I am not against conditional compilation of these filters. This includes ./configure detection of dsputil.h presence and check for called functions availability.
static libavcodec involves the presence of dsputil.h
So both solutions are equal?
Let's say it otherwise. Use:
#ifdef HAVE_LAVC_DSP
instead of these messy long
#if defined(USE_LIBAVCODEC) && ! defined(USE_LIBAVCODEC_SO)
fair enough?
fine for me; I'll update the patch later -- Email.it, the professional e-mail, gratis per te: http://www.email.it/f Sponsor: Grande Offerta di Benvenuto Cassine di Pietra: * 36 bottiglie di ottimo vino veneto direttamente a casa tua! * Clicca qui: http://adv.email.it/cgi-bin/foclick.cgi?mid=3923&d=15-3
Nico Sabbi wrote:
Nico Sabbi wrote:
Ivan Kalvachev wrote:
So both solutions are equal?
Let's say it otherwise. Use:
#ifdef HAVE_LAVC_DSP
instead of these messy long
#if defined(USE_LIBAVCODEC) && ! defined(USE_LIBAVCODEC_SO)
fair enough?
fine for me; I'll update the patch later
------------------------------------------------------------------------
as for Ivan's request on IIRC I committed only configure, libmpcodecs/Makefile and libmpcodecs/vf.c, because the filter files can be modified in a different way. Compilation is fine even without the changes to the single filters. Nico
On Thu, Mar 16, 2006 at 11:06:35PM +0100, Nico Sabbi wrote:
as for Ivan's request on IIRC I committed only configure, libmpcodecs/Makefile and libmpcodecs/vf.c, because the filter files can be modified in a different way.
And how? IMO this is just code obfuscation. If it cannot and does not work with shared lavc, the code should not imply it does. Diego
On Mon, Mar 20, 2006 at 01:35:38PM +0100, Diego Biurrun wrote:
On Thu, Mar 16, 2006 at 11:06:35PM +0100, Nico Sabbi wrote:
as for Ivan's request on IIRC I committed only configure, libmpcodecs/Makefile and libmpcodecs/vf.c, because the filter files can be modified in a different way.
And how? IMO this is just code obfuscation. If it cannot and does not work with shared lavc, the code should not imply it does.
Nico? Diego
Diego Biurrun wrote:
On Mon, Mar 20, 2006 at 01:35:38PM +0100, Diego Biurrun wrote:
On Thu, Mar 16, 2006 at 11:06:35PM +0100, Nico Sabbi wrote:
as for Ivan's request on IIRC I committed only configure, libmpcodecs/Makefile and libmpcodecs/vf.c, because the filter files can be modified in a different way.
And how? IMO this is just code obfuscation. If it cannot and does not work with shared lavc, the code should not imply it does.
Nico?
Diego
sorry for the delay, I missed your previous mail. Ivan's intention (IIRC) was to change the #include directive so as if users use external libavcodec and copy dsputil.h in a global include directive those filters can be still used -- Email.it, the professional e-mail, gratis per te: http://www.email.it/f Sponsor: Logos Finanziaria SPA. Società di credito ad erogazione diretta. Fino a 30.000 euro in 24 ore! Clicca e scopri come * Clicca qui: http://adv.email.it/cgi-bin/foclick.cgi?mid=2909&d=29-3
Nico Sabbi wrote:
Diego Biurrun wrote:
On Mon, Mar 20, 2006 at 01:35:38PM +0100, Diego Biurrun wrote:
On Thu, Mar 16, 2006 at 11:06:35PM +0100, Nico Sabbi wrote:
as for Ivan's request on IIRC I committed only configure, libmpcodecs/Makefile and libmpcodecs/vf.c, because the filter files can be modified in a different way.
And how? IMO this is just code obfuscation. If it cannot and does not work with shared lavc, the code should not imply it does.
Nico?
Diego
sorry for the delay, I missed your previous mail.
Ivan's intention (IIRC) was to change the #include directive so as if users use external libavcodec and copy dsputil.h in a global include directive those filters can be still used
After Diego's request on irc I committed the changes to the filter files, too.
2006/4/17, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Nico Sabbi wrote:
Diego Biurrun wrote:
On Mon, Mar 20, 2006 at 01:35:38PM +0100, Diego Biurrun wrote:
On Thu, Mar 16, 2006 at 11:06:35PM +0100, Nico Sabbi wrote:
as for Ivan's request on IIRC I committed only configure, libmpcodecs/Makefile and libmpcodecs/vf.c, because the filter files can be modified in a different way.
And how? IMO this is just code obfuscation. If it cannot and does not work with shared lavc, the code should not imply it does.
Nico?
Diego
sorry for the delay, I missed your previous mail.
Ivan's intention (IIRC) was to change the #include directive so as if users use external libavcodec and copy dsputil.h in a global include directive those filters can be still used
After Diego's request on irc I committed the changes to the filter files, too.
Never do that again. I did flame diego on irc. Do you really want to flame you too? Always put warning and arguments on the mailling list. Diego is not project leader, nor some kind of dictator to overthrow other developers decisions.
On Wed, Apr 19, 2006 at 03:22:28PM +0300, Ivan Kalvachev wrote:
2006/4/17, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Nico Sabbi wrote:
Diego Biurrun wrote:
On Mon, Mar 20, 2006 at 01:35:38PM +0100, Diego Biurrun wrote:
On Thu, Mar 16, 2006 at 11:06:35PM +0100, Nico Sabbi wrote:
as for Ivan's request on IIRC I committed only configure, libmpcodecs/Makefile and libmpcodecs/vf.c, because the filter files can be modified in a different way.
And how? IMO this is just code obfuscation. If it cannot and does not work with shared lavc, the code should not imply it does.
Nico?
sorry for the delay, I missed your previous mail.
Ivan's intention (IIRC) was to change the #include directive so as if users use external libavcodec and copy dsputil.h in a global include directive those filters can be still used
After Diego's request on irc I committed the changes to the filter files, too.
Never do that again.
I did flame diego on irc. Do you really want to flame you too?
Always put warning and arguments on the mailling list. Diego is not project leader, nor some kind of dictator to overthrow other developers decisions.
Nor are you if I may remind you. You have things backwards. It's not Nico and I that have to justify our actions, it's you. Shared lavc support was implemented by myself and Nico. While doing this Nico put some #ifdef lines in lavc-dependent filters. Michael spoke up against (mis)using nonpublic header files, so the support for shared lavc was removed from these filters again. If you want to keep some leftover #ifdef lines in a filter, come up with a good reason to keep them and a working implementation *first*. #ifdef is to be avoided, especially if it just obfuscates the code without working. Diego
2006/4/19, Diego Biurrun <diego@biurrun.de>:
On Wed, Apr 19, 2006 at 03:22:28PM +0300, Ivan Kalvachev wrote:
2006/4/17, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Nico Sabbi wrote: [...] After Diego's request on irc I committed the changes to the filter files, too.
Never do that again.
I did flame diego on irc. Do you really want to flame you too?
Always put warning and arguments on the mailling list. Diego is not project leader, nor some kind of dictator to overthrow other developers decisions.
Nor are you if I may remind you.
You have things backwards. It's not Nico and I that have to justify our actions, it's you.
I don't have the right to write angry mails, when somebody does something sneaky behind my back? You may have not noticed, but I don't like the WAY this was committed, not the changes that were committed. Actually Nico was quite reasonable and even explained quite well my position in this thread.
Shared lavc support was implemented by myself and Nico. While doing this Nico put some #ifdef lines in lavc-dependent filters. Michael spoke up against (mis)using nonpublic header files, so the support for shared lavc was removed from these filters again.
Michael was against installing internal headers as part of ffmpeg installation. AFAIK he never argued against using them to build *spp filter with external ffmpeg. (of course using dynamically linked ffmpeg is quite risky in such case, but static linking must be OK).
If you want to keep some leftover #ifdef lines in a filter, come up with a good reason to keep them and a working implementation *first*. #ifdef is to be avoided, especially if it just obfuscates the code without working.
I'm going to do one step more. I'm starting new thread with patch that removes about 28 lines total while bringing back the "left overs". It uses a trick very similar to the one freetype uses. Let's talk about code.
On Tue, Mar 14, 2006 at 02:09:05PM +0200, Ivan Kalvachev wrote:
2006/3/14, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Hi, last months we added dsputil.h to the install target of libavcodec; it was a mistake because that file is not supposed to be used outside of libavcodec.
This patch disables the inclusion of filters that depend on that header when libavcodec is not imported in main/; if applied I'll post a patch to ffmpeg-devel to restore the previous behaviour (not install dsputil.h).
Please, review.
I'm afraid this could be considered feature removal.
No, because shared lavc has only lived in CVS for a few weeks now.
The reason (interface purity) doesn't justify the result.
Michael has clearly spoken out against dsputil.h being considered part of the public interface of FFmpeg... Diego
On Tue, Mar 14, 2006 at 02:09:05PM +0200, Ivan Kalvachev wrote:
2006/3/14, Nico Sabbi <nicola_sabbi@fastwebnet.it>:
Hi, last months we added dsputil.h to the install target of libavcodec; it was a mistake because that file is not supposed to be used outside of libavcodec.
This patch disables the inclusion of filters that depend on that header when libavcodec is not imported in main/; if applied I'll post a patch to ffmpeg-devel to restore the previous behaviour (not install dsputil.h).
Please, review.
I'm afraid this could be considered feature removal. The reason (interface purity) doesn't justify the result.
However I am not against conditional compilation of these filters. This includes ./configure detection of dsputil.h presence and check for called functions availability.
It's not feature removal because the feature is still there if you compile with (the only supported way) internal lavc. Rich
participants (5)
-
Diego Biurrun -
Ivan Kalvachev -
Nico Sabbi -
Nico Sabbi -
Rich Felker