-
Notifications
You must be signed in to change notification settings - Fork 56
Make sure we terminate an ingress session in case sending on the app source doesn't cause the pipeline to emit an EOS event #379
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
Conversation
Were these causing some of the e2e failures which said something like |
} | ||
|
||
s.result <- err | ||
select { |
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.
trying to understand this part better - I assume this is done to avoid blocking - and since the channel is already buffered (1) - where else we have writing to the channel? Is whip different in that regard as try send isn't used there?
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 change should not be needed with the code as it stands. However, without this the code is not idempotent, and we have a risk of getting a deadlock if we ever (potentially after a code change somewhere else) have a case where we write to the channel more than once.
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.
👍 - updating whip appsrc with the same pattern sounds good then
would also be good to have pipeline state changes logged so we get more info when this happen to narrow down the source of the issue - I created a small PR for that: #380 |
Yes. e2e is catching the pipeline not shutting down with RTMP, as it should. |
No description provided.