[PATCH v2 13/13] qedit: Implement MediaDet_get_StreamLength.
Zebediah Figura
zfigura at codeweavers.com
Fri Apr 17 12:21:10 CDT 2020
On 4/17/20 7:24 AM, Gabriel Ivăncescu wrote:
> Hi Zeb,
>
> On 16/04/2020 19:52, Zebediah Figura wrote:
>> On 4/16/20 10:25 AM, Gabriel Ivăncescu wrote:
>>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>>> ---
>>> dlls/qedit/mediadet.c | 39 +++++++++++++++++++++++++++++++++++--
>>> dlls/qedit/tests/mediadet.c | 19 ++++++++++++++++++
>>> 2 files changed, 56 insertions(+), 2 deletions(-)
>>>
>>
>> I think adding the IMediaPosition and IMediaSeeking interfaces to
>> testfilter should be part of this patch rather than 12/13.
>>
>
> Right. Will do (same as some of the other emails I won't reply to, no
> point flooding the mailing list).
>
>>> diff --git a/dlls/qedit/mediadet.c b/dlls/qedit/mediadet.c
>>> index 9903238..4990dbe 100644
>>> --- a/dlls/qedit/mediadet.c
>>> +++ b/dlls/qedit/mediadet.c
>>> @@ -569,8 +569,43 @@ static HRESULT WINAPI
>>> MediaDet_get_StreamTypeB(IMediaDet* iface, BSTR *pVal)
>>> static HRESULT WINAPI MediaDet_get_StreamLength(IMediaDet* iface,
>>> double *pVal)
>>> {
>>> MediaDetImpl *This = impl_from_IMediaDet(iface);
>>> - FIXME("(%p): stub!\n", This);
>>> - return VFW_E_INVALIDMEDIATYPE;
>>> + IMediaSeeking *seeking;
>>> + IMediaPosition *pos;
>>> + HRESULT hr;
>>> +
>>> + TRACE("(%p)\n", This);
>>> +
>>> + if (!pVal)
>>> + return E_POINTER;
>>> +
>>> + if (!This->cur_pin)
>>> + return E_INVALIDARG;
>>> +
>>> + hr = IPin_QueryInterface(This->cur_pin, &IID_IMediaPosition,
>>> (void **) &pos);
>>> + if (SUCCEEDED(hr))
>>> + {
>>> + hr = IMediaPosition_get_Duration(pos, pVal);
>>> + IMediaPosition_Release(pos);
>>> + return hr;
>>> + }
>>> +
>>> + hr = IPin_QueryInterface(This->cur_pin, &IID_IMediaSeeking,
>>> (void **) &seeking);
>>> + if (SUCCEEDED(hr))
>>> + {
>>> + LONGLONG duration;
>>> +
>>> + hr = IMediaSeeking_GetDuration(seeking, &duration);
>>> + if (SUCCEEDED(hr))
>>> + {
>>> + hr = IMediaSeeking_ConvertTimeFormat(seeking, &duration,
>>> &TIME_FORMAT_MEDIA_TIME, duration, NULL);
>>> + if (SUCCEEDED(hr))
>>> + *pVal = (REFTIME)duration / 10000000;
>>> + }
>>> + IMediaSeeking_Release(seeking);
>>> + return hr;
>>> + }
>>
>> You're not really testing this, and for that matter, IMediaSeeking is
>> essentially unused in your tests.
>>
>
> It's tested for the AVI source, since those don't expose IMediaPosition.
> It also seems to be tested on Windows XP because it doesn't query
> IMediaPosition.
The AVI splitter does expose IMediaPosition on Windows (see
test_interfaces() in quartz:avisplit), so it's not really testing
anything here unless you mix components.
> Now, I can probably just drop the IMediaPosition entirely, if you want
> to be perfectly consistent with Windows, although I considered it more
> of an implementation detail. The reason I used it first, is because it
> has a method to retrieve the duration in the format we want, without
> having to resort to conversions.
If Windows doesn't bother with IMediaPosition, then yes, it would be
easier for us not to bother with it either.
> I don't know about other Windows versions than XP; some (newer) ones
> don't even have qedit.
Just Windows Server 2003, as far as I'm aware.
> Should I drop IMediaPosition?
>
>>> +
>>> + return hr;
>>> }
>>> static HRESULT WINAPI MediaDet_get_Filename(IMediaDet* iface, BSTR
>>> *pVal)
>>> diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c
>>> index 10127cf..14c99e8 100644
>>> --- a/dlls/qedit/tests/mediadet.c
>>> +++ b/dlls/qedit/tests/mediadet.c
>>> @@ -1060,6 +1060,7 @@ static void test_mediadet(void)
>>> GUID guid;
>>> BSTR bstr;
>>> AM_MEDIA_TYPE mt;
>>> + double duration;
>>> IUnknown *unk;
>>> double fps;
>>> int flags;
>>> @@ -1122,6 +1123,12 @@ static void test_mediadet(void)
>>> hr = IMediaDet_get_StreamTypeB(pM, NULL);
>>> ok(hr == E_INVALIDARG, "IMediaDet_get_StreamTypeB failed:
>>> %08x\n", hr);
>>> + hr = IMediaDet_get_StreamLength(pM, &duration);
>>> + ok(hr == E_INVALIDARG, "IMediaDet_get_StreamLength failed:
>>> %08x\n", hr);
>>> +
>>> + hr = IMediaDet_get_StreamLength(pM, NULL);
>>> + ok(hr == E_POINTER, "IMediaDet_get_StreamLength failed: %08x\n",
>>> hr);
>>> +
>>> hr = IMediaDet_get_Filter(pM, NULL);
>>> ok(hr == E_POINTER, "IMediaDet_get_Filter failed: %08x\n", hr);
>>> @@ -1202,6 +1209,10 @@ static void test_mediadet(void)
>>> ok(!wcscmp(bstr, L"{73646976-0000-0010-8000-00AA00389B71}"),
>>> "Wrong GUID %s\n", wine_dbgstr_w(bstr));
>>> SysFreeString(bstr);
>>> + hr = IMediaDet_get_StreamLength(pM, &duration);
>>> + ok(hr == S_OK, "IMediaDet_get_StreamLength failed: %08x\n", hr);
>>> + ok(duration >= 0.1 && duration < 0.10000001, "Wrong duration
>>> %.17g\n", duration);
>>
>> %.16e.
>>
>> I'm also not sure that I particularly care about adding these tests; as
>> mentioned in 12/13, I'd rather see the AVI parts go away entirely.
>>
>
> I'd rather we keep the AVI parts since they test a different thing (more
> of that in the other patch reply).
>
>>> +
>>> /* Even before get_OutputStreams. */
>>> hr = IMediaDet_put_CurrentStream(pM, 1);
>>> ok(hr == E_INVALIDARG, "IMediaDet_put_CurrentStream failed:
>>> %08x\n", hr);
>>> @@ -1264,6 +1275,10 @@ static void test_mediadet(void)
>>> ok(!wcscmp(bstr, L"{73646976-0000-0010-8000-00AA00389B71}"),
>>> "Wrong GUID %s\n", wine_dbgstr_w(bstr));
>>> SysFreeString(bstr);
>>> + hr = IMediaDet_get_StreamLength(pM, &duration);
>>> + ok(hr == S_OK, "IMediaDet_get_StreamLength failed: %08x\n", hr);
>>> + ok(duration >= 0.1 && duration < 0.10000001, "Wrong duration
>>> %.17g\n", duration);
>>> +
>>> hr = IMediaDet_get_FrameRate(pM, NULL);
>>> ok(hr == E_POINTER, "IMediaDet_get_FrameRate failed: %08x\n", hr);
>>> @@ -1392,6 +1407,10 @@ static void test_mediadet(void)
>>> ok(hr == S_OK, "IMediaDet_get_OutputStreams failed: %08x\n", hr);
>>> ok(nstrms == 1, "IMediaDet_get_OutputStreams: nstrms is %i\n",
>>> nstrms);
>>> + hr = IMediaDet_get_StreamLength(pM, &duration);
>>> + ok(hr == S_OK, "IMediaDet_get_StreamLength failed: %08x\n", hr);
>>> + ok(duration >= 4.2 && duration < 4.20000001, "Wrong duration
>>> %.17g\n", duration);
>>> +
>>
>> Surely we can at least compare this to 4.2 exactly? That's the double
>> that we return.
>>
>
> Well, Windows XP reports something like 4.2000000000002 (not the exact
> amount of zeros), because it uses IMediaSeeking and does the conversion
> which probably loses some accuracy. So we have to keep it, I guess.
In that case, can we instead borrow compare_double() from msvcrt:string?
>
>>> hr = IMediaDet_Release(pM);
>>> ok(hr == 0, "IMediaDet_Release returned: %x\n", hr);
>>>
>>
>
More information about the wine-devel
mailing list