[MPlayer-dev-eng] [PATCH] Implement DirectShow filter graph

[Reimar Döffinger]

On Sat, Mar 06, 2010 at 03:54:10PM +0100, Steinar H. Gunderson wrote:
> On Sat, Mar 06, 2010 at 03:20:04PM +0100, Reimar Döffinger wrote:
> >> +/*
> >> + * Modified for use with MPlayer, detailed changelog at
> >> + * http://svn.mplayerhq.hu/mplayer/trunk/
> >> + */
> > Uh, if it's modified you should obviously also indicate the source.
> > And it should have a proper license header.
> 
> All of this is copied from loader/dshow/allocator.c -- "modified" points to
> the avifile library as a whole, but I can add a comment indicating the
> original source is allocator.c?

Definitely a good idea.

> >> +static int GraphKeeper = 0;
> > Documentation?
> 
> Again, copied from allocator.c. I won't claim to understand to understand how
> the refcounting here works. :-)

Well, obviously you know it is used for reference counting, maybe you
can even guess why reference counting is needed.
Either of that would be a help.

> >> +    p = (IFilterGraph*) FilterGraphCreate();
> >> +    result = p->vt->QueryInterface((IUnknown*)p, iid, ppv);
> >> +    p->vt->Release((IUnknown*)p);
> > Wouldn't that make IUnknown the most appropriate type for p?
> 
> Again, copied from allocator.c. But I can change it if you'd like?

Maybe change it in both places? :-)

> > Also I consider something like
> > static const uint16_t filtername[] = {'F', 'i', 'l', 't', 'e', 'r', 0};
> > more readable, especially since I think your variant
> > 1) is incorrectly terminating (2 bytes 0 are needed)
> > 2) is not necessarily properly aligned (though irrelevant unless there are DirectShow filters
> > for ARM/wince and someone manages to make the work with our loader, but it still isn't
> > nice code).
> 
> Can I use something like L"Filter" here, or would that be a problematic
> gcc-ism?

Never used it, but I think "L" indicates a wide character string, which on
most Linux systems would be uint32_t, not uint16_t, not everyone had the
skills of Microsoft to choose the absolutely most annoying unicode encoding
possible...

> >>  	// enumerate pins
> >>  	result = This->m_pFilter->vt->EnumPins(This->m_pFilter, &enum_pins);
> >>  	if (result || !enum_pins)
> > Mixed tabs and spaces.
> 
> You're talking about the break here, right, not the part you quoted?

No, the part I quoted has 2 space and then a tab, which is the kind
of indentation method I think really never is anything but a pain.

> > I assume there's no way this might break compiling on Windows?
> 
> It shouldn't be relevant for Windows at all; I'd assume it's not even included
> in the compile?

I don't know, we do use the same DirectShow loader code on Windows
(of course we do not use the win32.c etc. stuff).
We can still fix it afterwards, but it would be better to know beforehand
if possible.