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

[Kevin Harwell]

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

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


Patch Set 1: Code-Review-1

(4 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@915 
PS1, Line 915: static int rtp_get_rate(struct ast_format *format)
             : {
             :         /* For those wondering: due to a fluke in RFC publication, G.722 is advertised
             :          * as having a sample rate of 8kHz, while implementations must know that its
             :          * real rate is 16kHz. Seriously.
             :          */
             :         return (ast_format_cmp(format, ast_format_g722) == AST_FORMAT_CMP_EQUAL) ? 8000 : (int)ast_format_get_sample_rate(format);
             : }
             : 
I've seen a similar check elsewhere in the code. Seems like this might be worth moving to a more central location.


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?


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. Unfortunately not sure how to alleviate that except either by modifying the frame, or using callbacks. The latter being more work. meh no great options here I guess.


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 information unless you meant to delete this?



-- 
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: 1
Gerrit-Owner: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Fri, 13 Sep 2019 21:34:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190913/becfb99c/attachment-0001.html>