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

[Zebediah Figura]

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.

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

-------------- 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/20200915/d4fe35a7/attachment-0001.sig>