Re: [PATCH 4/5] amstream: Reconnect if the new format is incompatible with the connection media type in AMDirectDrawStream::SetFormat.

[Anton Baskanov]

On Sunday, 30 August 2020 03:52:02 +07 you wrote:
> On 8/29/20 8:51 AM, Anton Baskanov wrote:
> > Signed-off-by: Anton Baskanov
> >
> > dlls/amstream/ddrawstream.c    |  37 +++++++++++
> >  dlls/amstream/tests/amstream.c | 116 ++++++++++++++++++++++++++++++++-
> >  2 files changed, 152 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/amstream/ddrawstream.c b/dlls/amstream/ddrawstream.c
> > index dcb36d0028f..5dc29bed033 100644
> > --- a/dlls/amstream/ddrawstream.c
> > +++ b/dlls/amstream/ddrawstream.c
> > @@ -43,6 +43,7 @@ struct ddraw_stream
> >  IDirectDraw *ddraw;
> >  CRITICAL_SECTION cs;
> >  IMediaStreamFilter *filter;
> >
> > -
> >
> > IFilterGraph *graph;
> >
> > IPin *peer;
> > IMemAllocator *allocator;
> >
> > @@ -304,6 +305,8 @@ static HRESULT WINAPI
> > ddraw_IAMMediaStream_JoinFilterGraph(IAMMediaStream *iface> 
> >      TRACE("stream %p, filtergraph %p.\n", stream, filtergraph);
> > -
> >
> > stream->graph = filtergraph;
> > -
> >
> > return S_OK;
> >
> > }
> >
> > @@ -442,6 +445,10 @@ static HRESULT WINAPI
> > ddraw_IDirectDrawMediaStream_SetFormat(IDirectDrawMediaStr> 
> >          const DDSURFACEDESC *format, IDirectDrawPalette *palette)
> >
> > {
> >  struct ddraw_stream *stream =
> >  impl_from_IDirectDrawMediaStream(iface);
> >
> > -
> >
> > AM_MEDIA_TYPE old_media_type;
> > -
> >
> > DDSURFACEDESC old_format;
> > -
> >
> > IPin *old_peer;
> > -
> >
> > HRESULT hr;
> >
> > TRACE("stream %p, format %p, palette %p.\n", stream, format,
> > palette);
> >
> > @@ -496,6 +503,7 @@ static HRESULT WINAPI
> > ddraw_IDirectDrawMediaStream_SetFormat(IDirectDrawMediaStr> 
> >      EnterCriticalSection(&stream->cs);
> > -
> >
> > old_format = stream->format;
> >
> > stream->format.dwFlags = DDSD_CAPS | (format->dwFlags & (DDSD_WIDTH |
> > DDSD_HEIGHT | DDSD_PIXELFORMAT)); if (format->dwFlags & DDSD_WIDTH)
> > stream->format.dwWidth = format->dwWidth;
> >
> > @@ -504,6 +512,35 @@ static HRESULT WINAPI
> > ddraw_IDirectDrawMediaStream_SetFormat(IDirectDrawMediaStr> 
> >      if (format->dwFlags & DDSD_PIXELFORMAT)
> >      stream->format.ddpfPixelFormat = format->ddpfPixelFormat;
> >
> > - if (stream->peer && !is_media_type_compatible(&stream->mt,
> >
> > &stream->format)) +    {
> > -
> >    hr = CopyMediaType(&old_media_type, &stream->mt);
> > -
> >    if (FAILED(hr))
> > -
> >    {
> > -
> >        stream->format = old_format;
> > -
> >        LeaveCriticalSection(&stream->cs);
> > -
> >        return hr;
> > -
> >    }
> > -
> >    old_peer = stream->peer;
> > -
> >    IPin_AddRef(old_peer);
> > -
> > -
> >    IFilterGraph_Disconnect(stream->graph, stream->peer);
> > -
> >    IFilterGraph_Disconnect(stream->graph, &stream->IPin_iface);
> > -
> >    hr = IFilterGraph_ConnectDirect(stream->graph, old_peer,
> >
> > &stream->IPin_iface, NULL); +        if (FAILED(hr))
> > -
> >    {
> > -
> >        stream->format = old_format;
> > -
> >        IFilterGraph_ConnectDirect(stream->graph, old_peer,
> >
> > &stream->IPin_iface, &old_media_type); +           
> > IPin_Release(old_peer);
> > -
> >        FreeMediaType(&old_media_type);
> > -
> >        LeaveCriticalSection(&stream->cs);
> > -
> >        return DDERR_INVALIDSURFACETYPE;
> > -
> >    }
>
> Falling back to the old connection type is an interesting behaviour,
> which, if correct, could explain the difference between the "current"
> and "desired" formats in GetFormat. That said, I'm probably just blind,
> but I don't see tests for this behaviour in this patch.

There is a test that tries to change the stream format with no media types 
advertised by the test source. I've added checks for current and desired 
formats to it and the formats seem to always be the same.
> > -
> > -
> >    IPin_Release(old_peer);
> > -
> >    FreeMediaType(&old_media_type);
> > -
> >
> > }
> > -
> >
> > LeaveCriticalSection(&stream->cs);
> >
> > return S_OK;
> >
> > diff --git a/dlls/amstream/tests/amstream.c
> > b/dlls/amstream/tests/amstream.c index fe1cca4036c..7c02bd223d4 100644
> > --- a/dlls/amstream/tests/amstream.c
> > +++ b/dlls/amstream/tests/amstream.c
> > @@ -1005,6 +1005,7 @@ struct testfilter
> >  IMediaSeeking IMediaSeeking_iface;
> >  LONGLONG current_position;
> >  LONGLONG stop_position;
> >
> > -
> >
> > const AM_MEDIA_TYPE *preferred_mt;
> >
> > HRESULT get_duration_hr;
> > HRESULT get_stop_position_hr;
> > HRESULT set_positions_hr;
> >
> > @@ -1065,6 +1066,16 @@ static inline struct testfilter
> > *impl_from_base_pin(struct strmbase_pin *iface)> 
> >      return CONTAINING_RECORD(iface, struct testfilter, source.pin);
> >
> > }
> >
> > +static HRESULT testsource_get_media_type(struct strmbase_pin *iface,
> > unsigned int index, AM_MEDIA_TYPE *mt) +{
> > - struct testfilter *filter = impl_from_base_pin(iface);
> > -
> > - if (index < 1 && filter->preferred_mt)
> > -
> >    return CopyMediaType(mt, filter->preferred_mt);
> > -
> > - return VFW_S_NO_MORE_ITEMS;
> >
> > +}
> > -
> >
> > static HRESULT testsource_query_interface(struct strmbase_pin *iface,
> >  REFIID iid, void **out) {
> >  struct testfilter *filter = impl_from_base_pin(iface);
> >
> > @@ -1079,6 +1090,25 @@ static HRESULT testsource_query_interface(struct
> > strmbase_pin *iface, REFIID iid> 
> >      return S_OK;
> >
> > }
> >
> > +static HRESULT WINAPI testsource_DecideAllocator(struct strmbase_source
> > *iface, IMemInputPin *pin, IMemAllocator **alloc) +{
> > - ALLOCATOR_PROPERTIES props = {0};
> > - HRESULT hr;
> > -
> > - /* AMDirectDrawStream tries to use it's custom allocator and
> > -
> > * when it is able to do so it's behavior changes slightly
> > -
> > * (e.g. it uses dynamic format change instead of reconnecting in
> >
> > SetFormat). +     * We don't yet implement the custom allocator so force
> > the standard one for now. */ +    hr =
> > BaseOutputPinImpl_InitAllocator(iface, alloc);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - IMemInputPin_GetAllocatorRequirements(pin, &props);
> > - hr = iface->pFuncsTable->pfnDecideBufferSize(iface, *alloc, &props);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - return IMemInputPin_NotifyAllocator(pin, *alloc, FALSE);
> >
> > +}
> > -
> >
> > static HRESULT WINAPI testsource_DecideBufferSize(struct strmbase_source
> >  *iface,>  
> >          IMemAllocator *alloc, ALLOCATOR_PROPERTIES *requested)
> >
> > {
> >
> > @@ -1098,10 +1128,11 @@ static HRESULT WINAPI
> > testsource_DecideBufferSize(struct strmbase_source *iface,> 
> >  static const struct strmbase_source_ops testsource_ops =
> >  {
> > -
> >
> > .base.pin_get_media_type = testsource_get_media_type,
> >
> > .base.pin_query_interface = testsource_query_interface,
> > .pfnAttemptConnection = BaseOutputPinImpl_AttemptConnection,
> > .pfnDecideBufferSize = testsource_DecideBufferSize,
> > - .pfnDecideAllocator = BaseOutputPinImpl_DecideAllocator,
> > - .pfnDecideAllocator = testsource_DecideAllocator,
> >
> > };
> >
> > static void testfilter_init(struct testfilter *filter)
> >
> > @@ -4985,10 +5016,15 @@ static void test_ddrawstream_set_format(void)
> >  IDirectDrawMediaStream *ddraw_stream;
> >  IAMMultiMediaStream *mmstream;
> >
> > -
> >
> > struct testfilter source;
> > -
> >
> > IGraphBuilder *graph;
> >
> > DDSURFACEDESC format;
> > IMediaStream *stream;
> > -
> >
> > VIDEOINFO video_info;
> > -
> >
> > AM_MEDIA_TYPE mt;
> >
> > HRESULT hr;
> > ULONG ref;
> > -
> >
> > IPin *pin;
> >
> > mmstream = create_ammultimediastream();
> >
> > @@ -5062,6 +5098,84 @@ static void test_ddrawstream_set_format(void)
> >  ref = IMediaStream_Release(stream);
> >  ok(!ref, "Got outstanding refcount %d.\n", ref);
> >
> > - mmstream = create_ammultimediastream();
> > -
> > - hr = IAMMultiMediaStream_AddMediaStream(mmstream, NULL,
> >
> > &MSPID_PrimaryVideo, 0, &stream); +    ok(hr == S_OK, "Got hr %#x.\n",
> > hr);
> > - hr = IMediaStream_QueryInterface(stream, &IID_IDirectDrawMediaStream,
> >
> > (void **)&ddraw_stream); +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - hr = IMediaStream_QueryInterface(stream, &IID_IPin, (void **)&pin);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - hr = IAMMultiMediaStream_GetFilterGraph(mmstream, &graph);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - ok(!!graph, "Expected non-NULL graph.\n");
> > -
> > - testfilter_init(&source);
> > -
> > - hr = IGraphBuilder_AddFilter(graph, &source.filter.IBaseFilter_iface,
> >
> > L"source"); +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - hr = IGraphBuilder_ConnectDirect(graph,
> >
> > &source.source.pin.IPin_iface, pin, &rgb8_mt); +    ok(hr == S_OK, "Got
> > hr %#x.\n", hr);
> > -
> > - source.preferred_mt = NULL;
> > -
> > - hr = IDirectDrawMediaStream_SetFormat(ddraw_stream, &rgb555_format,
> >
> > NULL); +    ok(hr == DDERR_INVALIDSURFACETYPE, "Got hr %#x.\n", hr);
> > - ok(IsEqualGUID(&source.source.pin.mt.subtype, &MEDIASUBTYPE_RGB8),
> > -
> >        "Got subtype %s.\n",
> >
> > wine_dbgstr_guid(&source.source.pin.mt.subtype)); +
> > - format = rgb555_format;
> > - format.dwFlags = 0;
> > - hr = IDirectDrawMediaStream_SetFormat(ddraw_stream, &format, NULL);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - ok(IsEqualGUID(&source.source.pin.mt.subtype, &MEDIASUBTYPE_RGB8),
> > -
> >        "Got subtype %s.\n",
> >
> > wine_dbgstr_guid(&source.source.pin.mt.subtype)); +
> > - source.preferred_mt = &rgb555_mt;
> > -
> > - hr = IDirectDrawMediaStream_SetFormat(ddraw_stream, &rgb8_format,
> >
> > NULL); +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - hr = IDirectDrawMediaStream_SetFormat(ddraw_stream, &rgb555_format,
> >
> > NULL); +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - ok(IsEqualGUID(&source.source.pin.mt.subtype, &MEDIASUBTYPE_RGB555),
> > -
> >        "Got subtype %s.\n",
> >
> > wine_dbgstr_guid(&source.source.pin.mt.subtype)); +
> > - video_info = rgb555_video_info;
> > - video_info.bmiHeader.biWidth = 222;
> > - video_info.bmiHeader.biHeight = -555;
> > - mt = rgb555_mt;
> > - mt.pbFormat = (BYTE *)&video_info;
> > - source.preferred_mt = &mt;
> > -
> > - format = rgb555_format;
> > - format.dwFlags |= DDSD_WIDTH | DDSD_HEIGHT;
> > - format.dwWidth = 222;
> > - format.dwHeight = 555;
> > - hr = IDirectDrawMediaStream_SetFormat(ddraw_stream, &format, NULL);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - ok(IsEqualGUID(&source.source.pin.mt.subtype, &MEDIASUBTYPE_RGB555),
> > -
> >        "Got subtype %s.\n",
> >
> > wine_dbgstr_guid(&source.source.pin.mt.subtype)); +    ok(((VIDEOINFO
> > *)source.source.pin.mt.pbFormat)->bmiHeader.biWidth == 222, +           
> > "Got width %d.\n", ((VIDEOINFO
> > *)source.source.pin.mt.pbFormat)->bmiHeader.biWidth); +    ok(((VIDEOINFO
> > *)source.source.pin.mt.pbFormat)->bmiHeader.biHeight == -555, +          
> >  "Got height %d.\n", ((VIDEOINFO
> > *)source.source.pin.mt.pbFormat)->bmiHeader.biHeight); +
> > - hr = IGraphBuilder_Disconnect(graph, &source.source.pin.IPin_iface);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - hr = IGraphBuilder_Disconnect(graph, pin);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - ref = IAMMultiMediaStream_Release(mmstream);
> > - ok(!ref, "Got outstanding refcount %d.\n", ref);
> > - ref = IGraphBuilder_Release(graph);
> > - ok(!ref, "Got outstanding refcount %d.\n", ref);
> > - IPin_Release(pin);
> > - IDirectDrawMediaStream_Release(ddraw_stream);
> > - ref = IMediaStream_Release(stream);
> > - ok(!ref, "Got outstanding refcount %d.\n", ref);
> >
> > }
> >
> > static void check_ammediastream_join_am_multi_media_stream(const CLSID
> >  *clsid)