[PATCH 1/4] quartz: AsyncReader is always expected to have output pin

[Miklós Máté]

On 31/05/17 16:32, Andrew Eikum wrote:
> Thanks for working on this.  I tested your patch sequence against a
> number of games as well as embedding videos in MSOffice 2007 and 2010
> and found no regressions. I do have some comments, though, in-line
> below.
Hi,

Thanks for the review.

>
> Is this something you could write a test for? E.g. create the filter
> and then query for its output pin immediately after creation.
Certainly.
>
> On Thu, May 25, 2017 at 04:54:47PM +0200, Miklós Máté wrote:
>> Even between create() and Load(). This fixes a crash in DSPlayer.
>>
>> Signed-off-by: Miklós Máté <mtmkls at gmail.com>
>> ---
>>   dlls/quartz/filesource.c | 31 ++++++++++++++++++++-----------
>>   1 file changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/dlls/quartz/filesource.c b/dlls/quartz/filesource.c
>> index ed7e5ee201..200463ff5b 100644
>> --- a/dlls/quartz/filesource.c
>> +++ b/dlls/quartz/filesource.c
>> @@ -73,7 +73,8 @@ static const IFileSourceFilterVtbl FileSource_Vtbl;
>>   static const IAsyncReaderVtbl FileAsyncReader_Vtbl;
>>   static const IAMFilterMiscFlagsVtbl IAMFilterMiscFlags_Vtbl;
>>   
>> -static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin);
>> +static HRESULT FileAsyncReader_Construct(IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin);
>> +static HRESULT FileAsyncReader_SetFile(IAsyncReader * iface, HANDLE hFile);
>>   
>>   static const WCHAR mediatype_name[] = {
>>       'M', 'e', 'd', 'i', 'a', ' ', 'T', 'y', 'p', 'e', 0 };
>> @@ -416,6 +417,7 @@ static const BaseFilterFuncTable BaseFuncTable = {
>>   HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv)
>>   {
>>       AsyncReader *pAsyncRead;
>> +    HRESULT hr;
>>       
>>       if( pUnkOuter )
>>           return CLASS_E_NOAGGREGATION;
>> @@ -429,7 +431,8 @@ HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv)
>>   
>>       pAsyncRead->IFileSourceFilter_iface.lpVtbl = &FileSource_Vtbl;
>>       pAsyncRead->IAMFilterMiscFlags_iface.lpVtbl = &IAMFilterMiscFlags_Vtbl;
>> -    pAsyncRead->pOutputPin = NULL;
>> +    hr = FileAsyncReader_Construct(&pAsyncRead->filter.IBaseFilter_iface, &pAsyncRead->filter.csFilter, &pAsyncRead->pOutputPin);
>> +    BaseFilterImpl_IncrementPinVersion(&pAsyncRead->filter);
> If this fails, I think we should free resources and return the error
> immediately.
Yes, you're right.
>
>>   
>>       pAsyncRead->pszFileName = NULL;
>>       pAsyncRead->pmt = NULL;
>> @@ -438,7 +441,7 @@ HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv)
>>   
>>       TRACE("-- created at %p\n", pAsyncRead);
>>   
>> -    return S_OK;
>> +    return hr;
>>   }
>>   
>>   /** IUnknown methods **/
>> @@ -606,7 +609,7 @@ static ULONG WINAPI FileSource_Release(IFileSourceFilter * iface)
>>   
>>   static HRESULT WINAPI FileSource_Load(IFileSourceFilter * iface, LPCOLESTR pszFileName, const AM_MEDIA_TYPE * pmt)
>>   {
>> -    HRESULT hr;
>> +    HRESULT hr = E_FAIL;
>>       HANDLE hFile;
>>       IAsyncReader * pReader = NULL;
>>       AsyncReader *This = impl_from_IFileSourceFilter(iface);
>> @@ -625,16 +628,15 @@ static HRESULT WINAPI FileSource_Load(IFileSourceFilter * iface, LPCOLESTR pszFi
>>           return HRESULT_FROM_WIN32(GetLastError());
>>       }
>>   
>> -    /* create pin */
>> -    hr = FileAsyncReader_Construct(hFile, &This->filter.IBaseFilter_iface, &This->filter.csFilter, &This->pOutputPin);
>> -    BaseFilterImpl_IncrementPinVersion(&This->filter);
>>   
>> -    if (SUCCEEDED(hr))
>> +    if (This->pOutputPin)
> Since we now create the output pin during filter creation, this should
> never be non-NULL, right?
I wasn't entirely sure that the pin can't get deleted somehow.
>
>>           hr = IPin_QueryInterface(This->pOutputPin, &IID_IAsyncReader, (LPVOID *)&pReader);
>>   
>>       /* store file name & media type */
>>       if (SUCCEEDED(hr))
>>       {
>> +        FileAsyncReader_SetFile(pReader, hFile);
>> +
>>           CoTaskMemFree(This->pszFileName);
>>           if (This->pmt)
>>               FreeMediaType(This->pmt);
>> @@ -939,7 +941,7 @@ static const BaseOutputPinFuncTable output_BaseOutputFuncTable = {
>>       BaseOutputPinImpl_BreakConnect
>>   };
>>   
>> -static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin)
>> +static HRESULT FileAsyncReader_Construct(IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin)
>>   {
>>       PIN_INFO piOutput;
>>       HRESULT hr;
>> @@ -952,9 +954,8 @@ static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter
>>   
>>       if (SUCCEEDED(hr))
>>       {
>> -        FileAsyncReader *pPinImpl =  (FileAsyncReader *)*ppPin;
>> +        FileAsyncReader *pPinImpl =  impl_from_IPin(*ppPin);
>>           pPinImpl->IAsyncReader_iface.lpVtbl = &FileAsyncReader_Vtbl;
>> -        pPinImpl->hFile = hFile;
>>           pPinImpl->bFlushing = FALSE;
>>           pPinImpl->sample_list = NULL;
>>           pPinImpl->handle_list = NULL;
>> @@ -965,6 +966,14 @@ static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter
>>       return hr;
>>   }
>>   
>> +static HRESULT FileAsyncReader_SetFile(IAsyncReader * iface, HANDLE hFile)
>> +{
>> +    FileAsyncReader *This = impl_from_IAsyncReader(iface);
>> +
>> +    This->hFile = hFile;
>> +    return S_OK;
>> +}
>> +
> Why is this a separate function instead of just setting hFile in
> FileSource_Load?
Setting the file handler is at line 643, but struct FileAsyncReader is 
defined on line 745. I didn't want to rearrange the file for this.

MM
>
>>   /* IAsyncReader */
>>   
>>   static HRESULT WINAPI FileAsyncReader_QueryInterface(IAsyncReader * iface, REFIID riid, LPVOID * ppv)
>> -- 
>> 2.11.0
>>
>>
>>