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

[Zebediah Figura]

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.
> -
>
> 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__ */