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

[Steinar H. Gunderson]

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?

>> +static int GraphKeeper = 0;
> Documentation?

Again, copied from allocator.c. I won't claim to understand to understand how
the refcounting here works. :-)

>> +    if (memcmp(clsid, &CLSID_FilterGraph, sizeof(GUID)))
> sizeof(*clsid) maybe?

Fixed.

>> +    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?

>> +  
> Trailing whitespace here and in many other places.

Fixed.

>> +    FilterGraph* This = (FilterGraph*) malloc(sizeof(FilterGraph));
>> +    This->vt = (IFilterGraph_vt*) malloc(sizeof(IFilterGraph_vt));
> Useless casts. And I'd consider calloc to ensure everything's initialized.

OK, changed.

> And sizeof(*variable) has a lower chance of breaking with future changes
> that sizeof(type), so I generally consider that preferable.

OK, changed.

>> +	graph=FilterGraphCreate();
>> +	result = This->m_pFilter->vt->JoinFilterGraph(This->m_pFilter, (IFilterGraph*)graph, (short *)"F\0i\0l\0t\0e\0r\0");
> Inconsistent space placement in casts.

Fixed.

> 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?

>>  	// 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?

> 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?

/* Steinar */
-- 
Homepage: http://www.sesse.net/