[Asterisk-code-review] res rtp asterisk: Add support for receiving and handling NAC... (asterisk[15])

[Joshua Colp]

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

Change subject: res_rtp_asterisk: Add support for receiving and handling NACK requests.
......................................................................


Patch Set 2: Code-Review-1

(9 comments)

This should also have an ASTERISK issue created for it.

https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@95
PS2, Line 95: 
            : #define DEFAULT_RTP_BUFFER_SIZE 100
I think 100 may be too large for now. I think 30 is more reasonable.


https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@112
PS2, Line 112: #define RTCP_PT_RTPFB	AST_RTP_RTCP_RTPFB
Since it's new code you can go ahead and use AST_RTP_RTCP_RTPFB directly. I used a define previously to reduce the size of the change.


https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@519
PS2, Line 519: struct ast_rtp_rtcp_nack_payload {
This should also be documented.


https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@520
PS2, Line 520: 	unsigned char buf[0];
We normally place zero sized arrays at the end of the structure as the last member.


https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@4411
PS2, Line 4411: 		if (ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_RETRANS_SEND) && rtp->send_buffer) {
You don't need to check the prop as well here, just checking for rtp->send_buffer is reasonable.


https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@5262
PS2, Line 5262: 		return res;
It may be useful to add a debug message here if this scenario occurs.


https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@5267
PS2, Line 5267: 	for (j = 3; j < length; j++) {
Document why magic number 3 is used.


https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@5274
PS2, Line 5274: 		}
If the payload wasn't found I think it would be useful to have a debug message too.


https://gerrit.asterisk.org/#/c/8773/2/res/res_rtp_asterisk.c@5709
PS2, Line 5709: 				if (!ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_RETRANS_SEND)) {
              : 					break;
              : 				}
Would it make sense to also just use rtp->send_buffer here for a check?



-- 
To view, visit https://gerrit.asterisk.org/8773
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f7f124af3b9d5d2fd9cffc6ba8cb48a6fff06ec
Gerrit-Change-Number: 8773
Gerrit-PatchSet: 2
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Mon, 16 Apr 2018 11:08:18 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180416/c279d60f/attachment.html>