[apps] FIX srt-live-transmit stops listening after SRT disconnect #2997... #3108
Conversation
maxsharabayko
added
Type: Bug
Co-authored-by: Maxim Sharabayko <[email protected]>
Co-authored-by: Maxim Sharabayko <[email protected]>
Co-authored-by: Maxim Sharabayko <[email protected]>
| @@ -712,6 +713,9 @@ class ConsoleSource: public Source | |||
| // The default stdin mode on windows is text. | |||
| // We have to set it to the binary mode | |||
| _setmode(_fileno(stdin), _O_BINARY); | |||
| #else | |||
| const int fd = fileno(stdin); | |||
| may_block = fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_NONBLOCK) < 0; | |||
There was a problem hiding this comment.
That doesn't make any sense. If this fails (best from the constructor), it should make the application exit. Error from this call is unlikely.
There was a problem hiding this comment.
Typically, when implementing an event loop for reading, the pattern is to use non-blocking file descriptors and continue reading until errno == EAGAIN is encountered.
If it’s not possible to make the file descriptors non-blocking, an alternative approach is to make a single read() call for each “read” event. While this approach is not optimal due to the increased number of system calls, it still ensures the process functions as expected from the user’s perspective. I prefer this pragmatic approach over halting the process just because it cannot run in an optimal manner.
| @@ -800,9 +843,10 @@ int main(int argc, char** argv) | |||
|
|
|||
| dataqueue.push_back(pkt); | |||
| receivedBytes += pkt->payload.size(); | |||
| if (src->MayBlock()) | |||
| break; | |||
There was a problem hiding this comment.
I think that actually reading should be done this way: either you have the device read-ready, so you read, and then after you read, you don't know, and have to recheck.
Or, you can resolve to reading multiple times, counting on that when particular time reading isn't ready, then the Read call should report an error. Might be, I think, a good idea, to keep the "blocked" state in the fields, which will be written to, in case when particular Read implementation finds out that the call failed due to not being ready. This way it won't need to see if this is SRT and this way we use that function to get the error and maybe check for an SRT-specific readiness failure.
There was a problem hiding this comment.
From what I understand, the current implementation already follows this approach. For file descriptors in blocking mode (case 1), it checks if the device is ready before attempting to read, performs the read operation, and rechecks the state afterward since readiness isn’t guaranteed. For file descriptors in non-blocking mode (case 2), it handles multiple read attempts and relies on the error returned by the Read call to detect if the device isn’t ready.
Could you clarify if there’s something specific you’d like me to adjust or add to the current implementation? Perhaps there’s a particular scenario or behavior you’d like to address that isn’t currently handled?
There was a problem hiding this comment.
This here is unclear. First, MayBlock() actually represent the state whether setting the nonblocking mode could be done, which is useless. If the architecture requires nonblocking mode, it should be nonblocking always, and all devices should be operated as such. The case if operating the console device in blocking mode should not be even taken into account.
The only thing I'm referring to is the approach to multiple reading calls, which should follow one of two methods:
- Check if read-ready always before calling Read(), and break the loop if it isn't
- Check if read-ready once before the loop, then call Read() in loop until Read() informs you that reading is no longer possible
Note that the second approach isn't possible to be used reliably in case of blocking mode, that's why it should not be taken into account.
There was a problem hiding this comment.
I understand the benefits of enforcing non-blocking mode for consistency and simplifying the architecture. However, I think it might be preferable to maintain support for both blocking and non-blocking modes. This flexibility ensures compatibility with a broader range of sources and use cases, particularly for systems where blocking mode is either required or more practical.
The current implementation can differentiate the handling of each mode:
• For blocking mode: Always check read-readiness before calling Read().
• For non-blocking mode: Check read-readiness once before the loop and perform multiple Read() calls until it’s no longer possible.
Would you be open to maintaining support for both modes, or do you see specific challenges in doing so?
... during no active input (FIX #2997)
There are two primary reasons for this issue: