-
Notifications
You must be signed in to change notification settings - Fork 337
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
kvssink soft start/stop support #1153
base: develop-pre-3.4.1
Are you sure you want to change the base?
Conversation
a4e9380
to
34a6c71
Compare
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.
Awesome!
src/gstreamer/gstkvssink.cpp
Outdated
LOG_WARN("Failed to put EOFR for " << kvssink->stream_name); | ||
} | ||
} else { | ||
LOG_WARN("NULL pointer! Failed to put EOFR for " << kvssink->stream_name); |
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.
Saying null ptr in warn looks scary & looks like an error. This is handling the case where kinesis_video_stream failed to initialize?
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.
Yes and as a safety measure in case either of these two pointers get nulled. This correlates with a STATUS_NULL_ARG error we get from Producer for these kind of null checks. How does "Null argument, failed to put EOFR for " sound?
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'm thinking that we should just make this an INFO log and say what it's doing literally.
"Kinesis Video Stream failed to initialize, not putting EOFR".
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'm still leaning towards having it be a WARN since it 'failed' to initialize. @disa6302 any thoughts?
Issue #, if available:
What was changed?
autovideosrc
) and test (videotestsrc
) GStreamer sourcesWhy was it changed?
x264enc
element will send null buffers to itself to clear its queue of buffered frames upon receiving an EOS event, so freeing KVS resources upon a null buffer would not allow for soft stops in this fashion.How was it changed?
The following is the flow of the new intermittent sample:
What testing was done for the changes?
Testing for data loss was conducted by streaming footage of a running stop watch at a start/stop period of less than a fragment (6, 8, and 10 second intervals were tested). The time segment captured by the ingested KVS stream was compared to the desired start/stop increments. All fragments were within 100ms of the expected duration, except for the first fragments of streams which were ~300ms shorter than expected, likely due to time taken constructing resources.
Testing was also conducted with longer increments of 2min playing and 5min paused. The stream continued to stop and start with no issues. Below shows 1 hour of such a test with the orange-colored timeline showing where there were ingested fragments.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.