fix: limit video device open attempts to prevent infinite loops #998
Conversation
adamshiervani
changed the title
…ter troubleshooting
…mproved stability
IDisposable
left a comment
IDisposable
left a comment
There was a problem hiding this comment.
While I understand the idea of this change is to give up completely if we can't safely open things up, a review of this code shows a lot of issues with resources that might be partially setup not being torn down correctly.
| attempts++; | ||
| } | ||
| if (!streaming_stopped) { | ||
| log_error("video streaming thread did not exit after 30s"); |
There was a problem hiding this comment.
is it worthwhile doing an exit(1) here too?
| if (open_attempts > MAX_OPEN_ATTEMPTS) { | ||
| log_error("Tried to open video device %d times without success. " | ||
| "Exiting to trigger supervisor restart.", MAX_OPEN_ATTEMPTS); | ||
| exit(1); |
There was a problem hiding this comment.
Maybe better as return (void *)errno (as that contains the error number from the last failed operation)
| struct timeval tv; | ||
| int r; | ||
| uint32_t num = 0; | ||
| VIDEO_FRAME_INFO_S stFrame; |
There was a problem hiding this comment.
By this point, all the setup work succeeded, but we still haven't gotten a valid frame, but we HAVE allocated memory that must be released.
|
|
||
| if (ioctl(video_dev_fd, VIDIOC_QBUF, &buf) < 0) | ||
| log_error("failure VIDIOC_QBUF: %s", strerror(errno)); | ||
| } |
There was a problem hiding this comment.
else open_attempts = 0; because we've safely handled a frame? Otherwise we could slowly accumulate failing counts, while actually still running mostly right.
| stFrame.stVFrame.enCompressMode = COMPRESS_MODE_NONE; | ||
| bool retried = false; | ||
| retry_send_frame: | ||
| if (RK_MPI_VENC_SendFrame(VENC_CHANNEL, &stFrame, 2000) != RK_SUCCESS) |
There was a problem hiding this comment.
Do we want a cap on the attempts to send the frame too? Seems like we could get hung up here otherwise.
| { | ||
| log_error("Set format fail: %s", strerror(errno)); | ||
| usleep(100000); // Sleep for 100 milliseconds | ||
| close(video_dev_fd); |
| close(video_dev_fd); | ||
| return (void *)errno; | ||
| } | ||
|
|
| log_info("running video stream"); | ||
|
|
||
| streaming_stopped = false; | ||
|
|
There was a problem hiding this comment.
bool stream_on == false;
| { | ||
| if (buffers[i].mb_blk != NULL) | ||
| { | ||
| RK_MPI_MB_ReleaseMB((buffers + i)->mb_blk); |
There was a problem hiding this comment.
Since this never happens if we return out as some paths currently do, we would leak memory if the memory was allocated and we return out...
| req_free.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; | ||
| req_free.memory = V4L2_MEMORY_DMABUF; | ||
|
|
||
| if (ioctl(video_dev_fd, VIDIOC_REQBUFS, &req_free) < 0) |
There was a problem hiding this comment.
note to self This actually allocates a zero-count of buffers, dropping what we had above
if (video_dev_fd > 0 && ioctl(video_dev_fd, VIDIOC_REQBUFS, &req_free) < 0)
|
When we enter sleep mode, we get a signal but no frames. When we request a frame, it times out after a second and then retries forever. The proper fix is to stop the video before we enter sleep mode, so we don't need to handle this special case. See #999 |
Fix video thread infinite restart loop on signal loss
Problem: When display sleeps, video thread can loop indefinitely trying to restart capture, causing:
Root cause: Race between fast polling video thread and slow event-driven format detection thread. Video thread sees stale
detected_signal=trueand keeps retrying before format thread updates it.Solution:
detected_signalto stop the loopvideo_start_streaming()to allow UI restart