[PATCH 2/7] qedit: Implement IMediaDet::EnterBitmapGrabMode.
Zebediah Figura
zfigura at codeweavers.com
Fri Oct 23 10:58:14 CDT 2020
On 10/21/20 7:49 AM, Gabriel Ivăncescu wrote:
> On 20/10/2020 20:11, Zebediah Figura wrote:
>>
>>
>> On 10/19/20 11:48 AM, Gabriel Ivăncescu wrote:
>>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>>> ---
>>> dlls/qedit/mediadet.c | 191 ++++++++++++++++++++++++++++++++++--
>>> dlls/qedit/tests/mediadet.c | 38 ++++---
>>> 2 files changed, 203 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/dlls/qedit/mediadet.c b/dlls/qedit/mediadet.c
>>> index bacf520..11498bd 100644
>>> --- a/dlls/qedit/mediadet.c
>>> +++ b/dlls/qedit/mediadet.c
>>> @@ -40,6 +40,7 @@ typedef struct MediaDetImpl {
>>> IGraphBuilder *graph;
>>> IBaseFilter *source;
>>> IBaseFilter *splitter;
>>> + ISampleGrabber *grabber;
>>> WCHAR *filename;
>>> LONG num_streams;
>>> LONG cur_stream;
>>> @@ -64,6 +65,8 @@ static void MD_cleanup(MediaDetImpl *This)
>>> This->source = NULL;
>>> if (This->splitter) IBaseFilter_Release(This->splitter);
>>> This->splitter = NULL;
>>> + if (This->grabber) ISampleGrabber_Release(This->grabber);
>>> + This->grabber = NULL;
>>> if (This->graph) IGraphBuilder_Release(This->graph);
>>> This->graph = NULL;
>>> free(This->filename);
>>> @@ -101,6 +104,38 @@ static HRESULT get_filter_info(IMoniker *moniker, GUID *clsid, VARIANT *var)
>>> return hr;
>>> }
>>>
>>> +static HRESULT get_first_pin(IBaseFilter *filter, PIN_DIRECTION pin_dir, IPin **out)
>>> +{
>>> + IEnumPins *pins;
>>> + HRESULT hr;
>>> + IPin *pin;
>>> +
>>> + if (FAILED(hr = IBaseFilter_EnumPins(filter, &pins)))
>>> + return hr;
>>> +
>>> + while (IEnumPins_Next(pins, 1, &pin, NULL) == S_OK)
>>> + {
>>> + PIN_DIRECTION dir;
>>> +
>>> + hr = IPin_QueryDirection(pin, &dir);
>>> + if (FAILED(hr))
>>> + {
>>> + IPin_Release(pin);
>>> + IEnumPins_Release(pins);
>>> + return hr;
>>> + }
>>> + if (dir == pin_dir)
>>> + {
>>> + *out = pin;
>>> + IEnumPins_Release(pins);
>>> + return S_OK;
>>> + }
>>> + IPin_Release(pin);
>>> + }
>>> + IEnumPins_Release(pins);
>>> + return E_NOTIMPL;
>>> +}
>>> +
>>> static HRESULT get_pin_media_type(IPin *pin, AM_MEDIA_TYPE *out)
>>> {
>>> IEnumMediaTypes *enummt;
>>> @@ -224,6 +259,71 @@ next:
>>> return hr;
>>> }
>>>
>>> +static HRESULT seek_source(MediaDetImpl *detector, double seek_time)
>>
>> "seek_source" is a bit of a weird name for this, because you're really
>> seeking the graph; it's just that seeking the graph implicitly means
>> seeking the source.
>>
>>> +{
>>> + FILTER_STATE state;
>>> + LONGLONG pos, stop;
>>> + IMediaControl *mc;
>>> + IMediaSeeking *ms;
>>> + IMediaFilter *mf;
>>> + GUID format;
>>> + HRESULT hr;
>>> +
>>> + if (FAILED(hr = IPin_QueryInterface(detector->cur_pin, &IID_IMediaSeeking, (void**)&ms)))
>>> + return hr;
>>> +
>>> + if (IMediaSeeking_IsUsingTimeFormat(ms, &TIME_FORMAT_MEDIA_TIME) != S_OK &&
>>> + (FAILED(IMediaSeeking_GetTimeFormat(ms, &format)) ||
>>> + !IsEqualGUID(&format, &TIME_FORMAT_MEDIA_TIME)))
>>> + {
>>> + /* Windows doesn't implement it */
>>> + hr = E_NOTIMPL;
>>> + }
>>> + IMediaSeeking_Release(ms);
>>> + if (FAILED(hr))
>>> + return hr;
>>> +
>>> + if (FAILED(hr = IGraphBuilder_QueryInterface(detector->graph, &IID_IMediaControl, (void**)&mc)))
>>> + return hr;
>>> +
>>> + if (FAILED(hr = IMediaControl_Stop(mc)))
>>> + goto done;
>>
>> Why do you need to stop the graph?
>>
>
> This is one of those bugs uncovered by the realistic custom filter :-)
>
> If I don't stop it, there's a race condition with the Sample Grabber.
> The SG just gets the last Paused sample. When the filter seeks, it gets
> the sample that blocked first (because it now returned), instead of the
> one on the new seek, which is sent after it. It happens at the same time
> but it's slower than the test, so it always failed for me.
>
> When I traced Windows with strmbase to see what it calls on my custom
> filter, I also got a Stop() when seeking.
>
> This is one of the reasons I'd really prefer to keep the custom filter
> the way it is right now. This bug wouldn't even be tested for if we
> didn't ran a realistic loop and flush, for example.
>
>>> +
>>> + if (seek_time >= 0.0)
>>> + {
>>> + if (FAILED(hr = IGraphBuilder_QueryInterface(detector->graph, &IID_IMediaSeeking, (void**)&ms)))
>>> + goto done;
>>> +
>>> + stop = pos = seek_time * 10000000.0;
>>> + hr = IMediaSeeking_SetPositions(ms, &pos, AM_SEEKING_AbsolutePositioning,
>>> + &stop, AM_SEEKING_AbsolutePositioning);
>>> + IMediaSeeking_Release(ms);
>>> + if (FAILED(hr))
>>> + goto done;
>>> + }
>>> +
>>> + if (FAILED(hr = IGraphBuilder_QueryInterface(detector->graph, &IID_IMediaFilter, (void**)&mf)))
>>> + goto done;
>>> + hr = IMediaFilter_SetSyncSource(mf, NULL);
>>> + IMediaFilter_Release(mf);
>>> + if (FAILED(hr)) goto done;
>>> +
>>> + if (FAILED(hr = IMediaControl_Pause(mc)))
>>> + goto done;
>>
>> Or pause it?
>>
>
> Same as above: if we don't pause it, it just pumps out samples, so we
> need to freeze it (if we didn't send samples in a loop with the filter,
> it wouldn't test for this, so another reason for the loop).
>
> I can probably add a test to check that the graph is indeed paused,
> because that's what Windows does (it doesn't use the OneShot capability
> of the Sample Grabber). This matters since the application can
> potentially grab the Sample Grabber and screw with it.
>
>>> +
>>> + /* Testing on Windows shows it waits up to 37500 ms */
>>> + if (FAILED(hr = IMediaControl_GetState(mc, 37500, (OAFilterState*)&state)))
>>> + {
>>> + if (hr == VFW_S_STATE_INTERMEDIATE) hr = VFW_E_TIME_EXPIRED;
>>> + goto done;
>>> + }
>>> +
>>> + hr = (state == State_Paused) ? S_OK : E_FAIL;
>>
>> Can this even happen?
>>
>
> Uh, I remember I hacked some stuff when testing on Windows and managed
> to get it return E_FAIL when I overrode the state in
> testfilter_init_stream. But it's probably not important so I'll leave it
> out.
If you return the wrong state from IBaseFilter::GetState(), the filter
graph will return E_FAIL (we actually have tests for that, and implement
it correctly even). But in that case we're probably just propagating the
same return value.
>
>>> +done:
>>
>> I don't think that labels are generally worthwhile, not when there's
>> only one line of cleanup.
>>
>
> But it's called from a lot of places and it simplifies the code,
> otherwise I'd have to open braces and add two lines on each of them.
> That would be almost 20 extra lines of code. Are you sure I should get
> rid of it?
I guess it's fine...
>
>>> + IMediaControl_Release(mc);
>>> + return hr;
>>> +}
>>> +
>>> /* MediaDet inner IUnknown */
>>> static HRESULT WINAPI MediaDet_inner_QueryInterface(IUnknown *iface, REFIID riid, void **ppv)
>>> {
>>> @@ -371,7 +471,7 @@ static HRESULT WINAPI MediaDet_get_OutputStreams(IMediaDet* iface, LONG *pVal)
>>>
>>> TRACE("(%p)\n", This);
>>>
>>> - if (!This->splitter)
>>> + if (This->grabber || !This->splitter)
>>> return E_INVALIDARG;
>>>
>>> if (This->num_streams != -1)
>>> @@ -466,6 +566,9 @@ static HRESULT WINAPI MediaDet_put_CurrentStream(IMediaDet* iface, LONG newVal)
>>>
>>> TRACE("(%p)->(%d)\n", This, newVal);
>>>
>>> + if (This->grabber)
>>> + return E_INVALIDARG;
>>> +
>>> if (This->num_streams == -1)
>>> {
>>> LONG n;
>>> @@ -495,6 +598,8 @@ static HRESULT WINAPI MediaDet_get_StreamType(IMediaDet *iface, GUID *majortype)
>>>
>>> if (!majortype)
>>> return E_POINTER;
>>> + if (detector->grabber)
>>> + return E_INVALIDARG;
>>>
>>> if (SUCCEEDED(hr = IMediaDet_get_StreamMediaType(iface, &mt)))
>>> {
>>> @@ -533,7 +638,7 @@ static HRESULT WINAPI MediaDet_get_StreamLength(IMediaDet *iface, double *length
>>> if (!length)
>>> return E_POINTER;
>>>
>>> - if (!detector->cur_pin)
>>> + if (detector->grabber || !detector->cur_pin)
>>> return E_INVALIDARG;
>>>
>>> if (SUCCEEDED(hr = IPin_QueryInterface(detector->cur_pin,
>>> @@ -647,7 +752,7 @@ static HRESULT WINAPI MediaDet_get_StreamMediaType(IMediaDet* iface,
>>> if (!pVal)
>>> return E_POINTER;
>>>
>>> - if (!This->cur_pin)
>>> + if (This->grabber || !This->cur_pin)
>>> return E_INVALIDARG;
>>>
>>> return get_pin_media_type(This->cur_pin, pVal);
>>> @@ -672,6 +777,8 @@ static HRESULT WINAPI MediaDet_get_FrameRate(IMediaDet* iface, double *pVal)
>>>
>>> if (!pVal)
>>> return E_POINTER;
>>> + if (This->grabber)
>>> + return E_INVALIDARG;
>>>
>>> hr = MediaDet_get_StreamMediaType(iface, &mt);
>>> if (FAILED(hr))
>>> @@ -693,9 +800,81 @@ static HRESULT WINAPI MediaDet_get_FrameRate(IMediaDet* iface, double *pVal)
>>> static HRESULT WINAPI MediaDet_EnterBitmapGrabMode(IMediaDet* iface,
>>> double SeekTime)
>>> {
>>> - MediaDetImpl *This = impl_from_IMediaDet(iface);
>>> - FIXME("(%p)->(%f): not implemented!\n", This, SeekTime);
>>> - return E_NOTIMPL;
>>> + MediaDetImpl *detector = impl_from_IMediaDet(iface);
>>> + IPin *sg_inpin, *sg_outpin, *null_pin;
>>> + IBaseFilter *sg_filter, *null_filter;
>>> + ISampleGrabber *sg;
>>> + AM_MEDIA_TYPE mt;
>>> + GUID major_type;
>>> + HRESULT hr;
>>> +
>>> + TRACE("(%p)->(%f)\n", detector, SeekTime);
>>
>> Use "%.16e" for doubles, please; we'd like to keep full precision.
>>
>>> +
>>> + if (detector->grabber)
>>> + return S_OK;
>>> + if (!detector->cur_pin)
>>> + return E_INVALIDARG;
>>> + if (FAILED(hr = get_pin_media_type(detector->cur_pin, &mt)))
>>> + return hr;
>>> + major_type = mt.majortype;
>>> + FreeMediaType(&mt);
>>> + if (!IsEqualGUID(&major_type, &MEDIATYPE_Video))
>>> + return VFW_E_INVALIDMEDIATYPE;
>>
>> Can this be tested?
>>
>
> I think so, I think I'll change just the major type to check that it's
> the only thing being tested.
>
>>> +
>>> + hr = CoCreateInstance(&CLSID_SampleGrabber, NULL, CLSCTX_INPROC_SERVER,
>>> + &IID_IBaseFilter, (void**)&sg_filter);
>>
>> I'd be more than a little tempted ot use sample_grabber_create()
>> directly, bypassing COM overhead...
>>
>>> + if (FAILED(hr)) return hr;
>>> +
>>> + if (FAILED(hr = IBaseFilter_QueryInterface(sg_filter, &IID_ISampleGrabber, (void**)&sg)))
>>> + {
>>> + IBaseFilter_Release(sg_filter);
>>> + return hr;
>>> + }
>>> +
>>> + memset(&mt, 0, sizeof(mt));
>>> + mt.majortype = MEDIATYPE_Video;
>>> + mt.subtype = MEDIASUBTYPE_RGB24;
>>> + mt.formattype = FORMAT_VideoInfo;
>>> + if (FAILED(hr = ISampleGrabber_SetMediaType(sg, &mt)) ||
>>> + FAILED(hr = CoCreateInstance(&CLSID_NullRenderer, NULL, CLSCTX_INPROC_SERVER,
>>> + &IID_IBaseFilter, (void**)&null_filter)))
>>
>> And the same here...
>>
>>> + {
>>> + ISampleGrabber_Release(sg);
>>> + IBaseFilter_Release(sg_filter);
>>> + return hr;
>>> + }
>>> + ISampleGrabber_SetBufferSamples(sg, TRUE);
>>> +
>>> + sg_inpin = sg_outpin = null_pin = NULL;
>>> + if (FAILED(hr = get_first_pin(sg_filter, PINDIR_INPUT, &sg_inpin))) goto err;
>>> + if (FAILED(hr = get_first_pin(sg_filter, PINDIR_OUTPUT, &sg_outpin))) goto err;
>>> + if (FAILED(hr = get_first_pin(null_filter, PINDIR_INPUT, &null_pin))) goto err;
>>
>> IBaseFilter::FindPin() would be simpler. Also, it's our code; the calls
>> shouldn't fail, which means at best they deserve an assert().
>>
>>> +
>>> + if (SUCCEEDED(hr = IGraphBuilder_AddFilter(detector->graph, sg_filter, L"BitBucket")))
>>> + {
>>
>> I would probably stick with FAILED() error handling here (i.e. reverse
>> it); it's consistent with the above and it keeps the indentation level low.
>>
>>> + if (SUCCEEDED(hr = IGraphBuilder_AddFilter(detector->graph, null_filter, L"NullRenderer")))
>>> + {
>>> + if (SUCCEEDED(hr = IGraphBuilder_Connect(detector->graph, detector->cur_pin, sg_inpin)) &&
>>> + SUCCEEDED(hr = IGraphBuilder_Connect(detector->graph, sg_outpin, null_pin)) &&
>>
>> At least the latter can be ConnectDirect().
>>
>>> + SUCCEEDED(hr = seek_source(detector, SeekTime)))
>>> + {
>>> + detector->grabber = sg;
>>> + goto done;
>>> + }
>>> + IGraphBuilder_RemoveFilter(detector->graph, null_filter);
>>> + }
>>> + IGraphBuilder_RemoveFilter(detector->graph, sg_filter);
>>> + }
>>> +
>>> +err:
>>> + ISampleGrabber_Release(sg);
>>> +done:
>>> + if (null_pin) IPin_Release(null_pin);
>>> + if (sg_outpin) IPin_Release(sg_outpin);
>>> + if (sg_inpin) IPin_Release(sg_inpin);
>>> + IBaseFilter_Release(null_filter);
>>> + IBaseFilter_Release(sg_filter);
>>> + return hr;
>>> }
>>>
>>> static const IMediaDetVtbl IMediaDet_VTable =
>>> diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c
>>> index 010b746..afad173 100644
>>> --- a/dlls/qedit/tests/mediadet.c
>>> +++ b/dlls/qedit/tests/mediadet.c
>>> @@ -1335,7 +1335,7 @@ static void test_bitmap_grab_mode(void)
>>> ok(hr == S_OK, "Got hr %#x.\n", hr);
>>>
>>> hr = IMediaDet_EnterBitmapGrabMode(detector, 0.0);
>>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>>
>>> /* EnterBitmapGrabMode only seeks once, and if SeekTime is non-negative */
>>> testfilter_init(&testfilter);
>>> @@ -1344,10 +1344,10 @@ static void test_bitmap_grab_mode(void)
>>> ok(hr == S_OK, "Got hr %#x.\n", hr);
>>>
>>> hr = IMediaDet_EnterBitmapGrabMode(detector, -1.0);
>>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> ok(testfilter.cur_pos == 0xdeadbeef, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>> hr = IMediaDet_EnterBitmapGrabMode(detector, 1.0);
>>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> ok(testfilter.cur_pos == 0xdeadbeef, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>>
>>> ref = IMediaDet_Release(detector);
>>> @@ -1371,11 +1371,11 @@ static void test_bitmap_grab_mode(void)
>>> hr = IMediaDet_EnterBitmapGrabMode(detector, 1337.0);
>>> if (time_formats[i] == &TIME_FORMAT_MEDIA_TIME)
>>> {
>>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> - todo_wine ok(testfilter.cur_pos == 13370000000LL, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + ok(testfilter.cur_pos == 13370000000LL, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>> hr = IMediaDet_EnterBitmapGrabMode(detector, 1.0);
>>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> - todo_wine ok(testfilter.cur_pos == 13370000000LL, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + ok(testfilter.cur_pos == 13370000000LL, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>> }
>>> else
>>> ok(hr == E_NOTIMPL, "Got hr %#x.\n", hr);
>>> @@ -1396,11 +1396,11 @@ static void test_bitmap_grab_mode(void)
>>> ok(hr == S_OK, "Got hr %#x.\n", hr);
>>>
>>> hr = IMediaDet_EnterBitmapGrabMode(detector, 0.0);
>>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> - todo_wine ok(testfilter.cur_pos == 0, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + ok(testfilter.cur_pos == 0, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>> hr = IMediaDet_EnterBitmapGrabMode(detector, 1.0);
>>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> - todo_wine ok(testfilter.cur_pos == 0, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + ok(testfilter.cur_pos == 0, "Current position was set to 0x%s.\n", wine_dbgstr_longlong(testfilter.cur_pos));
>>>
>>> /* These still work */
>>> hr = IMediaDet_get_Filter(detector, &unk);
>>> @@ -1415,21 +1415,19 @@ static void test_bitmap_grab_mode(void)
>>>
>>> /* These don't work anymore */
>>> hr = IMediaDet_get_OutputStreams(detector, &count);
>>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> hr = IMediaDet_get_FrameRate(detector, &duration);
>>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> hr = IMediaDet_get_StreamLength(detector, &duration);
>>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> hr = IMediaDet_get_StreamMediaType(detector, &mt);
>>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> - if (SUCCEEDED(hr)) FreeMediaType(&mt);
>>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> hr = IMediaDet_get_StreamType(detector, &guid);
>>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> hr = IMediaDet_get_StreamTypeB(detector, &str);
>>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> - if (SUCCEEDED(hr)) SysFreeString(str);
>>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> hr = IMediaDet_put_CurrentStream(detector, 0);
>>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>>
>>> /* Changing filter resets bitmap grab mode */
>>> testfilter.bitmap_grab_mode = FALSE;
>>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201023/d59e97d3/attachment.sig>
More information about the wine-devel
mailing list