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

[Zebediah Figura]

On 8/27/20 10:18 AM, Derek Lesho wrote:
> 
> 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 <dlesho at codeweavers.com>
>>> ---
>>> 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.

I think either way works; I've seen both happen in practice. Generally I
opt for not using a goto, in order to not thrash the end of the function.

> 
> 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?

Sure, but improving something's architecture counts as a good reason, I
think.

In general I'm inclined to prefer such an arrangement, but I'm not sure
I'm familiar enough with mfplat to comment on test_source_resolver().

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

-------------- 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/20200827/25e90935/attachment.sig>