[FFmpeg-devel] [PATCH] dshow: allow for more codecs take 2

[Hendrik Leppkes]

On Thu, Feb 14, 2013 at 9:39 PM, Roger Pack <rogerdpack2 at gmail.com> wrote:
> On 2/13/13, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>> On Wed, Feb 13, 2013 at 11:07 PM, Michael Niedermayer <michaelni at gmx.at>
>> wrote:
>>> On Sat, Feb 09, 2013 at 04:11:09AM +0100, Michael Niedermayer wrote:
>>>> On Fri, Feb 08, 2013 at 06:18:51PM -0700, Roger Pack wrote:
>>>> > On 2/7/13, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>> > > On Wed, Feb 06, 2013 at 08:19:01PM +0100, Michael Niedermayer wrote:
>>>> > >> On Wed, Feb 06, 2013 at 10:55:53AM -0700, Roger Pack wrote:
>>>> > >> > On 2/5/13, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>> > >> > > On Tue, Feb 05, 2013 at 02:24:42PM -0700, Roger Pack wrote:
>>>> > >> > >> On 2/5/13, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>> > >> > >> > On Tue, Feb 05, 2013 at 10:05:52AM -0700, Roger Pack wrote:
>>>> > >> > >> >> On 2/4/13, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>> > >> > >> >> > On Mon, Feb 04, 2013 at 01:50:11PM -0700, Roger Pack
>>>> > >> > >> >> > wrote:
>>>> > >> > >> >> >> On 1/25/13, Roger Pack <rogerdpack2 at gmail.com> wrote:
>>>> > >> > >> >> >> > Ok after the previous discussion
>>>> > >> > >> >> >> > http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2012-October/131942.html
>>>> > >> > >> >> >> > it appears the first patch attached (0001 --
>>>> > >> > >> >> >> > re-attached here
>>>> > >> > >> >> >> > and
>>>> > >> > >> >> >> > modified to apply cleaning to git master) is ok.
>>>> > >> > >> >> >> >
>>>> > >> > >> >> >> > Also attached is a second patch for better error
>>>> > >> > >> >> >> > logging
>>>> > >> > >> >> >> > messages.
>>>> > >> > >> >> >> > Thank you for your consideration in their regard.
>>>> > >> > >> >> >>
>>>> > >> > >> >> >> bump for these 2.  Or maybe you could give Ramiro commit
>>>> > >> > >> >> >> rights
>>>> > >> > >> >> >> I'm
>>>> > >> > >> >> >> sure he wouldn't abuse :)
>>>> > >> > >> >> >
>>>> > >> > >> >> > ramiro has commit rights, he also has a github repo
>>>> > >> > >> >> >
>>>> > >> > >> >> > you are dshow maintainer, thus if you want me to merge
>>>> > >> > >> >> > some git
>>>> > >> > >> >> > repo/branch just say which ...
>>>> > >> > >> >>
>>>> > >> > >> >> Ok please merge the https://github.com/rdp/ffmpeg
>>>> > >> > >> >> "combined"
>>>> > >> > >> >> branch
>>>> > >> > >> >> (Ramiro or Michael maybe whoever gets to it first).
>>>> > >> > >> >> Thank you.
>>>> > >> > >> >
>>>> > >> > >> > merged
>>>> > >> > >> >
>>>> > >> > >> > thanks
>>>> > >> > >>
>>>> > >> > >> Thanks!
>>>> > >> > >>
>>>> > >> > >> I am something of a novice to  this, and appears I made a
>>>> > >> > >> small
>>>> > >> > >> mistake and the "combined" branch was lacking a commit or 2,
>>>> > >> > >> could
>>>> > >> > >> you
>>>> > >> > >> pull and merge it again for me?
>>>> > >> > >> Sorry for the inconvenience.  Hopefully I'll get some
>>>> > >> > >> experience
>>>> > >> > >> with
>>>> > >> > >> this better..
>>>> > >> > >
>>>> > >> > > the combined branch is now in bad shape
>>>> > >> > > If i do merge it, it would result in below (more merge commits
>>>> > >> > > than
>>>> > >> > > non merge commits):
>>>> > >> > >
>>>> > >> > > I suggest you start a new branch and make sure you dont add
>>>> > >> > > merge
>>>> > >> > > commits into that branch, you dont need any ATM
>>>> > >> > > simply cherry pick all commits you need, then look at the
>>>> > >> > > changes
>>>> > >> > > and make sure they are ok and test them to make sure they work
>>>> > >> > > then tell me to merge
>>>> > >> >
>>>> > >> > Oops didn't realize git tracks merges as well.
>>>> > >> > Please then check the branch combined2 for appropriateness and
>>>> > >> > merge
>>>> > >> > if appropriate.
>>>> > >>
>>>> > >> merged
>>>> > >>
>>>> > >> thanks
>>>> > >
>>>> > > this (or possibly another commit) broke msvc10
>>>> > > see:
>>>> > > http://fate.ffmpeg.org/report.cgi?time=20130207203228&slot=x86_32-msvc10-dll-windows-native
>>>> >
>>>> > Inexperience shows its face again.  Shared builds were broken (mingw
>>>> > too).  I've set up my build system to catch them more easily next
>>>> > time.
>>>> > I believe I've figured them out (at least for mingw cross compile it
>>>> > works now) if you could merge rdp/dshow_shared.
>>>> > Thanks for your patience.
>>>>
>>>> merged
>>>>
>>>> thanks
>>>
>>> still seems to not work correctly:
>>>
>>> <BBB-work> someone broke msvc dll builds by marking ff_codec_bmp_tags[]
>>> with av_export
>>> <BBB-work> can whoever did that fix it?
>>> <BBB-work> https://github.com/libav/c99-to-c89/issues/5
>>>
>>>
>>> [...]
>>
>> This is also covered by a fate instance:
>> http://fate.ffmpeg.org/history.cgi?slot=x86_32-msvc10-dll-windows-native
>>
>> Shared tables cannot be used as a constant initializer.
>
> So what I'm understanding is that the following this is a feature of
> c99: "Shared tables used as a constant initializer" (i.e. a c99-c89
> code converter can't overcome it trivially?) so when I made that
> particular variable shared, it introduced it (by making the table
> shared).  I guess that makes sense.  Sorry for introducing the c99
> feature accidentally.

This has nothing to do with C99 or C89.
Its just a quirk between how importing data symbols works in MSVC and
how av_export is defined.

When you want to import a data symbol in MSVC, you have to mark the
definition with declspec(dllimport). Doing this, it basically adds
another indirection to the struct, which results in it not being able
to be used in constant initializers anymore.
However, the usual way to mark such symbols in headers is to have a
macro which at compile time is "declspec(dllexport)" (or empty if you
export it through other means), and when using the header to link
against the lib, the macro is defined to "declspec(dllimport)".

The problem here is that av_export is too dumb. Its always
"declspec(dllimport)", so the symbols get converted into a imported
symbol even if they are in the same library, and because the compiler
doesn't know where the linker will find the symbol, it'll add the
extra indirection, breaking constant initiliazers.
av_export was defined this way "on purpose", to keep it simple,
because the cross-library deps between different libs in the same
project make defining a "correct" macro for this task a bit more
complex, so this caveat was an accepted compromise.