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

[Zebediah Figura]

On 6/3/20 11:36 AM, Anton Baskanov wrote:
> On Tuesday, 2 June 2020 23:04:00 +07 you wrote:
>> 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.
> 
> ::SetPosition tests show that only one stream is used for seeking and that
> ::SupportSeeking determines which one will be used. Probably it's assumed that
> all the streams come from the same splitter filter.
> 
> 

Ah yes, I see you're right; I clearly didn't read closely enough.

I guess it makes sense anyway, after all, in the case of amstream 
everything's expected to go through OpenFile anyway.