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