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

[Zebediah Figura]

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?
> -
> - 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?
> -
> - /* 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?
> +done:

I don't think that labels are generally worthwhile, not when there's
only one line of cleanup.
> - 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?
> -
> - 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;