Re: [PATCH 2/7] qedit: Implement IMediaDet::EnterBitmapGrabMode.

[Gabriel Ivăncescu]

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
> >
> > 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.
> > +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?
> > - 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;