[Asterisk-code-review] app stream echo: Added a multi-stream echo application (asterisk[master])

[Kevin Harwell]

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

Change subject: app_stream_echo: Added a multi-stream echo application
......................................................................


Patch Set 2:

(4 comments)

https://gerrit.asterisk.org/#/c/5629/2/apps/app_stream_echo.c
File apps/app_stream_echo.c:

PS2, Line 177: 			if (f->subclass.integer == AST_CONTROL_VIDUPDATE && !update_sent) {
             : 				if (stream_echo_write(chan, f, one_to_one, type)) {
             : 					ast_frfree(f);
             : 					return -1;
             : 				}
             : 				update_sent = 1;
> I'm unsure on this, but should this block be dependent on the update_sent v
Maybe not? I pulled this behavior straight from app_echo. I tend to agree with you though, so I'll remove it unless someone else has a good reason why it should be there.


Line 199: 		    stream_echo_write(chan, f, one_to_one, type)) {
> Should failure to write a single frame result in the application being term
Again, this behavior was pulled from app_echo. I'm fine though with it not terminating due to a bad write. However, what should the threshold be for termination when a frame is not written? 5 in a row? 10 failures total? Something else?


PS2, Line 273: 	topology = ast_stream_topology_alloc();
             : 	if (!topology) {
             : 		return -1;
             : 	}
             : 
             : 	if (stream_echo_topology_append_streams(topology, num, type)) {
             : 		ast_log(LOG_ERROR, "Unable to append '%u' streams of type '%s' to"
             : 			" the topology\n", num, ast_codec_media_type2str(type));
             : 		ast_stream_topology_free(topology);
             : 		return -1;
             : 	}
> I feel like the algorithm here is a bit off. Consider if a channel were to 
I like what you have suggested here better I think. Originally, the plan was to just do 'n' streams of the type specified (at least from my interpretation). My only worry is the increasing complexity as more streams of a type are included.

What about doing what you suggest, but limiting it somewhat. Like what would you think of keeping it to a single stream of each type from the original offer, but multiply only the stream of the type specified and dropping any other streams of that type that were originally included. So for instance in your second scenario of 1 original audio and 2 original video streams (1 & 2) then video stream 2 would be dropped and video stream 1 would be cloned 'n' times?


PS2, Line 287: 	if (ast_channel_get_stream_topology(chan) != topology) {
             : 		ast_stream_topology_free(topology);
             : 	}
> Should this attempt to restore the channel's original topology when the app
I lean toward no right now. It adds complexity that may not be needed. If the case arises where we need to then it can be added in later. Unless you currently have a specific case in mind?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I254144486734178e196c7f590a26ffc13543ff2c
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes