Re: [PATCH v2 1/5] winegstreamer: Implement stub bytestream handler and media source.

[Derek Lesho]

On 8/26/20 6:42 PM, Zebediah Figura wrote:
> This patch is still awfully large to review all at once. Just from
> skimming I see the "event_queue" field, which could be split into a
> separate patch, and the different "container_stream_handler" and
> "media_source" objects, which could be split into separate patches.
>
> On 8/26/20 1:59 PM, Derek Lesho wrote:
> > Signed-off-by: Derek Lesho
> > v2: Fix tests on environments without gstreamer.
> >
> > dlls/mfplat/tests/mfplat.c                   |  14 +-
> >   dlls/winegstreamer/Makefile.in               |   1 +
> >   dlls/winegstreamer/gst_private.h             |   6 +
> >   dlls/winegstreamer/media_source.c            | 680 +++++++++++++++++++
> >   dlls/winegstreamer/mfplat.c                  |   9 +
> >   dlls/winegstreamer/winegstreamer.rgs         |  32 +
> >   dlls/winegstreamer/winegstreamer_classes.idl |   7 +
> >   include/mfidl.idl                            |   1 +
> >   8 files changed, 746 insertions(+), 4 deletions(-)
> >   create mode 100644 dlls/winegstreamer/media_source.c
> >
> > diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c
> > index 01749dd9ef8..29bcef7e46b 100644
> > --- a/dlls/mfplat/tests/mfplat.c
> > +++ b/dlls/mfplat/tests/mfplat.c
> > @@ -441,8 +441,8 @@ static void test_source_resolver(void)
> >   {
> >       struct test_callback callback = { { &test_create_from_url_callback_vtbl } };
> >       struct test_callback callback2 = { { &test_create_from_file_handler_callback_vtbl } };
> > - IMFPresentationDescriptor *descriptor = NULL;
> >  IMFSourceResolver *resolver, *resolver2;
> > - IMFPresentationDescriptor *descriptor;
> >  IMFSchemeHandler *scheme_handler;
> >  IMFMediaStream *video_stream;
> >  IMFAttributes *attributes;
> >
> > @@ -458,9 +458,12 @@ static void test_source_resolver(void)
> >       int i, sample_count;
> >       WCHAR *filename;
> >       PROPVARIANT var;
> > -
> >
> > BOOL is_wine;
> >  HRESULT hr;
> >  GUID guid;
> > -
> >
> > is_wine = !strcmp(winetest_platform, "wine");
> > -
> > if (!pMFCreateSourceResolver)
> > {
> >     win_skip("MFCreateSourceResolver() not found\n");
> >
> > @@ -529,7 +532,10 @@ static void test_source_resolver(void)
> >       ok(obj_type == MF_OBJECT_MEDIASOURCE, "got %d\n", obj_type);
> >   hr = IMFMediaSource_CreatePresentationDescriptor(mediasource, &descriptor);
> >
> > +if (!is_wine)
> >       ok(hr == S_OK, "Failed to get presentation descriptor, hr %#x.\n", hr);
>
> I suspect you're doing this because of the fallback path in
> source_resolver_CreateObjectFromByteStream(), combined with the optional
> presence of winegstreamer. I would propose that this patch series first
> remove that fallback path, as it is (as I understand) entirely
> incorrect, and since any potential regression will be addressed by this
> patch series itself.

Even after I remove the stub source, I think I still need two goto 
paths, one which triggers when the source fails to create, skipping all 
the actions on the source object, and the other which slowly recedes 
over the course of the patches.  (skipping tests after 
::CreatePresentationDescriptor before it's implemented, then skipping 
tests after ::Start before it's implemented, then finally disappearing 
once ::Start is implemented)  Is this okay?  Or alternatively should I 
just free the resources and return early when creating the media source 
fails, skipping some of the winegstreamer agnostic tests at the end.

In my opinion, the best solution would be to move the winegstreamer 
dependent part/s of the test function to the end so that we can return 
early instead of using a goto in this instance.  But isn't there a 
preference against moving code around unnecessarily?
> > -
> >
> > if (FAILED(hr))
> > -
> >    goto skip_source_tests;
> > ok(descriptor != NULL, "got %p\n", descriptor);
> >
> > hr = IMFPresentationDescriptor_GetStreamDescriptorByIndex(descriptor, 0, &selected, &sd);
> >
> > @@ -540,7 +546,7 @@ static void test_source_resolver(void)
> >       IMFStreamDescriptor_Release(sd);
> >   hr = IMFMediaTypeHandler_GetMajorType(handler, &guid);
> >
> > -todo_wine
> > +if (!is_wine)
> >       ok(hr == S_OK, "Failed to get stream major type, hr %#x.\n", hr);
> >       if (FAILED(hr))
> >           goto skip_source_tests;
> > @@ -632,8 +638,8 @@ todo_wine
> >       ok(hr == MF_E_SHUTDOWN, "Unexpected hr %#x.\n", hr);
> >
> > skip_source_tests:
> > -
> > - IMFPresentationDescriptor_Release(descriptor);
> > - if (descriptor)
> > -
> >    IMFPresentationDescriptor_Release(descriptor);
> > IMFMediaSource_Release(mediasource);
> > IMFByteStream_Release(stream);
> >
> > diff --git a/dlls/winegstreamer/Makefile.in b/dlls/winegstreamer/Makefile.in
> > index 337c1086e6b..e578d194f7f 100644
> > --- a/dlls/winegstreamer/Makefile.in
> > +++ b/dlls/winegstreamer/Makefile.in
> > @@ -10,6 +10,7 @@ C_SRCS = \
> >    gst_cbs.c \
> >    gstdemux.c \
> >    main.c \
> > - media_source.c \
> > mediatype.c \
> > mfplat.c \
> > pin.c \
> >
> > diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h
> > index e6fb841fc87..71ca4290885 100644
> > --- a/dlls/winegstreamer/gst_private.h
> > +++ b/dlls/winegstreamer/gst_private.h
> > @@ -54,4 +54,10 @@ void start_dispatch_thread(void) DECLSPEC_HIDDEN;
> >
> > extern HRESULT mfplat_get_class_object(REFCLSID rclsid, REFIID riid, void **obj) DECLSPEC_HIDDEN;
> >
> > +enum source_type
> > +{
> > - SOURCE_TYPE_MPEG_4,
> >
> > +};
>
> This enumeration, and variables associated with it, are dead code, and
> would be best introduced when used.
> > +HRESULT container_stream_handler_construct(REFIID riid, void **obj, enum source_type);
>
> Missing DECLSPEC_HIDDEN. Using the "_create" convention as elsewhere
> would probably not be a bad idea, either.
> > - #endif /* __GST_PRIVATE_INCLUDED__ */