On Wed, Oct 03, 2012 at 07:37:29AM -0700, Ronald S. Bultje wrote:
> On Wed, Oct 3, 2012 at 4:02 AM, Diego Biurrun <di...@biurrun.de> wrote:
> > On Tue, Oct 02, 2012 at 05:48:26PM -0700, Ronald S. Bultje wrote:
> >> 2) a lot of ff_ symbols, and dsputil_init, are accessed from outside
> >> the library in which they exist. These are bugs and should be fixed (i.e.
> >> renamed).
> >
> > You seem to be working on the wrong tree.
>
> This is Chrome's tree.
I suggest fixing our tree first and forward-porting it to Chrome's tree
afterwards; should be much quicker this way.
> >> --- a/configure
> >> +++ b/configure
> >> @@ -2616,7 +2616,7 @@ probe_cc(){
> >> _flags_filter=msvc_flags
> >> - _ld_lib='lib%.a'
> >> + _ld_lib='%.lib'
> >> _ld_path='-libpath:'
> >
> > Do static libs still work as expected?
>
> Will test.
>
> >> @@ -3078,11 +3078,13 @@ case $target_os in
> >> shlibdir_default="$bindir_default"
> >> SLIBPREF=""
> >> SLIBSUF=".dll"
> >> + LIBPREF=""
> >> + LIBSUF=".lib"
> >
> > How does this affect normal gcc MinGW builds?
>
> Haven't tested yet. It probably breaks it right now, I'll unbreak that later.
I don't mind RFC patches, but please mark them as such.
> >> --- a/libavcodec/golomb.h
> >> +++ b/libavcodec/golomb.h
> >> @@ -33,13 +33,14 @@
> >>
> >> -extern const uint8_t ff_golomb_vlc_len[512];
> >> -extern const uint8_t ff_ue_golomb_vlc_code[512];
> >> -extern const int8_t ff_se_golomb_vlc_code[512];
> >> -extern const uint8_t ff_ue_golomb_len[256];
> >> +extern AVCODEC_SYMBOL const uint8_t ff_golomb_vlc_len[512];
> >> +extern AVCODEC_SYMBOL const uint8_t ff_ue_golomb_vlc_code[512];
> >> +extern AVCODEC_SYMBOL const int8_t ff_se_golomb_vlc_code[512];
> >> +extern AVCODEC_SYMBOL const uint8_t ff_ue_golomb_len[256];
> >
> > I somehow doubt marking ff_-prefixed symbols is necessary.
> >
> >> --- a/libavcodec/raw.h
> >> +++ b/libavcodec/raw.h
> >> @@ -28,13 +28,14 @@
> >>
> >> -extern const PixelFormatTag ff_raw_pix_fmt_tags[];
> >> +extern AVCODEC_SYMBOL const PixelFormatTag ff_raw_pix_fmt_tags[];
> >
> > same
>
> So, yes, but, these are used between libraries. E.g.
> ff_raw_pix_fmt_tags is used in libavformat:
> dhcp-172-31-81-183:ffmpeg rbultje$ grep ff_raw_pix_fmt_tags libavformat/*.c
> libavformat/utils.c:
> if(ff_find_pix_fmt(ff_raw_pix_fmt_tags, tag) == st->codec->pix_fmt)
No:
$ git grep -l ff_raw_pix_fmt_tags
libavcodec/raw.c
libavcodec/raw.h
libavcodec/rawdec.c
> and the golomb symbols are used by the golomb fate test. Maybe we can
> link in a bunch of other .c files from libavcodec instead of linking
> to avcodec.dll instead (I agree that it's ugly). Note that gcc/mingw
> shared lib build (see symbol list in libavcodec.v) also exports those
> symbols.
No:
biurrun@passion:/opt/biurrun/libav $ git grep ff_.e_golomb_
libavcodec/golomb.c:const uint8_t ff_ue_golomb_vlc_code[512]={
libavcodec/golomb.c:const int8_t ff_se_golomb_vlc_code[512]={
libavcodec/golomb.c:const uint8_t ff_ue_golomb_len[256]={
libavcodec/golomb.h:extern const uint8_t ff_ue_golomb_vlc_code[512];
libavcodec/golomb.h:extern const int8_t ff_se_golomb_vlc_code[512];
libavcodec/golomb.h:extern const uint8_t ff_ue_golomb_len[256];
libavcodec/golomb.h: return ff_ue_golomb_vlc_code[buf];
libavcodec/golomb.h: return ff_ue_golomb_vlc_code[buf];
libavcodec/golomb.h: return ff_se_golomb_vlc_code[buf];
libavcodec/golomb.h: put_bits(pb, ff_ue_golomb_len[i], i+1);
biurrun@passion:/opt/biurrun/libav $ git grep ff_golomb_vlc
libavcodec/golomb.c:const uint8_t ff_golomb_vlc_len[512]={
libavcodec/golomb.h:extern const uint8_t ff_golomb_vlc_len[512];
libavcodec/golomb.h: LAST_SKIP_BITS(re, gb, ff_golomb_vlc_len[buf]);
libavcodec/golomb.h: LAST_SKIP_BITS(re, gb, ff_golomb_vlc_len[buf]);
libavcodec/golomb.h: LAST_SKIP_BITS(re, gb, ff_golomb_vlc_len[buf]);
> >> +#ifndef AVCODEC_SYMBOLS_H
> >> +#define AVCODEC_SYMBOLS_H
> >> +
> >> +#include "libavutil/avconfig.h"
> >> +
> >> +#if AV_HAVE_SHARED_LIBS && defined(_MSC_VER) &&
> >> !defined(COMPILING_avcodec)
> >
> > What's AV_HAVE_SHARED_LIBS? I'm not aware of it anywhere.
> > What do you need avconfig.h for?
>
> These AVCODEC_SYMBOL things will be used in public headers, thus the
> definition needs t exist in a public header. Since I need to use
> __declspec(dllimport) only for .dll (shared) builds, not for .lib
> (static) builds, I need to know in a public header whether I'm a
> shared lib or not - hence AV_HAVE_SHARED_LIBS (I added that to
> configure, based on CONFIG_SHARED).
The configure change you speak about is missing then; nothing adds
that define to avconfig.h ...
> >> +#ifndef AVUTIL_SYMBOLS_H
> >> +#define AVUTIL_SYMBOLS_H
> >> +
> >> +#include "libavutil/avconfig.h"
> >
> > No need for the "libavutil/" prefix, it's the same directory.
>
> It's generated, i.e. it lives in the build dir, not in the source dir,
> so if you don't have the libavutil prefix, it won't build, since the
> header isn't in the same dir (sourcedir vs. builddir) as the file that
> included it. See other files including specifically avconfig.h, they
> all use the libavutil/ prefix.
Yes, right, I forgot it is a generated header.
> >> --- a/library.mak
> >> +++ b/library.mak
> >> @@ -94,7 +95,7 @@ endef
> >>
> >> -$(EXAMPLES) $(TESTPROGS) $(TOOLS): $(THIS_LIB) $(DEP_LIBS)
> >> +#$(EXAMPLES) $(TESTPROGS) $(TOOLS): $(THIS_LIB) $(DEP_LIBS)
> >> $(TESTPROGS): $(SUBDIR)$(LIBNAME)
> >
> > stray change
>
> No, that's fully intentional, although it should be removed.
Please don't leave cruft behind.
> So, what this does, is that it adds DEP_LIBS (plus unrelated stuff)
> as a dependency to TESTPROGS, i.e. *.dll becomes a dep for e.g.
> dct-test.exe. If you look at the link rule for TESTPROGS, it uses
> $$< to link in all listed dependencies.
No, that rule uses $$^, which has different semantics.
> Now note that you shouldn't link
> against .dll files on Windows, you generate .lib files that contain
> the linker information to link against .dll files (but .lib without
> .dll is a static build, sorry if this is confusing). The .lib files
> are already part of FFEXTRALIBS. So what you end up getting is a
> linker that tries to link against .dll files directly (link
> -libpath:libavcodec -out:dct-test.exe dct-test.o [other object files]
> libavcodec/libavcodec.dll [other dlls] libavcodec.lib [other libs]
> [external lib dependencies]), which doesn't make any sense, and
> obviously fails. This change removes the dependency on the .dll as a
> listed dep, and thus instead it just links like "link
> -libpath:libavcodec -out:dct-test.exe dct-test.o [other object files]
> libavcodec.lib [other libs] [external lib dependencies]", which works
> as expected.
Does it pass "make check"? I'm suspicious that you may have broken some
test programs - some of them need to directly link against the libs
because they access internal symbols...
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
