<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/12869">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c">File main/abstract_jb.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c@915">Patch Set #1, Line 915:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static int rtp_get_rate(struct ast_format *format)<br>{<br> /* For those wondering: due to a fluke in RFC publication, G.722 is advertised<br> * as having a sample rate of 8kHz, while implementations must know that its<br> * real rate is 16kHz. Seriously.<br> */<br> return (ast_format_cmp(format, ast_format_g722) == AST_FORMAT_CMP_EQUAL) ? 8000 : (int)ast_format_get_sample_rate(format);<br>}<br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I've seen a similar check elsewhere in the code. Seems like this might be worth moving to a more central location.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c@948">Patch Set #1, Line 948:</a> <code style="font-family:monospace,monospace"> timestamp_diff = (frame->ts * (rtp_get_rate(frame->subclass.format) / 1000)) - stream_sync->timestamp;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this be using 'rate' vs rtp_get_rate?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c@1009">Patch Set #1, Line 1009:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct ast_rtp_rtcp_report *rtcp_report = frame->data.ptr;<br> struct jb_stream_sync *stream_sync = NULL;<br><br> /* Determine which stream this RTCP is in regards to */<br> if (framedata->audio_stream_id == frame->stream_num) {<br> stream_sync = &framedata->audio_stream_sync;<br> } else if (framedata->video_stream_id == frame->stream_num) {<br> stream_sync = &framedata->video_stream_sync;<br> }<br><br> if (stream_sync) {<br> /* Store the RTP and NTP timestamp mapping so we can derive an NTP timestamp for each frame */<br> stream_sync->timestamp = rtcp_report->sender_information.rtp_timestamp;<br> stream_sync->ntp = rtcp_report->sender_information.ntp_timestamp;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/12869/1/main/abstract_jb.c@1034">Patch Set #1, Line 1034:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_NOTICE, "Returned %d\n", frame->seqno);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This seems potentially spammy? Maybe should be a debug message? Also you might want to add more information unless you meant to delete this?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/12869">change 12869</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/12869"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I3fd75160426465e6d46bb2e198c07b9d314a4492 </div>
<div style="display:none"> Gerrit-Change-Number: 12869 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joshua C. Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 13 Sep 2019 21:34:33 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>