-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix input queueing when some of the inputs do not produce frames/samples #625
Fix input queueing when some of the inputs do not produce frames/samples #625
Conversation
2f0ecbb
to
61c5021
Compare
61c5021
to
ca75417
Compare
ca75417
to
fac9f7f
Compare
// ignore result, we only need to ensure samples are enqueued | ||
self.check_ready_for_pts(pts_range, queue_start); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's awful. I hate it. Over and over we rely on some internal behavior of this function (enqueuing frames / reading them from the channel).
I'll try try to simplify this logic, cause reading all the queue code slowly feels like reading brainfuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tradeoff we can either:
- check_ready_for_pts even though we don't need return value
- write function that is almost identical, or inline that logic inside this function
- ensure that when we check all inputs that
check_ready_for_pts
is always called for all inputs
My reasoning here is that:
- approach 1 is very safe, because it's "public` method that can be used atomically. Safe here means that it does not break anything, but still does not grantee that enqueue will happen inside.
- approach 2 would be more readable, but generated more code, or required risky refactor. It might be good idea for follow up, I'm open if you want to try to improve that, but note that this is more tricky than it seems and big rewrite is more likely to introduce more bugs.
- approach 3, Similar to approach 1, but worse because we need to be careful that checking port happens before get_frames
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer having a function like get_frames
and calling it here and in the check_ready_for_pts
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would not work with current architecture:
- get frames needs to return function whether it's ready or not
- currently get frames relies that returned frame will be used (it's needed for example to check whether EOS was sent)
And if you are suggesting to change the above, then your suggestion boils down to almost full rewrite of queuing code.
We can talk about it more at the office, topic is to complex for discussion via comments.
Steps to reproduce:
What happens:
Alternative approach could be to replace call to
any
https://github.com/membraneframework/live_compositor/blob/master/compositor_pipeline/src/queue/video_queue.rs#L103 with non-lazy alternative, but I think this is cleaner.