On Tue Oct 18 06:42:23 2022 +0000, Rémi Bernon wrote:
I don't think a preallocated pool can work. Not with allocations callbacks like in wmvcore and quartz. Especially as these may block and as they are supposed to be called at specific times and in specific threads. And yes I have considered and tried it, and some alternate approaches, and I believe the one implemented here is good, and that it is also not so complex. It brings wg_transform and wg_parser interfaces closer to each other and use the same zero copy primitives for both.
Just recently someone was put off from using it in new code...
I've missed that probably, in which context? Was the discussion public?
Sorry for the slow response.
I don't think a preallocated pool can work. Not with allocations callbacks like in wmvcore and quartz. Especially as these may block and as they are supposed to be called at specific times and in specific threads.
Sorry, "pool" was the wrong word to use. I mean rather that we should be able to provide a specific buffer to the winegstreamer backend, just like with the transform. We don't exactly know the frame size beforehand, but we can make a good guess in pretty much every case (after all, quartz basically has to), and if we fail then we can fall back to a blit.
I know there was discussion about this in merge request 197 already, but I'd like to bring this up again. (Partly because that discussion operated under some false premises on my part, e.g. we don't actually have to establish a fixed size pool beforehand, at least not for mfplat and wmvcore.)
Ultimately it'd be nice if this can act *exactly* like wg_transform, i.e. pass in a pre-allocated sample, let the backend fill that and zero-copy if it can, with the frontend agnostic as to whether zero-copy actually happened. That way we only have the one really complex pattern.
And yes I have considered and tried it, and some alternate approaches, and I believe the one implemented here is good, and that it is also not so complex. It brings wg_transform and wg_parser interfaces closer to each other and use the same zero copy primitives for both.
I understand it's frustrating to have design decisions questioned, or have approaches suggested that you've already tried and found inviable, but from my perspective, it's also frustrating to have no explanation of that. If this patch series is a worthwhile tradeoff, I'd like to understand why. I currently don't understand why. I also feel like this (and other design decisions made in winegstreamer, including those I've made) are not as simple as they're made out to be, and I'm not sure if that's an effect of the submitter's then-familiarity with the code, or a failing on my part to understand and remember the structure. To be sure, sometimes it's necessary to accept complex code, because of things like PE conversion or zero-copy not working well with the winegstreamer threading model, but it's not as clear to me that this is one of those times.
Having taken some more time to look at this, and examine the context around it: on its own, this change seems arguably like an improvement, on the grounds that we already need to buffer one sample on the wg_parser side anyway, and my counter-proposal would involve adding extra buffering. My concern, however, is that this buffering will make zero-copy support in the parser harder, rather than easier. Partly because it's more code to work around, but partly because I think that it would interact badly with a world where we don't call back to the PE side to allocate new samples, and it's really not clear to me that calling back to the PE side is what we want. But again, these are only the interactions that I can foresee; if there's others, it's not really clear to me what they are and why.
Just recently someone was put off from using it in new code...
I've missed that probably, in which context? Was the discussion public?
It wasn't public, no. The context was basically for a VfW decompressor, and the question was brought up to me privately about how we should implement it.