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

[Derek Lesho]

On 9/15/20 10:16 AM, Zebediah Figura wrote:
> On 9/15/20 9:06 AM, Derek Lesho wrote:
>> 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?
> I haven't profiled it. I don't think that caps copying is exactly cheap,
> though; from what I've seen gstreamer makes efforts to avoid it.
>
>>> 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?
> In general it's better to improve the code structure rather than to
> document bad code structure.
>
>>>    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.
> By which I mostly mean that the modifications made are orthogonal to the
> type conversion done. I.e. it might make more sense if you did catch-all
> handling of video format types that can't be converted (though returning
> failure and watching for that may likely be better structure), but in
> this case doing both things in the same function isn't really saving you
> anything.
Ok then, I'll factor out the function in v3.
>
>>>>> 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
>>>>
>>>>
>>>>