[PATCH 3/5] amstream: Implement MediaStreamFilter::SupportSeeking.

[Zebediah Figura]

On 6/2/20 10:54 AM, Zebediah Figura wrote:
> On 6/1/20 11:13 PM, Anton Baskanov wrote:
>> Signed-off-by: Anton Baskanov <baskanov at gmail.com>
>> ---
>>  dlls/amstream/filter.c         | 249 ++++++++++++++++++++++++-
>>  dlls/amstream/tests/amstream.c | 323 +++++++++++++++++++++++++++++++++
>>  2 files changed, 568 insertions(+), 4 deletions(-)
>>
>> diff --git a/dlls/amstream/filter.c b/dlls/amstream/filter.c
>> index 4b5e184f04..643cfcdbe4 100644
>> --- a/dlls/amstream/filter.c
>> +++ b/dlls/amstream/filter.c
>> @@ -163,6 +163,7 @@ static const IEnumPinsVtbl enum_pins_vtbl =
>>  struct filter
>>  {
>>      IMediaStreamFilter IMediaStreamFilter_iface;
>> +    IMediaSeeking IMediaSeeking_iface;
>>      LONG refcount;
>>      CRITICAL_SECTION cs;
>>  
>> @@ -171,6 +172,7 @@ struct filter
>>      IFilterGraph *graph;
>>      ULONG nb_streams;
>>      IAMMediaStream **streams;
>> +    IAMMediaStream *seekable_stream;
>>      FILTER_STATE state;
>>  };
>>  
>> @@ -181,6 +183,8 @@ static inline struct filter *impl_from_IMediaStreamFilter(IMediaStreamFilter *if
>>  
>>  static HRESULT WINAPI filter_QueryInterface(IMediaStreamFilter *iface, REFIID riid, void **ret_iface)
>>  {
>> +    struct filter *filter = impl_from_IMediaStreamFilter(iface);
>> +
>>      TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), ret_iface);
>>  
>>      *ret_iface = NULL;
>> @@ -191,10 +195,12 @@ static HRESULT WINAPI filter_QueryInterface(IMediaStreamFilter *iface, REFIID ri
>>          IsEqualIID(riid, &IID_IBaseFilter) ||
>>          IsEqualIID(riid, &IID_IMediaStreamFilter))
>>          *ret_iface = iface;
>> +    else if (IsEqualIID(riid, &IID_IMediaSeeking) && filter->seekable_stream)
>> +        *ret_iface = &filter->IMediaSeeking_iface;
>>  
>>      if (*ret_iface)
>>      {
>> -        IMediaStreamFilter_AddRef(*ret_iface);
>> +        IUnknown_AddRef((IUnknown *)*ret_iface);
>>          return S_OK;
>>      }
>>  
>> @@ -544,11 +550,74 @@ static HRESULT WINAPI filter_EnumMediaStreams(IMediaStreamFilter *iface, LONG in
>>      return S_OK;
>>  }
>>  
>> -static HRESULT WINAPI filter_SupportSeeking(IMediaStreamFilter *iface, BOOL bRenderer)
>> +static IMediaSeeking *get_seeking(IAMMediaStream *stream)
>>  {
>> -    FIXME("(%p)->(%d): Stub!\n", iface, bRenderer);
>> +    IPin *pin;
>> +    IPin *peer;
>> +    IMediaSeeking *seeking;
>>  
>> -    return E_NOTIMPL;
>> +    if (FAILED(IAMMediaStream_QueryInterface(stream, &IID_IPin, (void **)&pin)))
>> +    {
>> +        WARN("Stream %p does not support IPin.\n", stream);
>> +        return NULL;
>> +    }
>> +
>> +    if (FAILED(IPin_ConnectedTo(pin, &peer)))
>> +    {
>> +        IPin_Release(pin);
>> +        return NULL;
>> +    }
>> +
>> +    if (FAILED(IPin_QueryInterface(peer, &IID_IMediaSeeking, (void **)&seeking)))
>> +    {
>> +        IPin_Release(peer);
>> +        IPin_Release(pin);
>> +        return NULL;
>> +    }
>> +
>> +    IPin_Release(peer);
>> +    IPin_Release(pin);
>> +
>> +    return seeking;
>> +}
>> +
>> +static HRESULT WINAPI filter_SupportSeeking(IMediaStreamFilter *iface, BOOL renderer)
>> +{
>> +    struct filter *filter = impl_from_IMediaStreamFilter(iface);
>> +    unsigned int i;
>> +    HRESULT hr = E_NOINTERFACE;
>> +
>> +    TRACE("filter %p, renderer %d\n", iface, renderer);
> 
> Without tests to show whether/how the "renderer" value makes a
> difference, I think we should print a FIXME if it's FALSE.
> 
>> +
>> +    EnterCriticalSection(&filter->cs);
>> +
>> +    if (filter->seekable_stream)
>> +        return HRESULT_FROM_WIN32(ERROR_ALREADY_INITIALIZED);
> 
> Missing LeaveCriticalSection().
> 
>> +
>> +    for (i = 0; i < filter->nb_streams; ++i)
>> +    {
>> +        IMediaSeeking *seeking = get_seeking(filter->streams[i]);
>> +        LONGLONG duration;
>> +
>> +        if (!seeking)
>> +            continue;
>> +
>> +        if (FAILED(IMediaSeeking_GetDuration(seeking, &duration)))
>> +        {
>> +            IMediaSeeking_Release(seeking);
>> +            continue;
>> +        }
>> +
>> +        filter->seekable_stream = filter->streams[i];
>> +        hr = S_OK;
>> +        IMediaSeeking_Release(seeking);
> 
> Is it correct that only one stream is chosen for seeking, even if there
> are multiple available? Consider e.g. the quartz filter graph, which
> seeks all available streams.
> 
> For that matter, should we be delegating to the filter graph instead of
> seeking the pins directly?
> 
> One thing in particular to test is whether filters are paused when
> seeked. Even if we don't delegate to the filter graph, we likely want to
> do this anyway.
> 
> Another thing to test is whether GetCurrentPosition() returns something
> reflecting the reference clock time, or the last sample time, or whether
> it fails if no exposed streams support GetCurrentPosition().
> 

Sorry, on reflection, I realize this doesn't make sense, because we
*are* the sink filter. multimedia stream ≠ stream filter...

I think the question about seeking multiple streams still stands, though.