[Home]

Summary:ASTERISK-06720: [patch] ast_play_and_record() with silence detection doesn't trim the silence
Reporter:Tony Mountifield (softins)Labels:
Date Opened:2006-04-06 16:20:53Date Closed:2006-04-10 21:02:02
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) app.c-1.2.patch
( 1) app.c-1.2-v2.patch
( 2) app.c-trunk.patch
( 3) app.c-trunk-v2.patch
Description:In app.c function ast_play_and_record(), when using the silence detector, the truncation code never gets called to remove the detected silence, because the value of res is zero when control gets to that point.

The problem is fixed by changing the test from "if (res > 0)" to "if (res >= 0)".

Trivial patches attached for 1.2 and trunk (almost identical)
Comments:By: Andrey S Pankov (casper) 2006-04-06 17:34:29

I can't see where res may be set to 0. Can you explain please?
By: Tony Mountifield (softins) 2006-04-07 01:14:26

Yes, it gets left at 0 by the following code:

               if (f->frametype == AST_FRAME_VOICE) {
                       /* write each format */
                       for (x=0;x<fmtcnt;x++) {
                               res = ast_writestream(others[x], f);
                       }

and then if recording is terminated by the detection of silence, nothing else has written to res when you arrive at the test just before truncation:

               if (res > 0) {
                       if (totalsilence)
                               ast_stream_rewind(others[x], totalsilence-200);
                       else
                               ast_stream_rewind(others[x], 200);
               }
               ast_truncstream(others[x]);
               ast_closestream(others[x]);

So the rewind never gets done.

On second thoughts, an alternative approach would be to set res to a specific non-zero value (e.g. 's') in the relevant branch of the code to indicate that it was silence detection that terminated the recording. Just like other reasons have res set to '#', '0' or 't'.
By: Tony Mountifield (softins) 2006-04-07 02:18:08

Actually, if taking the second approach, it would probably be better to set res to something other than 's', in case the value ever gets used as an extension. 'S', perhaps? (extension matching is case-sensitive, isn't it?)
By: Tony Mountifield (softins) 2006-04-07 02:35:41

V2 patches uploaded using the second approach. The variable gotsilence was never tested, so I removed it at the same time. I also applied the same change to ast_play_and_prepend().
By: BJ Weschke (bweschke) 2006-04-07 07:45:20

softins - I'm not sure I like the second patch approach because that would cause us to return non-zero from the function, which is usually indicative of a problem on asterisk function returns.
By: BJ Weschke (bweschke) 2006-04-07 07:50:28

actually - scratch that. you're right. It's already returning non-zero stuff.
By: BJ Weschke (bweschke) 2006-04-07 08:03:55

softins: One more question though. It looks like we may break the current functionality in app_voicemail.c on the use of ast_play_and_prepend with your patch because a "valid" return could now also be 'S' instead of just 't'. Can you take a look and verify this please? Thanks.
By: Andrey S Pankov (casper) 2006-04-07 08:43:21

Off-topic: is ast_play_and_prepend is needed at all? It's not used anywhere.
By: Tony Mountifield (softins) 2006-04-07 09:57:34

casper: ast_play_and_prepend() is used in app_voicemail.c

bweschke: I've looked at app_voicemail.c, and it should be ok: ast_play_and_(record|prepend) will return '#' if the user ends their recording by pressing '#', and that value is not tested for specially either, so 'S' would be treated the same (goes back round to the default case: press 1 or press 2).

In app_dial.c, the return code from ast_play_and_record() is not used (actually, I guess it ought at least to check for -1).

The use of ast_play_and_record() within ast_record_review() in app.c looks fine too.

That's all the uses I can find.
By: Andrey S Pankov (casper) 2006-04-07 11:19:57

softins: I meant ast_app_getvoice :)
By: Andrey S Pankov (casper) 2006-04-07 11:30:59

Shouldn't ast_stream_rewind(others[x], totalsilence-200) actually be totalsilence+200? or +500? Does 200 stand for DTMF signal duration?

It would also be nice to remove duplicated code from both play|prepend functions and to constify arguments in prepend function.
By: Tony Mountifield (softins) 2006-04-07 11:54:40

No, I think it should be totalsilence-200, as it is truncating all but about 200ms of the silence at the end of the recording. If the recording is terminated by silence detection, there is no DTMF.

It may be that the 200 in the else clause is to eliminate vestigial DTMF.

But anyway, the purpose of this bug report is to fix a specific problem that needs fixing in both trunk and 1.2, without getting bogged down in other issues, thoughts of re-writing or re-factoring other parts. That may be worthwhile, but should be left for a separate exercise on trunk only, IMHO.
By: BJ Weschke (bweschke) 2006-04-07 12:14:39

softins: Agreed on the core topic of the bug. However, let's take a look at app_voicemail.c once more, because I think we do need a modification to afford this proposed change. The problem lies in vm_forwardoptions(...) where you call ast_play_and_prepend(..). If we start returning 'S' then cmd will not get set to 0 within vm_forwardoptions(...) and we're then going to start and infinite loop within that while statement. Also, the function calling that function is expecting it to get returned 0 on the condition of a "successful" execution. By making the change to return 'S' out of prepend without accomodating this new return in vm_forwardoptions(...) I think we're going to break forwarding of voicemail and possible cause a race state. Am I missing something?
By: BJ Weschke (bweschke) 2006-04-07 12:16:29

Actually - looking at the switch again, it won't cause race state, but it will iterate again into that while loop when it probably should not.
By: Tony Mountifield (softins) 2006-04-07 13:21:07

Won't it do exactly the same if ast_play_and_prepend() returns '#' because the user pressed '#' after speaking? If we conclude that a change is needed, then it needs to handle '#' and 'S' both the same.

Looking more closely at vm_forwardoptions(), when ast_play_and_prepend() returns, we restore the gain and then break out of the while loop (I overlooked that break in my first analysis and thought it would loop round again).

After breaking out of the loop, 't' is changed to 0, but anything else is left alone.

vm_forwardoptions() is called only from forward_message(), and its return value is only checked for zero or non-zero, with forwarding only done if the return value was zero.

So I think perhaps in vm_forwardoptions() we need to check for 'S' and '#' too:

if (cmd == 't' || cmd == '#' || cmd == 'S')
  cmd = 0;

(assuming we go with my 'S' suggestion, but '#' needs doing anyway).

You were right, it was worth looking more closely.
By: BJ Weschke (bweschke) 2006-04-10 21:02:01

Commits made to 1.2 and /trunk. Thanks softins!