-
Notifications
You must be signed in to change notification settings - Fork 282
feat: add frame_converter and provide consumers opaque frame type #1609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: add frame_converter and provide consumers opaque frame type #1609
Conversation
a108a36
to
3562feb
Compare
This removes the `image_data` on `const_frame` and replaces it with an opaque `image_ptr`. Consumers can use the `frame_converter` to convert that into a pixel buffer. For now this is the same basic texture to buffer copy as before, but will soon be replaced with a format conversion step
c6e2afc
to
e5dbb26
Compare
I like this in principle. Other than that I really like what this enables, and can't wait for it to land |
Yeah I am a little worried about potential performance impact, but I also wonder if it will just be fine? I havent done any performance testing though, just some light usage. The theory I have in my mind from the code is that this should be minimally different, a key thing is that I have made sure to queue the download before buffering the frames in consumers. I am currently thinking of releasing 2.5.0 any day now. |
I think an architecture where we isolate the usage of the frame_converter from the consumers would be cleaner and keep the complexity of consumers down. Something like this:
Pros:
The total complexity would not be greater than the current implementation in the PR. |
I did consider doing something like that, but decided against it for some reasons that I don't 100% remember. The other part of the reason was that I think it will a little complexity to consumers, as they will need to declare the array of frames they need. And then when consuming the frames will need to guard against the frames vector being the wrong length. At the time, I didn't have an idea on how to do this and it felt like it would become a mess, but maybe it won't be as bad as I was worried about.
Yeah it would do that better/cleaner, I have tried to handle this today with some caching, but I acknowledge that it could be prone to race conditions resulting in an unnecessary cache miss, because of the downloads being scheduled on different thread. I felt that a cache miss was better than wrapping it in a mutex.
I don't think it would change anything. The current approach of this PR allows consumers to download as many versions of the frame they want. All the subregion logic is being moved into this download step, so I don't see what would be any better
I'm not 100% convinced that will be the case. I suspect its going to add a bunch of complexity to the channel (output?) at the point where it needs to do the conversions. I am also curious what the performance implications would be. Doing the conversions in the channel would mean that (unless a new thread is introduced here for this purpose, with some handling for race conditions) some part of the channel will need to block while these downloads are being done, which goes against the way the mixer is pipelining the compositing? |
This is the first polished portion of #1581
This removes the
image_data
onconst_frame
and replaces it with an opaqueimage_ptr
. Consumers can use theframe_converter
to convert that into a pixel buffer.For now this is the same basic texture to buffer copy as before, #1610 will replace that with a proper conversion step
Most of the consumers need testing to make sure they work and don't crash immediately. ndi and ffmpeg appear to not work for me, but the same is true with master so is probably something weird on my machine today..