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

[Andrew Eikum]

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.

Is this something you could write a test for? E.g. create the filter
and then query for its output pin immediately after creation.

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(a)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.
> 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?
> 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?
> /* IAsyncReader */
>
> static HRESULT WINAPI FileAsyncReader_QueryInterface(IAsyncReader * iface, REFIID riid, LPVOID * ppv)
> -- 
> 2.11.0

[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(a)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