[Asterisk-code-review] stream: Unit tests for stream read and tweaks framework (asterisk[master])

George Joseph asteriskteam at digium.com
Tue Feb 28 18:08:41 CST 2017


George Joseph has uploaded a new change for review. ( https://gerrit.asterisk.org/5109 )

Change subject: stream: Unit tests for stream read and tweaks framework
......................................................................

stream: Unit tests for stream read and tweaks framework

* Removed the AST_CHAN_TP_MULTISTREAM tech property.  We now rely
  on read_stream being set to indicate a multi stream channel.
* Fixed issue where stream and default_stream weren't being set on
  a frame retrieved from the queue.
* Now testing for NULL being returned from the driver's read or
  read_stream callback.
* Fixed issue where the dropnondefault code was crashing on a
  NULL f.
* Now enforcing that if either read_stream or write_stream are
  set when ast_channel_tech_set is called that BOTH are set.
* Added the unit tests.

ASTERISK-26816

Change-Id: If7792b20d782e71e823dabd3124572cf0a4caab2
---
M include/asterisk/channel.h
M main/channel.c
M main/channel_internal_api.c
M tests/test_stream.c
4 files changed, 308 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/09/5109/1

diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index f6e0925..211d645 100644
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -911,10 +911,6 @@
 	 * world
 	 */
 	AST_CHAN_TP_INTERNAL = (1 << 2),
-	/*!
-	 * \brief Channels with this particular technology support multiple simultaneous streams
-	 */
-	AST_CHAN_TP_MULTISTREAM = (1 << 3),
 };
 
 /*! \brief ast_channel flags */
diff --git a/main/channel.c b/main/channel.c
index e3e9561..f2de862 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -3944,13 +3944,17 @@
 			default:
 				break;
 			}
-		} else if (!(ast_channel_tech(chan)->properties & AST_CHAN_TP_MULTISTREAM) && (
-			f->frametype == AST_FRAME_VOICE || f->frametype == AST_FRAME_VIDEO)) {
-			/* Since this channel driver does not support multistream determine the default stream this frame
-			 * originated from and update the frame to include it.
-			 */
-			stream = default_stream = ast_channel_get_default_stream(chan, ast_format_get_type(f->subclass.format));
-			f->stream_num = ast_stream_get_position(stream);
+		} else if (f->frametype == AST_FRAME_VOICE || f->frametype == AST_FRAME_VIDEO) {
+			if (ast_channel_tech(chan) && ast_channel_tech(chan)->read_stream) {
+				stream = ast_stream_topology_get_stream(ast_channel_get_stream_topology(chan), f->stream_num);
+				default_stream = ast_channel_get_default_stream(chan, ast_format_get_type(f->subclass.format));
+			} else {
+				/* Since this channel driver does not support multistream determine the default stream this frame
+				 * originated from and update the frame to include it.
+				 */
+				stream = default_stream = ast_channel_get_default_stream(chan, ast_format_get_type(f->subclass.format));
+				f->stream_num = ast_stream_get_position(stream);
+			}
 		}
 	} else {
 		ast_channel_blocker_set(chan, pthread_self());
@@ -3970,7 +3974,7 @@
 			 * thing different is that we need to find the default stream so we know whether to invoke the
 			 * default stream logic or not (such as transcoding).
 			 */
-			if (f->frametype == AST_FRAME_VOICE || f->frametype == AST_FRAME_VIDEO) {
+			if (f && (f->frametype == AST_FRAME_VOICE || f->frametype == AST_FRAME_VIDEO)) {
 				stream = ast_stream_topology_get_stream(ast_channel_get_stream_topology(chan), f->stream_num);
 				default_stream = ast_channel_get_default_stream(chan, ast_format_get_type(f->subclass.format));
 			}
@@ -3980,25 +3984,13 @@
 			/* Since this channel driver does not support multistream determine the default stream this frame
 			 * originated from and update the frame to include it.
 			 */
-			if (f->frametype == AST_FRAME_VOICE || f->frametype == AST_FRAME_VIDEO) {
+			if (f && (f->frametype == AST_FRAME_VOICE || f->frametype == AST_FRAME_VIDEO)) {
 				stream = default_stream = ast_channel_get_default_stream(chan, ast_format_get_type(f->subclass.format));
 				f->stream_num = ast_stream_get_position(stream);
 			}
 		}
 		else
 			ast_log(LOG_WARNING, "No read routine on channel %s\n", ast_channel_name(chan));
-	}
-
-	if (dropnondefault && stream != default_stream) {
-		/* If the frame originates from a non-default stream and the caller can not handle other streams
-		 * absord the frame and replace it with a null one instead.
-		 */
-		ast_frfree(f);
-		f = &ast_null_frame;
-	} else if (stream == default_stream) {
-		/* Perform the framehook read event here. After the frame enters the framehook list
-		 * there is no telling what will happen, <insert mad scientist laugh here>!!! */
-		f = ast_framehook_list_read_event(ast_channel_framehooks(chan), f);
 	}
 
 	/*
@@ -4013,6 +4005,18 @@
 		struct ast_party_connected_line connected;
 		int hooked = 0;
 
+		if (dropnondefault && stream != default_stream) {
+			/* If the frame originates from a non-default stream and the caller can not handle other streams
+			 * absord the frame and replace it with a null one instead.
+			 */
+			ast_frfree(f);
+			f = &ast_null_frame;
+		} else if (stream == default_stream) {
+			/* Perform the framehook read event here. After the frame enters the framehook list
+			 * there is no telling what will happen, <insert mad scientist laugh here>!!! */
+			f = ast_framehook_list_read_event(ast_channel_framehooks(chan), f);
+		}
+
 		/* if the channel driver returned more than one frame, stuff the excess
 		   into the readq for the next ast_read call
 		*/
diff --git a/main/channel_internal_api.c b/main/channel_internal_api.c
index 362bd1a..5d2989a 100644
--- a/main/channel_internal_api.c
+++ b/main/channel_internal_api.c
@@ -867,7 +867,7 @@
 		return;
 	}
 
-	if (!chan->tech || !(chan->tech->properties & AST_CHAN_TP_MULTISTREAM) || !value) {
+	if (!(chan->tech && chan->tech->read_stream) || !value) {
 		struct ast_stream_topology *new_topology;
 
 		if (!value) {
@@ -949,6 +949,10 @@
 }
 void ast_channel_tech_set(struct ast_channel *chan, const struct ast_channel_tech *value)
 {
+	if (value->read_stream || value->write_stream) {
+		ast_assert(value->read_stream && value->write_stream);
+	}
+
 	chan->tech = value;
 }
 enum ast_channel_adsicpe ast_channel_adsicpe(const struct ast_channel *chan)
@@ -1798,7 +1802,7 @@
 	ast_assert(chan != NULL);
 
 	/* A non-MULTISTREAM channel can't manipulate topology directly */
-	ast_assert(chan->tech != NULL && (chan->tech->properties & AST_CHAN_TP_MULTISTREAM));
+	ast_assert(chan->tech != NULL && chan->tech->read_stream);
 
 	/* Unless the channel is being destroyed, we always want a topology on
 	 * it even if its empty.
diff --git a/tests/test_stream.c b/tests/test_stream.c
index d602d52..c9ad228 100644
--- a/tests/test_stream.c
+++ b/tests/test_stream.c
@@ -804,8 +804,61 @@
 	return res;
 }
 
+struct mock_channel_pvt {
+	int mallocd;
+	unsigned int wrote;
+	unsigned int wrote_stream;
+	int stream_num;
+	int frame_limit;
+	int frame_count;
+	int streams;
+};
+
+static struct ast_frame *mock_channel_read(struct ast_channel *chan)
+{
+	struct mock_channel_pvt *pvt = ast_channel_tech_pvt(chan);
+	struct ast_frame f = { 0, };
+
+	if (pvt->frame_count >= pvt->frame_limit) {
+		return NULL;
+	}
+
+	if (pvt->frame_count % 2 == 0) {
+		f.frametype = AST_FRAME_VOICE;
+		f.subclass.format = ast_format_ulaw;
+	} else {
+		f.frametype = AST_FRAME_VIDEO;
+		f.subclass.format = ast_format_h264;
+	}
+	f.seqno = pvt->frame_count;
+	f.stream_num = pvt->frame_count % pvt->streams;
+	pvt->frame_count++;
+
+	return ast_frdup(&f);
+}
+
+static int mock_channel_write(struct ast_channel *chan, struct ast_frame *fr)
+{
+	struct mock_channel_pvt *pvt = ast_channel_tech_pvt(chan);
+
+	pvt->wrote = 1;
+
+	return 0;
+}
+
+static int mock_channel_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame *fr)
+{
+	struct mock_channel_pvt *pvt = ast_channel_tech_pvt(chan);
+
+	pvt->wrote_stream = 1;
+	pvt->stream_num = stream_num;
+
+	return 0;
+}
+
 static const struct ast_channel_tech mock_stream_channel_tech = {
-	.properties = AST_CHAN_TP_MULTISTREAM,
+	.read_stream = mock_channel_read,
+	.write_stream = mock_channel_write_stream,
 };
 
 AST_TEST_DEFINE(stream_topology_channel_set)
@@ -853,33 +906,14 @@
 	return res;
 }
 
-struct mock_channel_pvt {
-	unsigned int wrote;
-	unsigned int wrote_stream;
-	int stream_num;
-};
-
-static int mock_channel_write(struct ast_channel *chan, struct ast_frame *fr)
-{
-	struct mock_channel_pvt *pvt = ast_channel_tech_pvt(chan);
-
-	pvt->wrote = 1;
-
-	return 0;
-}
-
-static int mock_channel_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame *fr)
-{
-	struct mock_channel_pvt *pvt = ast_channel_tech_pvt(chan);
-
-	pvt->wrote_stream = 1;
-	pvt->stream_num = stream_num;
-
-	return 0;
-}
-
 static int mock_channel_hangup(struct ast_channel *chan)
 {
+	struct mock_channel_pvt *pvt = ast_channel_tech_pvt(chan);
+
+	if (pvt->mallocd) {
+		ast_free(pvt);
+	}
+
 	ast_channel_tech_pvt_set(chan, NULL);
 	return 0;
 }
@@ -894,7 +928,7 @@
 {
 	RAII_VAR(struct ast_format_cap *, caps, NULL, ao2_cleanup);
 	struct ast_channel *mock_channel;
-	struct mock_channel_pvt pvt;
+	struct mock_channel_pvt pvt = { 0, };
 	enum ast_test_result_state res = AST_TEST_FAIL;
 	struct ast_frame frame = { 0, };
 
@@ -981,10 +1015,10 @@
 }
 
 static const struct ast_channel_tech mock_channel_write_stream_tech = {
-	.properties = AST_CHAN_TP_MULTISTREAM,
 	.write = mock_channel_write,
 	.write_video = mock_channel_write,
 	.write_stream = mock_channel_write_stream,
+	.read_stream = mock_channel_read,
 	.hangup = mock_channel_hangup,
 };
 
@@ -1207,6 +1241,8 @@
 		goto end;
 	}
 
+
+
 	if (!pvt.wrote_stream) {
 		ast_test_status_update(test, "Successfully wrote a frame of h264 to the first video stream but it never reached the channel driver\n");
 		goto end;
@@ -1268,6 +1304,215 @@
 	return res;
 }
 
+static int load_stream_readqueue(struct ast_channel *chan, int frames)
+{
+	struct mock_channel_pvt *pvt = ast_channel_tech_pvt(chan);
+	struct ast_frame f = { 0, };
+	struct ast_frame *frame = NULL;
+	int i;
+
+	while ((frame = AST_LIST_REMOVE_HEAD(ast_channel_readq(chan), frame_list)))
+			ast_frfree(frame);
+
+	for (i = 0; i < frames; i++) {
+		if (pvt->frame_count % 2 == 0) {
+			f.frametype = AST_FRAME_VOICE;
+			f.subclass.format = ast_format_ulaw;
+		} else {
+			f.frametype = AST_FRAME_VIDEO;
+			f.subclass.format = ast_format_h264;
+		}
+		f.stream_num = pvt->frame_count % pvt->streams;
+		f.seqno = pvt->frame_count;
+		ast_queue_frame(chan, ast_frdup(&f));
+		pvt->frame_count++;
+	}
+
+	return 0;
+}
+
+static struct ast_channel *make_channel(struct ast_test *test, int streams,
+		struct ast_channel_tech *tech)
+{
+	RAII_VAR(struct ast_format_cap *, caps, NULL, ao2_cleanup);
+	struct ast_channel *mock_channel = NULL;
+	struct mock_channel_pvt *pvt = NULL;
+	struct ast_stream_topology *topology = NULL;
+	struct ast_stream *stream;
+	enum ast_test_result_state res = AST_TEST_PASS;
+	int i;
+
+	mock_channel = ast_channel_alloc(0, AST_STATE_DOWN, NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, "TestChannel");
+	ast_test_validate_cleanup(test, mock_channel, res, done);
+	ast_channel_tech_set(mock_channel, tech);
+
+	if (tech->read_stream) {
+		topology = ast_stream_topology_alloc();
+		ast_test_validate_cleanup(test, topology, res, done);
+
+		for (i = 0; i < streams; i++) {
+			stream = ast_stream_alloc((i % 2 ? "video": "audio"), (i % 2 ? AST_MEDIA_TYPE_VIDEO : AST_MEDIA_TYPE_AUDIO));
+			ast_test_validate_cleanup(test, stream, res, done);
+			ast_test_validate_cleanup(test, ast_stream_topology_append_stream(topology, stream) == i, res, done);
+		}
+		ast_test_validate_cleanup(test, ast_stream_topology_get_count(topology) == streams, res, done);
+		ast_channel_set_stream_topology(mock_channel, topology);
+		topology = NULL;
+	} else {
+		caps = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
+		ast_test_validate_cleanup(test, caps, res, done);
+
+		ast_test_validate_cleanup(test, ast_format_cap_append(caps, ast_format_ulaw, 0) == 0, res, done);
+		ast_test_validate_cleanup(test, ast_format_cap_append(caps, ast_format_h264, 0) == 0, res, done);
+		ast_channel_nativeformats_set(mock_channel, caps);
+	}
+
+	pvt = ast_calloc(1, sizeof(*pvt));
+	ast_test_validate_cleanup(test, pvt, res, done);
+	pvt->mallocd = 1;
+	ast_channel_tech_pvt_set(mock_channel, pvt);
+
+	ast_channel_unlock(mock_channel);
+
+done:
+	ast_stream_topology_free(topology);
+	if (res == AST_TEST_FAIL && mock_channel) {
+		ast_hangup(mock_channel);
+	}
+
+	return mock_channel;
+}
+
+enum CHANNEL_READ_TYPE {
+	CHANNEL_READ,
+	CHANNEL_READ_STREAM
+};
+
+static struct ast_frame *read_from_chan(enum CHANNEL_READ_TYPE rt, struct ast_channel *chan)
+{
+	if (rt == CHANNEL_READ_STREAM) {
+		return ast_read_stream(chan);
+	} else {
+		return ast_read(chan);
+	}
+}
+
+static enum ast_test_result_state read_test(struct ast_test *test, struct ast_channel_tech *tech,
+		enum CHANNEL_READ_TYPE rt, int streams, int frames, int expected_nulls)
+{
+	struct ast_channel *mock_channel;
+	struct mock_channel_pvt *pvt;
+	struct ast_frame *fr = NULL;
+	enum ast_test_result_state res = AST_TEST_PASS;
+	int i;
+	int null_frames;
+
+	ast_test_status_update(test, "ChanType: %s ReadType: %s Streams: %d Frames: %d Expected Nulls: %d\n",
+			tech->read_stream ? "MULTI" : "NON-MULTI",
+			rt == CHANNEL_READ_STREAM ? "STREAM" : "NON-STREAM",
+			streams, frames, expected_nulls);
+	mock_channel = make_channel(test, 4, tech);
+	ast_test_validate_cleanup(test, mock_channel, res, done);
+
+	i = 0;
+	null_frames = 0;
+	pvt = ast_channel_tech_pvt(mock_channel);
+	pvt->frame_count = 0;
+	pvt->frame_limit = frames;
+	pvt->streams = streams;
+
+	load_stream_readqueue(mock_channel, frames / 2);
+	ast_channel_fdno_set(mock_channel, 0);
+
+	while ((fr = read_from_chan(rt, mock_channel))) {
+		ast_channel_fdno_set(mock_channel, 0);
+		if (fr->frametype != AST_FRAME_NULL) {
+			ast_test_validate_cleanup(test, i == fr->seqno, res, done);
+			ast_test_validate_cleanup(test, fr->frametype == ( i % 2 ? AST_FRAME_VIDEO : AST_FRAME_VOICE), res, done);
+			ast_test_validate_cleanup(test, fr->stream_num == ( i % streams ), res, done);
+			ast_frfree(fr);
+		} else {
+			null_frames++;
+		}
+		fr = NULL;
+		i++;
+	}
+	ast_test_status_update(test, "Frames read: %d NULL frames: %d\n", i, null_frames);
+	ast_test_validate_cleanup(test, i == frames, res, done);
+	ast_test_validate_cleanup(test, null_frames == expected_nulls, res, done);
+
+done:
+	ast_hangup(mock_channel);
+
+	return res;
+}
+
+AST_TEST_DEFINE(stream_read_non_multistream)
+{
+	struct ast_channel_tech tech = {
+		.read = mock_channel_read,
+		.hangup = mock_channel_hangup,
+	};
+
+	enum ast_test_result_state res = AST_TEST_PASS;
+
+	switch (cmd) {
+	case TEST_INIT:
+		info->name = "stream_read_non_multistream";
+		info->category = "/main/stream/";
+		info->summary = "stream reading from non-multistream capable channel test";
+		info->description =
+			"Test that reading frames from a non-multistream channel works as expected";
+		return AST_TEST_NOT_RUN;
+	case TEST_EXECUTE:
+		break;
+	}
+
+	res = read_test(test, &tech, CHANNEL_READ, 2, 16, 0);
+	ast_test_validate(test, res == AST_TEST_PASS, "non multi, non read stream, 4 stream");
+
+	res = read_test(test, &tech, CHANNEL_READ_STREAM, 2, 16, 0);
+	ast_test_validate(test, res == AST_TEST_PASS, "non multi, read stream, 4 stream");
+
+	return res;
+}
+
+AST_TEST_DEFINE(stream_read_multistream)
+{
+	struct ast_channel_tech tech = {
+		.read_stream = mock_channel_read,
+		.write_stream = mock_channel_write_stream,
+		.hangup = mock_channel_hangup,
+	};
+	enum ast_test_result_state res = AST_TEST_PASS;
+
+	switch (cmd) {
+	case TEST_INIT:
+		info->name = "stream_read_multistream";
+		info->category = "/main/stream/";
+		info->summary = "stream reading from multistream capable channel test";
+		info->description =
+			"Test that reading frames from a multistream channel works as expected";
+		return AST_TEST_NOT_RUN;
+	case TEST_EXECUTE:
+		break;
+	}
+
+	res = read_test(test, &tech, CHANNEL_READ, 2, 16, 0);
+	ast_test_validate(test, res == AST_TEST_PASS, "multi, non read stream, 2 stream");
+
+	res = read_test(test, &tech, CHANNEL_READ_STREAM, 2, 16, 0);
+	ast_test_validate(test, res == AST_TEST_PASS, "multi, read stream, 2 stream");
+
+	res = read_test(test, &tech, CHANNEL_READ, 4, 16, 8);
+	ast_test_validate(test, res == AST_TEST_PASS, "multi, non read stream, 4 stream");
+
+	res = read_test(test, &tech, CHANNEL_READ_STREAM, 4, 16, 0);
+	ast_test_validate(test, res == AST_TEST_PASS, "multi, read stream, 4 stream");
+
+	return res;
+}
+
 static int unload_module(void)
 {
 	AST_TEST_UNREGISTER(stream_create);
@@ -1286,6 +1531,8 @@
 	AST_TEST_UNREGISTER(stream_topology_channel_set);
 	AST_TEST_UNREGISTER(stream_write_non_multistream);
 	AST_TEST_UNREGISTER(stream_write_multistream);
+	AST_TEST_UNREGISTER(stream_read_non_multistream);
+	AST_TEST_UNREGISTER(stream_read_multistream);
 	return 0;
 }
 
@@ -1306,6 +1553,8 @@
 	AST_TEST_REGISTER(stream_topology_channel_set);
 	AST_TEST_REGISTER(stream_write_non_multistream);
 	AST_TEST_REGISTER(stream_write_multistream);
+	AST_TEST_REGISTER(stream_read_non_multistream);
+	AST_TEST_REGISTER(stream_read_multistream);
 	return AST_MODULE_LOAD_SUCCESS;
 }
 

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If7792b20d782e71e823dabd3124572cf0a4caab2
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: George Joseph <gjoseph at digium.com>



More information about the asterisk-code-review mailing list