-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Timestamp correction for janus-pp-rec #2573
Conversation
👍 |
Thanks for the contribution and the detailed insight on this, it's very appreciated!
|
Thanks for your insights! I'll try and fix those things as soon as I can. |
I updated the manual page as requested and tried to fix the coding style as best as I could. |
@tbence94 thanks for the effort and the very good presentation of the implementation.
Have you tried to solve the original issue with ASD ? Would you mind sharing a sample in order to validate the algorithm ? |
Thank you for the comment. About audioskew and how did we get here...As a matter of fact we tried to solve the issue with
We implemented a solution using where we checked the audio and video duration with But we also had streams with larger gaps (more than 10s) and multiple gaps (2-3x 10s). First we experimented with the values you recommended Since these lipsync problems seem to be high priority issues for our clients we needed needed a solution where these kind of gaps were filled with silence instead of "streching" the audio. Therfore I created this AnswersYou work in the time domain with double variables, while ASD works in RTP domain with integers You use a So we are not going back constantly to get the mavg. Only if there really seems to be a "big enough" jump in latency. I think we could sacrifice some memory and create a solution where we store the N previous latency values so we could avoid back tracking. I created this version becuase I thought it would be easier to calculate this way (only when we need it). Have you tried to solve the original issue with ASD ? Would you mind sharing a sample in order to validate the algorithm? |
If you expect people to test this and provide feedback, we'll need an mjr file that is publicly available. |
PS: apologies for the late reply, we've been busy with the IETF meeting. |
Hello. Thanks for your reply. I can attach a short example. It includes the converted (original / faulty) result. As you can see there is a part when you here him counting (around If I remember correctly we produced this by calling the phone while streaming but we did not take the call. I don't have too much time myself but let me know if I can help somehow to speed this up and I'll try to find time for this. |
Thanks for the sample! We'll look into this, and do a more in depth review of your PR as well 👍 |
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 seems a valid enhancement in general. Still I have left some comments, check them out.
@tbence94 any feedback on Alessandro's note above? Thanks! |
Sorry, but I am really swamped with work right now. I will try my best to find some time to answer... |
Thanks @tbence94 for the changes. |
@tbence94 sorry I just remembered why I was mentioning seq nums fixing.
This is because when dealing with opus, janus-pprec evaluates the number of silence packets according to the RTP ts difference. janus-gateway/postprocessing/pp-opus.c Lines 76 to 83 in fee03d4
As you can see that will lead to a confusing situation where:
I guess this is not a big deal, since the user is being informed of the restamping algorithm kicking in.
I'm not sure this would need a fix somewhere in the PR on in pprec code though. |
@atoppi This purpose of this PR is to shift the timestamps and write silences. In this case the original video should contain silences there... And as you said we informed the user about the restamping. Plus it's still an optional feature which is disabled by default. So the user can passing in the flags should probably expect these silences. |
Yeah probably this not an issue, but still worth mentioning. In audio skew when the rtp timestamps are being increased due to a slow media clock, we also adjust sequence numbers, in order to obtain a coherent "fake" packet loss before letting pp-rec generate silence packets. As I said not a big deal, but it might become an issue in case pp-rec will handle some audio codecs by seq nums and not rtp timestamps like we did before on g711/g722. In that case no silence will be generated. |
Sounds like something we can address later on if issues arise: in the meanwhile, it looks good to me, so merging. Thanks again! |
* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c
* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c
* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c
* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c
* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c
* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c
* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c (cherry picked from commit 1827315)
* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c (cherry picked from commit 1827315) (cherry picked from commit b9318b7)
restamping1.zip i am trying to fix this with restamping options but it doesn't look like it does justice to all of the affected recordings, Please share the detail on how to use the restamp options properly to fix these type of issues |
any updates on last comment |
Summary
Devices can send wrong RTP timestamps after some kind of interruption.
For example mobile devices tend to send wrong timestamps after receiving a call.
This feature attempts to provide a solution for correcting wrong RTP timestamps in the recorded streams by adding some optional flags to
janus-pp-rec
.Problem
Use case
We managed to reproduce this issue with both iOS and Android devices by accepting a call while streaming.
The recorded and converted video stream was OK but the audio stream after the call was not in sync with the video.
This was due to the lack of silence in the audio stream. There should have been a gap in the timestamps at the call.
Example
Let's say there was a
5s
long call at10s
and the whole video is30s
long.The video stream looks like this (normal):
0s
to10s
15s
then it continues to grow to30s
But the audio stream looks like this (faulty):
0s
to10s
10s
then it continues to grow to25s
When we convert this using janus-pp-rec the output file will have a
30s
long video with5s
black-screen in it starting at10s
.But there is no gap in the audio timestamps so if we made any sound at
16s
(in reality) it will be replayed at11s
in the converted file.How to detect?
We can detect issues like this by comparing the RTP timestamps to the arrival time of the packets.
(Introduced here: #1719)
There is a sudden jump in latency after events like the one described in the use case section.
Solution
How to correct?
The current implementation provides a solution which compares the current latency (arrival time - timestamp) to the
moving average latency of the last 10 (default) packets. If the current latency is higher then then the moving average
multiplied with (the number provided as the
restamping
flag value divided by 1000) than we calculate an offset basedon the latency which will be added to the RTP timestamps starting from the current packet.
I also introduced a minimum threshold to avoid correction of smaller "jumps" in latency which might be cuased by network issues.
Although this is not a perfect solution it helps to create something that might be closer to the thing that was seen and heard by the people watching the live stream.
Flags and environment variables
restamp
If the latency of a packet is bigger than the
moving_average_latency * (<restamp>/1000)
the timestamps will be corrected.Disabled if
0
restamp-packets
Number of packets used for calculating moving average latency for timestamp correction.
restamp-min-th
Minimum latency of moving average to reach before starting to correct timestamps.
If the current latency is below this threshold the timestamps will not be changed.
Below the threshold we ignore the moving average.