On Fri Oct 20 01:24:23 2023 +0000, Zebediah Figura wrote:
Hi, really sorry about the very late review on this one (I was out for a while, and then spent a lot of time catching up on review while I was out, and then trying to get some more important Wine work done...) 2/7, seeking parts aside, is broadly doing the right thing (from my perspective) but I would like to see it arranged differently. What I would like to see is something like the following, in a series of individual patches:
- Introduce a wg_parser_create option to use the encoded format; this
should end up being translated to "chain_decodebins" basically as you have it. I would also rename that to something like "output_compressed" (with inverted meaning). Note that in this case autoplug-continue already does what we want, you shouldn't need to set the "caps" property on top of that.
- Use this property, plus WG_PARSER_DECODEBIN, in the existing frontend
instead of WG_PARSER_MPEGAUDIOPARSE. Delete WG_PARSER_MPEGAUDIOPARSE.
- Ideally patch 1/7 should be moved from the beginning of the series to here.
- Add MPEG-1 system stream support to the quartz frontend.
Yes, IRL stuff is always in the way (from us internet folks' perspective), and code review isn't the most exciting task.
I don't understand the point of that autoplug-continue handler. It looks like it says "yep, I want this" for certain formats, then it turns around and says "no, I don't want this after all, decode it again" in the pad-added signal. I'm sure that complexity exists for some reason, otherwise someone would've deleted it long ago, but...
But I don't need to understand it, as long as someone does.
This took me longer to implement than I expected, didn't expect you to be this thorough. Sure helps to have a pair of fresh eyes on it. Worth the wait (though it would've helped to have known how long it'd take).
I think I fixed everything (except those with follow-up questions), but some changes are pretty big and/or not exactly what you were thinking of, so I suspect you'll have another set of remarks.
-
Alfred Agrell (＠Alcaro)