[Asterisk-code-review] func_jitterbuffer: Add audio/video sync support. (...asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/12869 )

Change subject: func_jitterbuffer: Add audio/video sync support.
......................................................................


Patch Set 2:

(3 comments)

https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c 
File main/abstract_jb.c:

https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c@948 
PS1, Line 948: 	timestamp_diff = (frame->ts * (rtp_get_rate(frame->subclass.format) / 1000)) - stream_sync->timestamp;
> Should this be using 'rate' vs rtp_get_rate?
Alas, no. The specific part converts the timestamp into the "proper" timestamp. Audio differs from what is received in RTP, while video does not. Using rtp_get_rate ensures that the video part calculates correctly.


https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c@1009 
PS1, Line 1009: 			struct ast_rtp_rtcp_report *rtcp_report = frame->data.ptr;
              : 			struct jb_stream_sync *stream_sync = NULL;
              : 
              : 			/* Determine which stream this RTCP is in regards to */
              : 			if (framedata->audio_stream_id == frame->stream_num) {
              : 				stream_sync = &framedata->audio_stream_sync;
              : 			} else if (framedata->video_stream_id == frame->stream_num) {
              : 				stream_sync = &framedata->video_stream_sync;
              : 			}
              : 
              : 			if (stream_sync) {
              : 				/* Store the RTP and NTP timestamp mapping so we can derive an NTP timestamp for each frame */
              : 				stream_sync->timestamp = rtcp_report->sender_information.rtp_timestamp;
              : 				stream_sync->ntp = rtcp_report->sender_information.ntp_timestamp;
              : 			}
> Not a fan of the rtp engine dependency introduced here. […]
Yeah, not really possible except by defining some generic mechanism to indicate synchronization from the various sources with a timing clock source. If we get to that point this logic could be moved there and this updated to use the new functionality.


https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c@1034 
PS1, Line 1034: 				ast_log(LOG_NOTICE, "Returned %d\n", frame->seqno);
> This seems potentially spammy? Maybe should be a debug message? Also you might want to add more info […]
Debug, yo. Will remove.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/12869
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I3fd75160426465e6d46bb2e198c07b9d314a4492
Gerrit-Change-Number: 12869
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Mon, 16 Sep 2019 17:47:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190916/5ac15758/attachment.html>