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

[Mark Michelson]

Mark Michelson 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: Code-Review-1

(6 comments)

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

Line 74: 	ast_frame_type2str(frame->frametype, frame_type, 32);
nit: Use sizeof(frame_type) here instead of 32.


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 variable? I would think that if you get a VIDUPDATE frame that you should probably send it even if you previously have sent a VIDUPATE.


Line 199: 		    stream_echo_write(chan, f, one_to_one, type)) {
Should failure to write a single frame result in the application being terminated?


PS2, Line 215: 	for (i = 0; i < num; ++i) {
             : 		struct ast_stream *stream = ast_stream_alloc(NULL, type);
             : 
             : 		if (!stream) {
             : 			return -1;
             : 		}
             : 
             : 		ast_stream_set_state(stream, i == 0 ?
             : 			AST_STREAM_STATE_SENDRECV : AST_STREAM_STATE_RECVONLY);
             : 
             : 		if (ast_stream_topology_append_stream(topology, stream) < 0) {
             : 			ast_stream_free(stream);
             : 			return -1;
             : 		}
             : 	}
This does not set formats on the streams being created. You probably should just use ast_stream_clone() to create the streams to append to the topology.


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 originally negotiate an audio and a video stream. Then the channel enters StreamEcho and says "I want 3 video streams echoed back to me." The result is that you'd end up renegotiating the topology on the channel to be 3 video streams. But the audio stream that was there before is now gone.

Similarly, if the channel originally negotiated an audio stream and 2 video streams, and says "I want 3 video streams echoed back to me," then again the result seems off. Again the audio stream would be gone, but then what would the expected behavior be for the video streams? Is only one of them being echoed out? Are both streams being echoed out? And if so, where do the frames from each video source go?

I think what would work better is something like the following:

1) Create the empty topology.
2) Append clones of the channel's original streams to this topology.
3) For each stream in the channel's original topology that matches the requested type, create N stream clones of that type.

So let's say the channel originally negotiated one audio and one video stream, then asks StreamEcho to echo 3 video streams. In this case, I think that means that the resulting topology will have:
* The original audio stream
* The original video stream
* 2 more video streams, each clones of the original video stream. This assumes that the original stream counts towards the total number of video streams requested.

Then, frames from the audio stream get echoed back out the audio stream. Frames from the original video stream get echoed out the original video stream as well as the 2 additional streams that were appended.

And in the case where the channel originally negotiated one audio and two video streams, the resulting topology would be the following:
* The original audio stream
* Original video stream 1
* Original video stream 2
* 2 more clones of video stream 1
* 2 more clones of video stream 2

Frames that come in on the audio stream get echoed back out the audio stream. Frames that come in on original video stream 1 get echoed back on it and on its 2 clones. Frames that come in on original video stream 2 get echoed back on it and on its 2 clones.

Let me know what you think.


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 application exits?


-- 
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: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes