<div dir="ltr"><div>Hello, thanks for reviewing my patches! I've addressed your comments in the new version.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 24 Feb 2020 14:21:32 -0600 Zebediah Figura wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello Anton, thanks for the patch! I have a few comments inlined below:<br>
<br>
On 2/24/20 12:03 PM, Anton Baskanov wrote:<br>
> Signed-off-by: Anton Baskanov <<a href="mailto:baskanov@gmail.com" target="_blank">baskanov@gmail.com</a>><br>
> ---<br>
>  dlls/amstream/audiostream.c    |   8 +-<br>
>  dlls/amstream/tests/amstream.c | 336 +++++++++++++++++++++++++++++++++<br>
>  2 files changed, 342 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/dlls/amstream/audiostream.c b/dlls/amstream/audiostream.c<br>
> index 2875e1f59b..8a16512dcd 100644<br>
> --- a/dlls/amstream/audiostream.c<br>
> +++ b/dlls/amstream/audiostream.c<br>
> @@ -499,13 +499,17 @@ static HRESULT WINAPI<br>
> audio_IAudioMediaStream_GetFormat(IAudioMediaStream *iface<br>
>  {<br>
>      struct audio_stream *This = impl_from_IAudioMediaStream(iface);<br>
> <br>
> -    FIXME("(%p/%p)->(%p) stub!\n", iface, This, wave_format_current);<br>
> +    TRACE("(%p/%p)->(%p)\n", iface, This, wave_format_current);<br>
> <br>
>      if (!wave_format_current)<br>
>          return E_POINTER;<br>
> <br>
> -    return MS_E_NOSTREAM;<br>
> +    if (!This->peer)<br>
> +        return MS_E_NOSTREAM;<br>
> <br>
> +    *wave_format_current = *(WAVEFORMATEX *)This->mt.pbFormat;<br>
> +<br>
> +    return S_OK;<br>
<br>
I suspect this should be protected by the stream's critical section.<br>
<br>
>  }<br>
> <br>
>  static HRESULT WINAPI<br>
> audio_IAudioMediaStream_SetFormat(IAudioMediaStream *iface, const<br>
> WAVEFORMATEX *wave_format)<br>
> diff --git a/dlls/amstream/tests/amstream.c b/dlls/amstream/tests/amstream.c<br>
> index 6497dde91c..1fb3cc239b 100644<br>
> --- a/dlls/amstream/tests/amstream.c<br>
> +++ b/dlls/amstream/tests/amstream.c<br>
> @@ -2293,6 +2293,340 @@ out_unknown:<br>
>      IUnknown_Release(unknown);<br>
>  }<br>
> <br>
> +static LONG filter_ref_count = 1;<br>
> +<br>
> +static HRESULT WINAPI filter_QueryInterface(IBaseFilter *iface,<br>
> REFIID riid, void **ret_iface)<br>
> +{<br>
> +    if (IsEqualGUID(&IID_IUnknown, riid) || IsEqualGUID(&IID_IPersist, riid) ||<br>
> +        IsEqualGUID(&IID_IMediaFilter, riid) ||<br>
> IsEqualGUID(&IID_IBaseFilter, riid))<br>
> +    {<br>
> +        *ret_iface = iface;<br>
> +        IBaseFilter_AddRef(iface);<br>
> +        return S_OK;<br>
> +    }<br>
> +    return E_NOINTERFACE;<br>
> +}<br>
> +<br>
> +static ULONG WINAPI filter_AddRef(IBaseFilter *iface)<br>
> +{<br>
> +    return InterlockedIncrement(&filter_ref_count);<br>
> +}<br>
> +<br>
> +static ULONG WINAPI filter_Release(IBaseFilter *iface)<br>
> +{<br>
> +    return InterlockedDecrement(&filter_ref_count);<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_GetClassID(IBaseFilter *iface, CLSID *clsid)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_Stop(IBaseFilter *iface)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_Pause(IBaseFilter *iface)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_Run(IBaseFilter *iface, REFERENCE_TIME start)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_GetState(IBaseFilter *iface, DWORD<br>
> timeout, FILTER_STATE *state)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_SetSyncSource(IBaseFilter *iface,<br>
> IReferenceClock *clock)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_GetSyncSource(IBaseFilter *iface,<br>
> IReferenceClock **clock)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_EnumPins(IBaseFilter *iface, IEnumPins<br>
> **enum_pins)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_FindPin(IBaseFilter *iface, const WCHAR<br>
> *id, IPin **out)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_QueryFilterInfo(IBaseFilter *iface,<br>
> FILTER_INFO *info)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_JoinFilterGraph(IBaseFilter *iface,<br>
> +        IFilterGraph *graph, const WCHAR *name)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI filter_QueryVendorInfo(IBaseFilter *iface,<br>
> LPWSTR *vendor_info)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static const IBaseFilterVtbl filter_vtbl =<br>
> +{<br>
> +    filter_QueryInterface,<br>
> +    filter_AddRef,<br>
> +    filter_Release,<br>
> +    filter_GetClassID,<br>
> +    filter_Stop,<br>
> +    filter_Pause,<br>
> +    filter_Run,<br>
> +    filter_GetState,<br>
> +    filter_SetSyncSource,<br>
> +    filter_GetSyncSource,<br>
> +    filter_EnumPins,<br>
> +    filter_FindPin,<br>
> +    filter_QueryFilterInfo,<br>
> +    filter_JoinFilterGraph,<br>
> +    filter_QueryVendorInfo,<br>
> +};<br>
> +<br>
> +static IBaseFilter filter = {&filter_vtbl};<br>
> +<br>
> +static LONG output_pin_ref_count = 1;<br>
> +<br>
> +static HRESULT WINAPI output_pin_QueryInterface(IPin *iface, REFIID<br>
> iid, void **out)<br>
> +{<br>
> +    if (IsEqualGUID(&IID_IUnknown, iid) || IsEqualGUID(&IID_IPin, iid))<br>
> +    {<br>
> +        *out = iface;<br>
> +        IPin_AddRef(iface);<br>
> +        return S_OK;<br>
> +    }<br>
> +    return E_NOINTERFACE;<br>
> +}<br>
> +<br>
> +static ULONG WINAPI output_pin_AddRef(IPin *iface)<br>
> +{<br>
> +    return InterlockedIncrement(&output_pin_ref_count);<br>
> +}<br>
> +<br>
> +static ULONG WINAPI output_pin_Release(IPin *iface)<br>
> +{<br>
> +    return InterlockedDecrement(&output_pin_ref_count);<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_Connect(IPin *iface, IPin *peer,<br>
> const AM_MEDIA_TYPE *mt)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_ReceiveConnection(IPin *iface, IPin<br>
> *peer, const AM_MEDIA_TYPE *mt)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_Disconnect(IPin *iface)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_ConnectedTo(IPin *iface, IPin **peer)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_ConnectionMediaType(IPin *iface,<br>
> AM_MEDIA_TYPE *mt)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_QueryPinInfo(IPin *iface, PIN_INFO *info)<br>
> +{<br>
> +    if (winetest_debug > 1) trace("QueryPinInfo\n");<br>
> +    info->pFilter = &filter;<br>
> +    info->dir = PINDIR_OUTPUT;<br>
> +    lstrcpyW(info->achName, L"output");<br>
> +    IBaseFilter_AddRef(info->pFilter);<br>
> +    return S_OK;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_QueryDirection(IPin *iface,<br>
> PIN_DIRECTION *dir)<br>
> +{<br>
> +    if (winetest_debug > 1) trace("QueryDirection\n");<br>
> +    *dir = PINDIR_OUTPUT;<br>
> +    return S_OK;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_QueryId(IPin *iface, WCHAR **id)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_QueryAccept(IPin *iface, const<br>
> AM_MEDIA_TYPE *mt)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_EnumMediaTypes(IPin *iface,<br>
> IEnumMediaTypes **out)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_QueryInternalConnections(IPin<br>
> *iface, IPin **pins, ULONG *count)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_EndOfStream(IPin *iface)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_BeginFlush(IPin *iface)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_EndFlush(IPin *iface)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static HRESULT WINAPI output_pin_NewSegment(IPin *iface,<br>
> REFERENCE_TIME start, REFERENCE_TIME stop, double rate)<br>
> +{<br>
> +    ok(0, "Unexpected call.\n");<br>
> +    return E_NOTIMPL;<br>
> +}<br>
> +<br>
> +static const IPinVtbl output_pin_vtbl =<br>
> +{<br>
> +    output_pin_QueryInterface,<br>
> +    output_pin_AddRef,<br>
> +    output_pin_Release,<br>
> +    output_pin_Connect,<br>
> +    output_pin_ReceiveConnection,<br>
> +    output_pin_Disconnect,<br>
> +    output_pin_ConnectedTo,<br>
> +    output_pin_ConnectionMediaType,<br>
> +    output_pin_QueryPinInfo,<br>
> +    output_pin_QueryDirection,<br>
> +    output_pin_QueryId,<br>
> +    output_pin_QueryAccept,<br>
> +    output_pin_EnumMediaTypes,<br>
> +    output_pin_QueryInternalConnections,<br>
> +    output_pin_EndOfStream,<br>
> +    output_pin_BeginFlush,<br>
> +    output_pin_EndFlush,<br>
> +    output_pin_NewSegment,<br>
> +};<br>
> +<br>
> +static IPin output_pin = {&output_pin_vtbl};<br>
<br>
I would suggest using strmbase for this instead of manually implementing<br>
the source pin and filter, similar to how it's done in the quartz<br>
renderer tests.<br>
<br>
> +<br>
> +static void test_audiostream_get_format(void)<br>
> +{<br>
> +    IAMMultiMediaStream *mmstream = create_ammultimediastream();<br>
> +    IMediaStream *stream = NULL;<br>
> +    IAudioMediaStream *audio_stream = NULL;<br>
> +    IPin *pin = NULL;<br>
> +    WAVEFORMATEX stream_format = {0};<br>
> +    WAVEFORMATEX media_type_format = {0};<br>
> +    AM_MEDIA_TYPE media_type = {0};<br>
> +    HRESULT hr;<br>
> +<br>
> +    hr = IAMMultiMediaStream_AddMediaStream(mmstream, NULL,<br>
> &MSPID_PrimaryAudio, 0, &stream);<br>
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);<br>
> +    hr = IMediaStream_QueryInterface(stream, &IID_IAudioMediaStream,<br>
> (void **)&audio_stream);<br>
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);<br>
> +    hr = IMediaStream_QueryInterface(stream, &IID_IPin, (void **)&pin);<br>
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);<br>
> +<br>
> +    hr = IAudioMediaStream_GetFormat(audio_stream, NULL);<br>
> +    ok(hr == E_POINTER, "Got hr %#x.\n", hr);<br>
> +<br>
> +    hr = IAudioMediaStream_GetFormat(audio_stream, &stream_format);<br>
> +    ok(hr == MS_E_NOSTREAM, "Got hr %#x.\n", hr);<br>
> +<br>
> +    media_type_format.wFormatTag = WAVE_FORMAT_PCM;<br>
> +    media_type_format.nChannels = 2;<br>
> +    media_type_format.nSamplesPerSec = 44100;<br>
> +    media_type_format.nAvgBytesPerSec = 176400;<br>
> +    media_type_format.nBlockAlign = 4;<br>
> +    media_type_format.wBitsPerSample = 16;<br>
> +    media_type_format.cbSize = 0;<br>
> +<br>
> +    media_type.majortype = MEDIATYPE_Audio;<br>
> +    media_type.subtype = MEDIASUBTYPE_PCM;<br>
> +    media_type.bFixedSizeSamples = TRUE;<br>
> +    media_type.bTemporalCompression = FALSE;<br>
> +    media_type.lSampleSize = 2;<br>
> +    media_type.formattype = FORMAT_WaveFormatEx;<br>
> +    media_type.pUnk = NULL;<br>
> +    media_type.cbFormat = sizeof(media_type_format);<br>
> +    media_type.pbFormat = (BYTE *)&media_type_format;<br>
> +<br>
> +    hr = IPin_ReceiveConnection(pin, &output_pin, &media_type);<br>
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);<br>
<br>
And similarly, this should use IFilterGraph2::ConnectDirect() with the<br>
multimedia stream's filter graph, unless there's a good reason not to.<br>
<br>
> +<br>
> +    stream_format.wFormatTag = 0xdead;<br>
> +    stream_format.nChannels = 0xdead;<br>
> +    stream_format.nSamplesPerSec = 0xdeadbeef;<br>
> +    stream_format.nAvgBytesPerSec = 0xdeadbeef;<br>
> +    stream_format.nBlockAlign = 0xdead;<br>
> +    stream_format.wBitsPerSample = 0xdead;<br>
> +    stream_format.cbSize = 0xdead;<br>
<br>
memset(&stream_format, 0xcc, sizeof(stream_format)) would probably be<br>
easier.<br>
<br>
> +    hr = IAudioMediaStream_GetFormat(audio_stream, &stream_format);<br>
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);<br>
> +    ok(WAVE_FORMAT_PCM == stream_format.wFormatTag, "got %u\n",<br>
> stream_format.wFormatTag);<br>
> +    ok(2 == stream_format.nChannels, "got %u\n", stream_format.nChannels);<br>
> +    ok(44100 == stream_format.nSamplesPerSec, "got %u\n",<br>
> stream_format.nSamplesPerSec);<br>
> +    ok(176400 == stream_format.nAvgBytesPerSec, "got %u\n",<br>
> stream_format.nAvgBytesPerSec);<br>
> +    ok(4 == stream_format.nBlockAlign, "got %u\n", stream_format.nBlockAlign);<br>
> +    ok(16 == stream_format.wBitsPerSample, "got %u\n",<br>
> stream_format.wBitsPerSample);<br>
> +    ok(0 == stream_format.cbSize, "got %u\n", stream_format.cbSize);<br>
<br>
It's my stylistic preference (mostly borrowed from d3d) to put the<br>
constant on the right, and capitalize and punctuate trace messages.<br>
Adding a description (e.g. "Got alignment %u.") would probably also be<br>
appreciated.<br>
<br>
> +<br>
> +    hr = IPin_Disconnect(pin);<br>
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);<br>
> +<br>
> +    hr = IAudioMediaStream_GetFormat(audio_stream, &stream_format);<br>
> +    ok(hr == MS_E_NOSTREAM, "Got hr %#x.\n", hr);<br>
> +<br>
> +    IPin_Release(pin);<br>
> +    IAudioMediaStream_Release(audio_stream);<br>
> +    IMediaStream_Release(stream);<br>
> +    IAMMultiMediaStream_Release(mmstream);<br>
<br>
Can you please check that all references are released, as is done elsewhere?<br>
<br>
> +}<br>
> +<br>
>  START_TEST(amstream)<br>
>  {<br>
>      HANDLE file;<br>
> @@ -2326,5 +2660,7 @@ START_TEST(amstream)<br>
>      test_audiodata_get_format();<br>
>      test_audiodata_set_format();<br>
> <br>
> +    test_audiostream_get_format();<br>
> +<br>
>      CoUninitialize();<br>
>  }<br>
> --<br>
> 2.17.1<br>
> <br>
</blockquote></div></div>