[PATCH 3/3] winegstreamer: Implement IMFMediaStream::GetStreamDescriptor.

[Derek Lesho]

On 9/14/20 3:49 PM, Zebediah Figura wrote:
>
> On 9/14/20 2:28 PM, Derek Lesho wrote:
>> On 9/10/20 7:54 PM, Zebediah Figura wrote:
>>
>>> It's not really idiomatic code structure. More concretely, it also means
>>> you're copying the caps when you don't need to.
>> This doesn't strike me as a concern during source initialization. We
>> won't be calling this function while media is playing.
> Sure, though startup time shouldn't exactly be ignored, either. [For
> instance, quartz + gstreamer currently can take multiple hundreds of
> milliseconds to start up, which is actually kind of abhorrent.]
Oof, have you profiled this?  I doubt that excessive caps copying would 
be the reason.  Maybe the issue is with gstreamer?
>
> My concern is more about idiomatic structure than anything else, in any
> case. The function, for instance, is doing multiple things at once, and
> it's not how I'd expect a conversion function to behave. Wrapping it in
> a different function helps abstract that away to a more idiomatic
> interface, but the implementation remains harder to read simply because
> it's not (maximally) modularized.
Maybe the function seems simpler to read for me than it actually is, 
since I wrote it.  Would adding a better description comment 
`transform_to_media_type` help?
>   The fact that those tasks are (at
> least as of this patch) entirely orthogonal doesn't help either.
Can you elaborate what you mean by the tasks being orthogonal?  As I 
said in my last email, the two functions based on 
`transform_to_media_type` are used hand-in-hand in the main parser path, 
as I linked in my last email.  I do now realize though that 
`make_mf_compatible_caps` is dead code in this patch and should be moved 
up though.
>
>>> If you really need to do something like this, you probably want a
>>> separate function that takes a GstCaps and returns a new GstCaps that is
>>> compatible with mfplat, then you feed the output of that through
>>> mf_media_type_from_caps().
>> This is what I currently do[1], it's just that internally I prefer to
>> wrap this into a single function, to keep the conversion code and the
>> correction code next to eachother.
>>
>> I'm about to send v2, and everything but this comment should be addressed.
>>
>> 1:
>> https://github.com/Guy1524/wine/blob/mfplat_cleanup/dlls/winegstreamer/media_source.c#L1133
>>
>>
>>