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

[Zebediah Figura]

On 6/1/20 11:13 PM, Anton Baskanov wrote:
> Signed-off-by: Anton Baskanov
>
> 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().