<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3338/">https://reviewboard.asterisk.org/r/3338/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 13th, 2014, 12:43 p.m. CDT, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You need to add cases to the switches that handle AST_FRAME_BRIDGE_ACTION in frame.c and bridge_softmix.c. It is probably best if the AST_FRAME_BRIDGE_ACTION_SYNC case is next to the AST_BRIDGE_ACTION case.
The main difficulty with this patch is that you are adding an allocated resource to ast_frame's which inherently doesn't support this.</pre>
</blockquote>
<p>On March 13th, 2014, 1:58 p.m. CDT, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't understand the comment about adding an allocated resource to ast_frame. The frame payload is allocated on the stack, but that's the same strategy used for all bridge_playfile payloads right now. The bridge_playfile is memcpy'd into the sync_payload, and the sync_payload's data length is adjusted to account for this. When ast_frdup() copies the frame data to another frame, the entire sync_payload and encased bridge_playfile is copied to the other frame, so I'm not sure what the problem here is.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The allocated resource is referring to the fact that you have to explicitly keep track of the frame to manage the semaphore. If that frame is simply destroyed, you will wait forever; which is a resource leak.</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On March 13th, 2014, 2 p.m. CDT, Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and rmudgett.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated March 13, 2014, 2 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">ARI tests were occasionally showing a channel being stuck forever in Stasis. After looking at a live system where the problem was occurring, it became clear that the channel was stuck trying to wait for a playback to finish. However, it was also clear that the playback had been abandoned long ago.
When playing back a file to a channel that is in a bridge, the idea was to queue the playfile action onto the corresponding bridge channel, and then wait until the ARI custom playback function completed to signal that the playback had finished. There were several ways that this could fail:
* If the bridge channel were not found in the bridge, then we would never attempt to queue the playfile action, but we would still try to wait for it to finish happening.
* If queuing the playfile action did not succeed, then we would still attempt to wait for the action to occur.
* If the playback action was successfully queued, but the bridge channel were removed from the bridge before the playfile action could start, then the playback would never happen, and we'd wait forever.
The responsibility of knowing whether a playfile action has conpleted or ever will occur is the bridge's, since ARI can never fully know. With this in mind, I have created a new bridge channel API call to queue a playfile action synchronously. The way it is done, synchronous queuing operations could be created for other bridge actions quite easily. A new frametype, AST_FRAME_BRIDGE_ACTION_SYNC, contains a payload consisting of a synchronization ID and the actual payload of the bridge action that is to be queued. When the frame is queued, a synchronization structure is created and added to a linked list. The procedure then blocks until it is told that the frame has been disposed of.
When the frame gets freed (which will occur whether the playfile action succeeds or does not happen), the freer of the frame signals the waiting procedure that the playfile action has terminated, and control returns to whoever queued the playfile action in the first place.
>From ARI's perspective, this greatly simplifies its code. Most of the res_stasis_playback changes are code removal. The bridge_channel changes, though, are pretty large, which is why I've included Richard on this issue.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Initially, I did some manual playbacks using Swagger-UI to a channel in a bridge to ensure that the specified file was actually being played as promised. I also queued up several files in quick succession to ensure that they played in series and not on top of each other.
In addition, I have created an automated test that is up for review at /r/3339</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/12/res/res_stasis_playback.c <span style="color: grey">(410467)</span></li>
<li>/branches/12/main/frame.c <span style="color: grey">(410467)</span></li>
<li>/branches/12/main/channel.c <span style="color: grey">(410467)</span></li>
<li>/branches/12/main/bridge_channel.c <span style="color: grey">(410467)</span></li>
<li>/branches/12/include/asterisk/frame.h <span style="color: grey">(410467)</span></li>
<li>/branches/12/include/asterisk/bridge_channel.h <span style="color: grey">(410467)</span></li>
<li>/branches/12/funcs/func_frame_trace.c <span style="color: grey">(410467)</span></li>
<li>/branches/12/bridges/bridge_softmix.c <span style="color: grey">(410467)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3338/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>