The N Habits of Highly Defective DirectShow Applications

First off, let me pay proper homage by linking to the inspiration for this post, The n Habits of Highly Defective Windows Applications. If you do Windows development, I highly recommend reading through this article; it covers numerous pitfalls in Windows application development.

Anyway, I stumbled upon that article and though that it could use a DirectShow version, since there are many poor examples of DirectShow coding floating around. Many of the defects discussed in this post are common and have been addressed in the newsgroups numerous times. Generally when something is going amiss in a DirectShow program, it's for one of the following reasons.

If you find that I've listed something you're doing (or not doing) in your program, don't take it personally. I know most of the below are defects from personal experience, which is why I've been nice enough to compile them so you don't have to suffer like I did (and trust me, a messed up DirectShow program is a way to truly suffer). I think of this article as a means of helping people improve their code, rather than an attempt on my behalf to belittle other people's coding practices.

In keeping with the original article's overall layout, I've assigned a severity to defects:

• Potentially Fatal: The use of this technique shouldn't work at all. If it appears to, it is an illusion.
  
• Potentially Harmful: There is an excellent chance that minor changes in the program will result in a malfunctioning program, or necessitate gratuitous changes that a proper program construction would not have required.
  
• Inefficient: this covers most polling situations. You will be wasting vast amounts of CPU time and accomplishing nothing.
  
• Poor Style: While the code works, it represents poor style of programming. It should be cleaned up to something that is manageable.
  
• Deeply Suspect: While the code works, there is almost certainly a better way to accomplish the same task which would be simpler, faster, cleaner, or some other suitably positive adjective. Generally, there are sometimes valid reasons for using the technique specified, but they are fairly rare and easily identified. You probably don't have one of these reasons.

Not using Smart Pointers
Severity: Potentially Harmful, Poor Style, Deeply Suspect

The most beneficial thing you can do for your program is use Smart Pointers instead of declaring regular COM pointers.

What are smart pointers, you ask? Smart Pointers are a simple wrapper class around a COM pointer which help manage the object's scope, in addition to several other nice perks. Instead of doing this:

IGraphBuilder * pIGraphBuilder;    // A regular stupid pointer.  This is bad.
HRESULT hr = CoCreateInstance( CLSID_FilterGraph, NULL, CLSCTX_INPROC_SERVER, IID_IGraphBuilder, ( void ** ) &pIGraphBuilder );   // yuk!

...try doing this:

CComPtr pIGraphBuilder;    // A Smarter pointer--much better.
HRESULT hr = pIGraphBuilder.CoCreateInstance( CLSID_FilterGraph );  // just a tad less ugly than the one above

Using smart pointers will:

• Make your code much cleaner.
  
• Make your code much safer.
  
• Enable you to do things that normally would be difficult, clumsy, and/or dangerous with normal pointers.
  

Since this is such a huge subject and so many people make the mistake of not using Smart Pointers(this is unsurprising, given that the DirectShow documentation itself does not use smart pointers--shame on MSFT!), I've written an entire article devoted to this very subject. Also, The March Hare (another newsgroup junkie) has a quick tutorial on his site--click here to visit that article.

Not using Smart Pointers will make any substantial DirectShow program buggy, unreliable, unstable, and prone to leaking memory. Using them also makes DirectShow code more maintainable and easy to read.

Using wrong synchronization primitives (i.e.: mutex, critical sections, etc.) for a given task.
Severity: Inefficient, Poor Style

Programming audio and video is a threaded endeavor, by its very nature. As such, Microsoft has provided us with all sorts of ways to manage threads. The three most common ways of doing this are:

1. Mutexes - A mutex is a synchronization primitive that prevents two threads from executing the same code simultaneously. The two threads do not need to be in the same process space. See here for more information.
   
2. Critical Sections - A Critical Section is a synchronization primitive that prevents two threads from executing the same code simultaneously (yes, its function is identical to that of a mutex). The two threads need to be in the same process space. See here for more information.
   
3. Semaphores - A semaphore is a synchronization primitive that maintains a count between zero and a specified maximum value. The count is decremented each time a thread completes a wait for the semaphore and incremented each time a thread releases the semaphore. When the count reaches zero, no more threads can successfully wait for the semaphore object state to become signaled. The state of a semaphore is set to signaled when its count is greater than zero, and non signaled when its count is zero. The semaphore object is useful in controlling a shared resource that can support a limited number of users. It acts as a gate that limits the number of threads sharing the resource to a specified maximum number. See here for more information.
   

Phew!

Because a mutex actually shares a lock between processes (rather than within the same process space), it involves more overhead than a critical section, which only has a scope of the host process.

Semaphores aren't typically used in DirectShow coding, because they're more applicable to applications where the need to limit access to a given number of threads is a requirement (e.g.: database applications, web servers, etc.). While a semaphore with a one-thread limit is functionally the same thing as a mutex or critical section, the performance is not.

Generally, a critical section is the correct primitive to be using in DirectShow. Using anything else is suspect.

Lastly, using critical sections in filter and DirectShow code is easier as some very kind person has been nice enough to write the two classes that are the subject of our next bad habit:

Not using CCritSec and CAutoLock
Severity: Potentially Harmful, Inefficient, Poor Style, Deeply Suspect

CCritSec and CAutoLock are two classes provided by the DirectShow SDK that help simplify the implementation of thread synchronization.

The CCritSec class wraps a critical section object and contains methods to lock and unlock the critical section at will--additionally, it handles created and destruction of the critical section, making it very handy for keeping your code leak free.

However, the CAutoLock is where things get really slick. Observe the following code:

HRESULT CMyClass::SomeMethod()
{
// We can't have threads accessing this at will!
CAutoLock MyLock( &myCCritSecInstance );

// do something...

// Here we can return without releasing the lock--because the
// destructor of the CAutoLock class takes care of it for us
return S_OK;
}

...The beauty of the above code is that you can't forget to release the lock to your critical section because it's managed by our CAutoLock instance. This code, on the other hand, is heinously unsafe:

HRESULT CMyClass::SomeMethod()
{
myCCritSecInstance.Lock();

// Do something for a few hundred lines of code where we potentially
// forget to release the above lock, encounter an exception, or
// have some sort of failure that we didn't account for.  This results in a
// deadlock.

myCCritSecInstance.Unlock();

return S_OK;
}

...the bad thing about it is that the programmer must do their own accounting throughout the code. It can be done, but the probability of programmer error increases substantially, and the resulting code is difficult to maintain and troubleshoot.

Why make an accounting nightmare? It's much easier and more reliable to let C++ handle scope than to attempt to deal with manually locking and unlocking critical sections. There's also an entire article devoted to using these classes for thread protection.

Not protecting graph construction and destruction with a synchronization primitive.
Severity: Potentially Fatal


Because graph construction and destruction can take substantial amounts of time to happen, the possibility of these two procedures overlapping becomes quite realistic.

I've seen this error several times. Somebody will post code that looks something like:

// Somewhere, Graph Builder is defined...
CComPtr m_pGraph;

// Function to build graph
HRESULT BuildMyGraph()
{
HRESULT hr = m_pGraph.CoCreateInstance( CLSID_FilterGraph );

// ...a couple hundred lines of code and setup pass

hr = m_pGraph->Render( &pSomeIPin );
return hr;
}


// Function to tear down graph
HRESULT TearDownGraph()
{
m_pGraph.Release();
return S_OK;
}

...there's nothing wrong with the above code, except for the fact that m_pGraph isn't protected by any thread synchronization mechanism. If your calls to the functions are protected by a critical section, great--read no further. Otherwise, the above code is dangerous because someone may call TearDownGraph() right in the middle of a call to BuildMyGraph(), which results in m_pGraph being a null pointer and ultimately an application crash.

If a programmer is really unlucky, the above code will always work until someone runs it on a dual core machine, or a faster/slower machine than your development machine, etc. Then they have something that worked without fail in the office, but crashes and burns out in the field.

Ideally, the above should be protected with a critical section that makes it impossible to call the two functions simultaneously. One would have to wait for the other to finish, and vice-versa.

Ideally, the above code should resemble something like so in order to be thread safe:

// Function to build graph
HRESULT BuildMyGraph()
{
// LOCK THIS CODE
CAutoLock cAutoLock( &m_pMyCCritSecInstance );
CComPtr m_pGraph;
HRESULT hr = m_pGraph.CoCreateInstance( CLSID_FilterGraph );

// ...a couple hundred lines of code and setup pass

m_pGraph->Render( &pSomeIPin );
return hr;
}


// Function to tear down graph
HRESULT TearDownGraph()
{
// LOCK THIS CODE
CAutoLock cAutoLock( &m_pMyCCritSecInstance );

m_pGraph.Release();
return S_OK;
}

This is a pretty simplified example, but the point stands.

Assigning smart pointer to a regular pointer.
Severity: Potentially Fatal

One of the benefits of smart pointers is that you can assign one smart pointer to another smart pointer because someone has cleverly overloaded the "=" operator. This is very useful; you can use local smart pointers to build a graph and only assign them at the end of the build process, thus never keeping around any permanent COM pointers unless necessary. However, trying to assign a regular pointer to a smart pointer is a big, big mistake.

I've seen programmers do this several times. A lot of this comes from the fact that people typically don't use smart pointers to begin with because none of the DirectShow samples use them. So you'll see something that ends up like so:

// Somewhere:
IGraphBuilder * m_pIGraphBuilder;

// Somewhere else:
CComPtr pIGraphBuilder;
pIGraphBuilder.CoCreateInstance( CLSID_FilterGraph );

// And, somewhere else...
m_pIGraphBuilder = pIGraphBuilder;   // this is bad!

...The insidious thing about this error is that m_pIGraphBuilder will appear to be a valid pointer, but drilling down to the guts, it'll be null and ultimately result in a crash.

Not calling CoInitialize and CoUninitialize on new threads.
Severity: Potentially Fatal or Harmful

One of the fundamental rules of modern-day COM is that every thread that uses COM should first initialize COM by calling either CoInitialize or CoInitializeEx. Not doing so can result in some strange behavior--if you're lucky, the behavior will just be failed COM API calls. If you're not so lucky, the problems can be rather sublime.

An example of correctly calling CoInitialize on a new thread:

unsigned int dwThreadId = 0;
HANDLE hThreadHandle;
hThreadHandle = (HANDLE) _beginthreadex( NULL, 0, BuildMyFilterGraph, pMyData, 0, &dwThreadId );

...the above code launches a new thread, calling some generic method/function called BuildMyFilterGraph. Now, in BuildMyFilterGraph:

unsigned int __stdcall BuildMyFilterGraph( LPVOID lpParam )
{
CoInitializeEx( NULL, COINIT_APARTMENTTHREADED );

// Build our graph, do something, etc....

// When we're ready to exit the thread
CoUninitialize();
return 1;
}

So, the above calls CoInitializeEx when the thread is launched, and calles CoUninitialize when it's all done. Great.

However, the above code has the same issue that not using smart pointers has--what if the thread exits before calling CoUninitialize? What if the function is modified by another programmer who exits the thread and forgets to call CoUninitialize? We could account for each and every one of these exits by calling CoUninitialize directly, but such a solution is clumsy at best, especially given that DirectShow code tends to be sequential and the accounting often snowballs out of control.

The solution to this is actually quite simple:

class CInitCom
{
public:
CInitCom(DWORD dwCoInit = COINIT_APARTMENTTHREADED )
{
CoInitializeEx( NULL, dwCoInit );
}
~CInitCom()
{
CoUninitialize();
}
};

A coworker of mine clued me in on this class, and it's quite elegant: the constructor calls CoInitializeEx, and the subsequent call to CoUninitialize is managed entirely by the scope of the class.

Using this class, our BuildMyFilterGraph method/function now becomes:

unsigned int __stdcall BuildMyFilterGraph( LPVOID lpParam )
{
CInitCom cInitCom;

// Build our graph, do something, etc....

// Here, we just exit.  The call to cInitCom's destructor will take care
// of calling CoUninitialize for us.
return 1;
}

We no longer have to clean up after ourselves. The code is protected from people exiting or modifying the function/method in strange ways, and much cleaner overall.


Deriving transform filter from CTransInPlace when CTransform should be used.
Severity: Inefficient, Deeply Suspect


Probably once a month or so you'll see someone post a plea for help on the newsgroups about how their transform filter works, but it's really slow. This is almost always a consequence of people deriving their filter from CTransInPlaceFilter instead of CTransformFilter.

The difference between CTransInPlaceFilter and CTransformFilter can appear to be subtle, but they're really two very different creatures. Many people are attracted to the very simple implementations of CTransInPlaceFilter derived filters, which can be as short as fifty lines of code (see the null filter sample in the DirectShow samples for a good example of this). CTransInPlaceFilter derived implementations modify the frame data of passing samples in its original buffer and don't have their own allocator.

To paraphrase the notes in the base classes defining In-Place Transform Filters: An in-place transform tries to do its work in someone else's buffers. It tries to persuade the filters on either side to use the same allocator (and for that matter the same media type). So, essentially, an In-Place Transform filter that is positioned directly upstream from a video rendering filter typically ends up having access to frame data after it already resides in video memory.

Writing to video memory is really, really fast--reading from video memory is really, really slow. So if you read from the buffer in the IMediaSample supplied in the Transform method of a CTransInPlaceFilter, you can expect to have seriously slow performance. Writing to the buffer is very quick and incurs no additional penalty, but any sort of read operation will incur steep penalties.

Moral of the story: don't read from video memory. The most common way this happens is people deriving their transform filters from CTransInPlaceFilter instead of CTransformFilter, which has its own allocators. CTransformFilter is the appropriate base class to use if you're going to be reading from a buffer as it passes through your transform filter.


Not Checking returned HRESULT values
Severity: Potentially Harmful, Poor Style, Deeply Suspect

Almost all DirectShow functions (and COM functions, for that matter) return an HRESULT data type. This result indicates if a given function was successful or if it failed. Often times, the failure is very specific and can lead to easy troubleshooting of a problem. Other times, a function returns success, but it has multiple successful returns.

Knowing that, it's very clear that not checking an HRESULT for failure or success is a great way to write a program that fails miserably.

Observe the following function:

// Code creates a Filter graph manager and assigns it to a member
// variable.
HRESULT InstantiateGraphbuilder()
{
CComPtr pIGraphBuilder;
pIGraphBuilder.CoCreateInstance( CLSID_FilterGraph );

m_pIGraphBuilder = pIGraphBuilder;
return S_OK;
}

So, the above code has several flaws. For starters, CoCreateInstance returns an HRESULT, which the above code does absolutely nothing with. It falls right on the floor. If you forgot to initialize COM and called the above code, it would fail, attempt to assign the pointer, and the programmer would have no idea why the program failed, simply because there was no check for the HRESULT return value. This is a somewhat simplified example of forgetting to check the HRESULT, but the general idea is the same.

The second flaw is returning S_OK, which is the general HRESULT value for success--this is also pointless. This denies the calling function from making sure things were successful or taking any sort of action based on a more specific return value. As it stands, the above code might as well have a void return value.

A much better implementation:

// Code creates a Filter graph manager and assigns it to a member
// variable.
HRESULT InstantiateGraphbuilder()
{
CComPtr pIGraphBuilder;
HRESULT hr = pIGraphBuilder.CoCreateInstance( CLSID_FilterGraph );
if( FAILED( hr ) )    // CHECK RETURN VALUE USING FAILED MACRO
{
return hr;
}
else
{
m_pIGraphBuilder = pIGraphBuilder;
return hr;
}
}

The above code checks the return value of the CoCreateInstance using the FAILED() macro. You can also check an HRESULT for success using the SUCCEEDED() macro, which also takes an HRESULT as a parameter and functions in exactly the opposite way as FAILED().

This is really just a basic example; there are literally thousands of possible return types for an HRESULT, and certain methods have their own specific set of HRESULT values they return that should be checked consistently.

Lastly, you can do the following to get more information about a given error:

#include 

HRESULT hr = S_OK;

// we encounter some error which has been assigned to hr

CString Error( ::DXGetErrorString9( hr ) );
Error += ::DXGetErrorDescription9( hr );

...this is nice because it's easy to output it to a message log or display the error in English rather than some weird value in hex. Additionally, there's a utility in the DXSDK called "DirectX Error Lookup"; as the name implies it will look up a given hex or decimal value and display the error for you.

Forgetting to catch and examine HRESULT values consistently is, without a doubt, deeply suspect. Additionally, make sure functions return HRESULT values that are useful.

Using a do...while(0) loop
Severity: Potentially Harmful, Poor Style, Deeply Suspect

Observe the following function:

HRESULT ShowFilterPropertyPage(IBaseFilter *pFilter, HWND hwndParent)
{
HRESULT hr;
ISpecifyPropertyPages *pSpecify=0;

if (!pFilter)
return E_NOINTERFACE;

// Discover if this filter contains a property page
hr = pFilter->QueryInterface(IID_ISpecifyPropertyPages, (void **)&pSpecify);
if (SUCCEEDED(hr))
{
do
{
FILTER_INFO FilterInfo;
hr = pFilter->QueryFilterInfo(&FilterInfo);
if (FAILED(hr))
break;

CAUUID caGUID;
hr = pSpecify->GetPages(&caGUID);
if (FAILED(hr))
break;

// Display the filter's property page
OleCreatePropertyFrame(
hwndParent,             // Parent window
0,                      // x (Reserved)
0,                      // y (Reserved)
FilterInfo.achName,     // Caption for the dialog box
1,                      // Number of filters
(IUnknown **)&pFilter,  // Pointer to the filter
caGUID.cElems,          // Number of property pages
caGUID.pElems,          // Pointer to property page CLSIDs
0,                      // Locale identifier
0,                      // Reserved
NULL                    // Reserved
);

CoTaskMemFree(caGUID.pElems);
FilterInfo.pGraph->Release();

} while(0);
}

// Release interfaces
if (pSpecify)
pSpecify->Release();

pFilter->Release();
return hr;
}

This is an actual function taken out of the DirectShow samples provided by Microsoft. The use of the do...while(0) loop is strange for two reasons:

1. If you're like me and chronically dyslexic, it looks as though the loop never exits because the break points will rarely (if ever) get hit, and
   
2. It will exit, because it's a while(0) loop--which begs the very simple question: WHY?

Why would a programmer do such a strange thing? The removal of the loop causes the function to work exactly the same way, except for one small thing: the programmer didn't use smart pointers, which meant they had to find alternate ways to clean up. So in the event of an error, they "break" out of the loop and clean up after themselves (albeit poorly--the code will leak a reference inside the FILTER_INFO structure if the second break is executed).

If you're trying to find creative ways to clean up after COM pointers, please, just take a look at smart pointers to avoid the above, which is absolute foolishness. A much, much better rewrite of this function is:

HRESULT ShowFilterPropertyPage( IBaseFilter * pFilter, HWND hwndParent )
{
if( !pFilter )
{
return E_POINTER;
}

CComPtr pSpecify;

// Discover if this filter contains a property page
HRESULT hr = pFilter->QueryInterface( IID_ISpecifyPropertyPages, ( void ** ) &pSpecify );
if( SUCCEEDED( hr ) )
{
FILTER_INFO FilterInfo;
hr = pFilter->QueryFilterInfo( &FilterInfo );
if( FAILED( hr ) )
{
return hr;
}

CAUUID caGUID;
hr = pSpecify->GetPages(&caGUID);
if (FAILED(hr))
{
FilterInfo.pGraph->Release();
return hr;
}

// Display the filter's property page
OleCreatePropertyFrame(
hwndParent,             // Parent window
0,                      // x (Reserved)
0,                      // y (Reserved)
FilterInfo.achName,     // Caption for the dialog box
1,                      // Number of filters
(IUnknown **)&pFilter,  // Pointer to the filter
caGUID.cElems,          // Number of property pages
caGUID.pElems,          // Pointer to property page CLSIDs
0,                      // Locale identifier
0,                      // Reserved
NULL                    // Reserved
);

CoTaskMemFree(caGUID.pElems);
FilterInfo.pGraph->Release();
}

return hr;
}

...no strange while loop, and no leaks.

Using a goto Statement
Severity: Potentially Harmful, Poor Style, Deeply Suspect


This is really a variant on the above issue (use of a do...while(0) loop) and not using smart pointers.

Statements like goto should not be used, because they produce unmanageable code. More importantly, people almost exclusively use the goto statement as a means of cleaning up after COM pointers in the event of an error. Something much like so:

HRESULT SomeFunction()
{
IBaseFilter * pSomeFilter;  // a very friendly dumb pointer
HRESULT hr = pSomeFilter.CoCreateInstance( CLSID_SomeFilter );
if( FAILED( hr ) )
goto Exit;

// similar error checking for another couple hundred lines of code...

Exit:
SAFE_RELEASE( pSomeFilter );
return hr;
}

None of this nonsense is necessary with smart pointers. Much to my dismay, Microsoft even has this macro available in the WMFSDK and it is commonly used throughout their samples:

#ifndef GOTO_EXIT_IF_FAILED
#define GOTO_EXIT_IF_FAILED(hr) if(FAILED(hr)) goto Exit;
#endif

...this is employed liberally throughout the WMF samples to help deal with cleaning up errant COM pointers. I've seen this exact macro used in DirectShow code as well; needless to say, this is poor code! Not only does it employ an already suspect goto statement, but it will ultimately produce unmanageable, error prone code that's difficult to support.

Never use a goto statement in your DirectShow code. If you're tempted to use one, ask yourself the following questions:

1. Why would I use a goto statement to clean up my COM pointers?
   
2. Is there something better to use?
   

...and then go read up on smart pointers. Again, if you're having issues cleaning up after COM pointers, the simple, effective solution to that problem is to use Smart Pointers.

Leaking memory or ending up with invalid pointers while using AM_MEDIA_TYPE
Severity: Potentially Fatal


Because of the way AM_MEDIA_TYPE has been defined, it is very easy to leak memory using it. This problem is surprisingly pervasive, with examples of potential leaks riddled throughout the SDK samples.

First, let's look at how AM_MEDIA_TYPE is definied:

typedef struct  _MediaType
{
GUID      majortype;
GUID      subtype;
BOOL      bFixedSizeSamples;
BOOL      bTemporalCompression;
ULONG    lSampleSize;
GUID      formattype;
IUnknown * pUnk;
ULONG    cbFormat;
BYTE *    pbFormat;
} AM_MEDIA_TYPE;

AM_MEDIA_TYPE is a structure used to define the format of a given audio or video type. Most of the variables are pretty self-explanatory. However, note the GUID value "formattype"; this variable defines a type that is subsequently represented with another structure. This "other" structure is referenced as a BYTE pointer, pbFormat, and a variable to specify the length of the structure, cbFormat. For example, pbFormat often points to a VIDEOINFOHEADER or WAVEFORMATEX structure.

pbFormat can, in theory, point to just about anything, and of just about any length. This can lead to some pretty elaborate memory leaks. For example, this code excerpt is from the contrast filter sample:

AM_MEDIA_TYPE * pAdjustedType = NULL;
pMediaSample->GetMediaType(&pAdjustedType);

if(pAdjustedType != NULL)
{
if(CheckInputType(&CMediaType(*pAdjustedType)) == S_OK)
{
m_pInput->CurrentMediaType() = *pAdjustedType;
CoTaskMemFree(pAdjustedType);
}
else
{
CoTaskMemFree(pAdjustedType);
return E_FAIL;
}
}

The above code looks fine--the AM_MEDIA_TYPE pointer is created using GetMediaType, and if created, should be destroyed using CoTaskMemFree().

Sadly, this is not the case--the above code will leak sizeof(cbFormat) bytes with every AM_MEDIA_TYPE that is successfully created. The reason for this is GetMediaType returns a full AM_MEDIA_TYPE structure, and internally will call CoTaskMemAlloc to allocate pbFormat--calling CoTaskMemFree merely deletes the AM_MEDIA_TYPE structure, and will not clean up the associated pbFormat structure. To delete this format block, DeleteMediaType should be called. DeleteMediaType deletes both the AM_MEDIA_TYPE structure and also the pbFormat structure. For reference, it looks like so:

void WINAPI DeleteMediaType(AM_MEDIA_TYPE *pmt)
{
// allow NULL pointers for coding simplicity

if (pmt == NULL)
{
return;
}

FreeMediaType(*pmt);
CoTaskMemFree((PVOID)pmt);
}

// DeleteMediaType calls FreeMediaType, although FreeMediaType can
// be used to only delete the pbFormat structure for an AM_MEDIA_TYPE
void WINAPI FreeMediaType(AM_MEDIA_TYPE& mt)
{
if (mt.cbFormat != 0)
{
CoTaskMemFree((PVOID)mt.pbFormat);

// Strictly unnecessary but tidier
mt.cbFormat = 0;
mt.pbFormat = NULL;
}

if (mt.pUnk != NULL)
{
mt.pUnk->Release();
mt.pUnk = NULL;
}
}

...DeleteMediaType calls FreeMediaType (which deletes pbFormat) and then calls CoTaskMemFree, which deletes the rest of the AM_MEDIA_TYPE structure.

Another major problem is using the assignment operator between two AM_MEDIA_TYPE structures. The problem with this is that it will only copy the value of the pbFormat pointer, and not the data it actually points to. Should the right hand argument be deleted correctly (although assuming someone is trying to use the "=" operator between AM_MEDIA_TYPE, they might not be deleting it correctly anyways), then the pointer in the copy will be null, which can result in some rather strange behavior when DirectShow is asked to handle a bogus pbFormat block.

A better solution to all of this is to avoid the use of AM_MEDIA_TYPE whenever possible, and instead use CMediaType. CMediaType is a class derived from an AM_MEDIA_TYPE structure that includes methods in the destructor that make it much more difficult to leak memory. It also overloads the assignment operators, so in most instances, it will copy the pbFormat block automatically. Basically, it provides a lot of protection against leaking memory and having bogus pointers.

Mixing AM_MEDIA_TYPE and CMediaType can still be nasty, however:

STDMETHODIMP SomeClass::GetAMMediaType( AM_MEDIA_TYPE * pAMMEDIATYPE )
{
*pAMMEDIATYPE = *m_pCMediaType;
return S_OK;
}

The above will work, but only so long as m_pCMediaType is a valid CMediaType structure. If m_pCMediaType is destroyed, the pointer in pAMMEDIATYPE is no longer valid, because it merely copied the value of pbFormat and not the contents. It would be much better to do this:

STDMETHODIMP SomeClass::GetAMMediaType( CMediaType * pCMediaType )
{
*pCMediaType = *m_pCMediaType;
return S_OK;
}

...now the format block will be copied automatically, and because the type here is CMediaType, deallocation is managed automatically.

Not listening for and responding to DirectShow events
Severity: Potentially Harmful, Deeply Suspect

DirectShow provides a mechanism for listening and responding to events from the underlying filter graph. Via the IMediaEvent and IMediaEventEx interfaces, an application can listen for events, many of which are highly useful and/or informative.

Not all of the events sent by the underlying filter graph and the filter graph manager are useful, but in many instances, failures in the baseclasses or the underlying graph are communicated exclusively by the DirectShow eventing. The underlying implication here is pretty straightforward. If an application isn't listening for events, those failures:

1. Fall on deaf ears--the application doesn't know something has gone wrong, and:
   
2. Are almost always impossible to debug after the fact, because the specific HRESULT failure returned in the event interface, as well as the specific event, are long gone. Had the events been received, the application can potentially recover from the error, and also log the problem for future troubleshooting.
   

To list one example, I troubleshot a DirectShow application with several custom filters, including a custom source filter derived from CSource and CSourceStream. The source filter was logging errors with a custom logging interface. In one run of the application, the Deliver() call in the overridden DoBufferProcessingLoop returned 0x8004020B (VFW_E_RUNTIME_ERROR: A run-time error occurred. How informative!). Going through the baseclasses, I quickly noticed that this error was commonly associated with render and transform filters, and whenever it occured, it would send EC_ERRORABORT along with an HRESULT detailing the failure. Some filter downstream from the source filter had encountered a runtime error, and was communicating it both via a failure to the Deliver call, and a much more informative event to the controlling application.

Because the application was not listening for events, it had no idea that a real issue had even occured. The source filter continued chugging away, for hours, doing almost nothing (well, nothing besides logging ridiculous amounts of VFW_E_RUNTIME_ERROR failures) and oblivious to the fact that a serious error had occured. To add insult to injury, because the event was not received (and thus no HRESULT was available for further troubleshooting), it's almost impossible to know "why" or "where" the problem even occured. It could have been in any of the several downstream filters, and for any number of reasons.

Mitigating this is very, very easy:

• Make sure that your application listens for events.
  
• Make sure it responds to events.
  
• Make sure you log extranious events for post-run analysis. Not doing so will result in erratic behavior and strange failures that are impossible to isolate after they've occured.

Not using DirectShow eventing in custom filters, or using a secondary eventing scheme
Severity: Potentially Harmful, Deeply Suspect

I've seen several custom filters now that implement their own home-baked eventing scheme. This is bad for several reasons:

• DirectShow provides an eventing mechanism, free of charge, that filters can leverage. Not using it means someone is developing something that already exists. Re-inventing the wheel = bad
  
• The DirectShow eventing is asynchronous. Not using asynchronous eventing (which I've seen several instances of now) means the possibility of very elaborate deadlocks becomes much greater.
  
• An application can consolidate two eventing interfaces into one. Really, why make things difficult? Multithreading is complicated as it is; having redundant eventing handlers (e.g.: the standard DShow eventing, as well as some other custom eventing) is literally begging for a few extra late nights at the office.
  

To elaborate on all of the above:

DirectShow filters can leverage the IMediaEventSink interface, which allows filters to notify the filter graph manager of events via the Notify() method. Applications should not use this interface; it is specifically for filters. Filters may also define custom events with the range of EC_USER and higher, with a few relatively minor caveats:

• The Filter Graph Manager cannot free the event parameters using the normal IMediaEvent::FreeEventParams method. The application must free any memory or reference counts associated with the event parameters.
  
• The filter should only send the event from within an application that is prepared to handle the event. (Possibly the application can set a custom property on the filter to indicate that it is safe to send the event.)

That being said, there is a very comprehensive listing of events, and most filters would probably not need to define custom events.

The other really helpful thing about using IMediaEventSink::Notify is not having to worry about sending off events synchronously. Sending a synchronous event from the CSourceStream's worker thread that causes the graph to get torn down is instantaneous deadlock. DirectShow is heavily threaded; between implementing my own asynchronous eventing service and simply leveraging the standard one provided to me, the choice becomes rather obvious.

Lastly, the calling application can now centralize graph handling in one routine. There are not multiple interfaces to reconcile, and assuming the second eventing interface is asynchronous (like it should be), no synchronization issues between the two handlers.

Make life easy: always use the standard DirectShow eventing in custom filters unless there is absolutely a need to do otherwise. Quite frankly, I can't think of an instance where the need would be absolute, but always and never are two words to always remember to never use.


Registering Filters with the Operating System
Severity: Potentially Harmful, Deeply Suspect

First off, I hesitate to list this defect. After all, I am essentially telling you not to do what the documentation tells you to do. But after supporting a production application for the last several years, I am of the opinion that anywhere you are using Intelligent Connect, your application will (sooner or later) fail in the wild. By using this feature of DirectShow, you are exposed to any arbitrary filter (or filters) the user installs (coughffdshowcough). Other downsides to filter registration include people being able to re-use your filters, making your application easier to reverse-engineer, and exposing settings in the registry that can be modified by third-party applications. You cannot count on filter merit to save you.

Simply put, you're asking for trouble. In the case of my application, it took a year or two, but one day a support case came in where the application was crashing. The source? I had forgotten to change a small piece of the application to use ConnectDirect() rather than Connect(), which resulted in my filter graph picking up some rogue filter. I was crashing inside someone else's code. I had previously removed all of the Connect() calls, since those had resulted in crashing in the wild via the same mechanism, but I missed one spot when I added a new feature to the application. And surprisingly, it caught up with me.

One other substantial pain-point with respect to registering filters is registration typically happens at install-time, so it's one more thing that can potentially cause the installer to fail. DirectShow filters often end up with numerous dependancies (many of my filters have dependancies on several other runtimes), and if any of them are unfulfilled, the filter will not register, resulting in a failed installer or error messages the user doesn't understand. Sometimes all that needs to happen is the filters need to be re-registered after the installer has run, but if you avoid the registry, you avoid this problem completely.

The solution? Do not register your filters. Simply instantiate them with new.

(addendum: this advice doesn't apply if you are in fact attempting to integrate with existing applications. If your filter is specific to your application, then consider not registering and using ConnectDirect() to manually build your graph)


Not using IFileSourceFilter in Custom Filters
Severity: Poor Style, Deeply Suspect

IFileSourceFilter is an interface "...to set the file name and media type of the media file that they are to render." This may sound like a minor interface, but the advantages of using IFileSourceFilter are huge. First of all, it is the standard interface for configuring source filters, so suddenly any code that uses IFileSourceFilter will be capable of loading up your filter. Second, testing your filters in applications like GraphEdit becomes trivial. Lastly, should you want to register a custom protocol or file type and associate it with your filter, you must implement IFileSourceFilter.

Do you have to implement IFileSourceFilter? No, of course not. But, without this interface, your filter is largely incompatible with most existing DirectShow applications, and people using your filter will have to do a lot of extra work to make it all happen. Your filter is also much more difficult to test because you can't fire it up in GraphEdit. And it also precludes you from using your filter in Windows Media Player, Windows Media Center, and a whole host of other applications that depend on this standard interface. In general, source filters should always implement IFileSourceFilter.