Conversation
|
An early comment since it's still a draft. Perhaps removing the |
Sure, actually early feedbacks is one of the goal of creating this draft.
Yes, the sync module is generic for any media, especially delivered via RTP/RTCP and using a shared/synchronized NTP source. IMO the 'av_sync' prefix will enhance intuitiveness, i.e: it sounds related to presentation time synchronization. While 'sync' prefix may be somewhat ambiguous, e.g: what to sync?. I recall how 'ssl_sock' term was chosen over |
|
The term should be inter-media synchronization, but yes, it's not quite as catchy, so I suppose it's okay if the 'av' stays. |
The jitter buffer is designed for audio, while in video it is just a normal/dummy buffer. Hence managing delay in audio stream is much easier than in video stream. As normally video presentation is later than audio, it should be fine to adjust the delay in audio stream only for the synchronization.
pjmedia/src/pjmedia/av_sync.c
Outdated
| ms_diff = ntp_to_ms(&ntp_diff); | ||
|
|
||
| /* Smoothen and round down the delay */ | ||
| ms_diff = ((ms_diff + 19 * media->smooth_diff) / 200) * 10; |
There was a problem hiding this comment.
Should an upper bound considered? Where does the magic numbers come from?
There was a problem hiding this comment.
Just added the upper bound 60s in the last commit, actually not really sure if it is needed.
The magic numbers are weighting for the smoothing, i.e: last delay uses weight of 19, newly received delay uses weight of 1, then divide the sum by 20 for the new delay. Instead of by 20, it is by 200 then multiplied to 10 for rounding down to nearest 10.
Thanks for the feedback.
|
Ready to review perhaps :) Currently the delay adjustment is implemented in audio stream only, because:
|
Sorry, quick question about that: Are there any specific plans to implement this (more specifically: RTT with T.140)? |
|
Yes, I'm currently implementing it. The basic functionality is already working (can send and receive text), but there's still a lot more to do, such as redundancy, docs, etc. |
If this work available publicly? From a quick glance I couldn't find a proper branch. |
|
Not yet public. Will create a branch early next week. |
sauwming
left a comment
There was a problem hiding this comment.
CR first stage, I haven't checked av_sync.c
pjsip/src/pjsua-lib/pjsua_media.c
Outdated
| sizeof(call->media_prov[0]) * call->med_prov_cnt); | ||
|
|
||
| /* Create synchronizer */ | ||
| if ((maudcnt+mvidcnt) > 1 && !call->av_sync) { |
There was a problem hiding this comment.
Do we need pjsua config to enable/disable the sync, as well as to choose which media will be sync?
There was a problem hiding this comment.
Yes, enable/disable should be needed. As this is a new feature, I guess we can postpone fine-tuning settings such as selecting which media to sync, allowing two or more synchronizers in a call, we can add the settings later based on user feedback?
There was a problem hiding this comment.
The thing is, I'm not sure if text stream should be synchronized by default, so that's why I proposed the setting to let the user decide.
Having said that, I'm not sure whether we should also implement text sync, so I suppose enable/disable is sufficient for now, with doc noting that it currently only applies for audio and video.
There was a problem hiding this comment.
In the future, text sync may be useful for streaming a video with subtitle (may require something like text-player, similar to wav/video-player). However, for now I think having text sync is not so urgent.
Just to add a sample scenario of two or more synchronizers in a call, in a conference call, there can be two sets of audio+video stream (so totally there will be 4 streams), rather than synchronize all 4 streams using one synchronizer, perhaps it is better to employ a separate synchronizer for each set (due to different original sources, they may have different codec delay, network delay, NTP source, etc). So in the future, when designing fine tune settings per media, perhaps we need to also consider this scenario.
Actually I was thinking the other way around. Because while it's true that video is initially played asap without buffering/prefetching, eventually it will be video that will be lagging due to its high bandwidth, and overall processing power required. So perhaps it should be tested in such scenario, for example using HD video and/or minimal connection speed such as mobile data. |
Actually the description above is a bit misleading, it was supposed to explain that video stream does not have delay management and implementing it is not so simple for now because the jitter buffer does not really help. It needs to be implemented in the video stream itself from scratch. In audio, the optimal delay calculation is already done by jbuf and increasing delay can be easily inserted into jbuf. Yes, in real world the video tends to lag behind the audio (from codecs & delivery aspects). That's why it is sufficient to have delay adjustment implemented only in audio, as mostly we need to add delay to audio (from its optimal delay). Btw, adding delay to video is perhaps not so complex or risky, will try to implement it. |
Also add safety net to ignore any excessive delay request from avsync (i.e: > 5 seconds).
|
I'm not sure whether the video "delay" adjustment is completed, but it seems that the current patch can only add/decrease delay to the video? I believe a more realistic usage would be to speed up the video, i.e. by dropping or skipping frames. In other words, instead of adding delay to audio, which will increase the latency/lag of the entire av streams, there can be another option for the user, which is to speed up the video, so the streams still feel like real time. |
Also move speed-up markup by 4/3 to the synchronizer (was in each stream)
Previously I assume that video decoding is always done as fast as possible, no room for decreasing the delay. However, after implementing your idea here, the log showed some frames being skipped. |
|
Next, will try to integrate the AV sync to AVI player in the source side (for streaming & local playback). Currently the exising AVI player synchronization seems to be done in the rendering side (for local playback only?). |
|
Right, currently sync is only done in aviplay. |
There was a problem hiding this comment.
Copilot reviewed 19 out of 24 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- pjmedia/build/Makefile: Language not supported
- pjmedia/build/pjmedia.vcproj: Language not supported
- pjmedia/build/pjmedia.vcxproj: Language not supported
- pjmedia/build/pjmedia.vcxproj.filters: Language not supported
- pjsip-apps/src/swig/symbols.i: Language not supported
| pjmedia_port_destroy(&fport[i]->base); | ||
| } | ||
|
|
||
| if (*p_streams && (*p_streams)->avsync) { |
There was a problem hiding this comment.
In the cleanup block where pjmedia_av_sync_del_media is called, a NULL is passed as the synchronizer parameter. To ensure proper removal of media from the synchronizer, pass the actual AV sync instance (e.g., (*p_streams)->avsync) instead of NULL.
How to use
For app using PJSUA-LIB
By default it is enabled for audio & video calls. It can be disabled per call basis by setting
PJSUA_CALL_NO_MEDIA_SYNCinpjsua_call_setting.flag.For app using PJMEDIA
pjmedia_av_sync_create().pjmedia_av_sync_add_media(). If the media is a audio/video stream, usepjmedia_stream_common_set_avsync()instead.pjmedia_av_sync_update_ref().port.get_frame(), update AV sync usingpjmedia_av_sync_update_pts(), the function may request delay adjustment: increase or decrease delay.pjmedia_av_sync_del_media().pjmedia_av_sync_destroy().For app using PJMEDIA AVI File Player
By default it is enabled. It can be disabled by setting
PJMEDIA_AVI_FILE_NO_SYNCflag in creating AVI file player.Global macro settings
Configurable via
config_site.h.Logic in
pjmedia_av_sync_update_pts()PJMEDIA_AVSYNC_MAX_SPEEDUP_REQ_CNT) and the lag is still beyond a tolerable value (i.e: configurable viaPJMEDIA_AVSYNC_MAX_TOLERABLE_LAG_MSEC), the function will issue slow down request to the fastest media (which set the AV sync's maximum PTS).