[Home]

Summary:ASTERISK-13827: [patch] Add support for relative path to digits,letters and phonetics files in say.c
Reporter:Nicolas Chapleau (nicchap)Labels:
Date Opened:2009-03-25 08:44:37Date Closed:2009-10-21 18:48:24
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Addons/New Feature
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) diff
Description:We use asterisk as our IVR base and thus support multiple applications for multiple clients. Each client have their own set of prompts and voices, including what I call VOCAB files, or digits - letters and phonetics. Currently Asterisk only supports these files from the language root:

ie: digits/ or fr/digits or fr/letters and so on.

This patch enables us to have multiple recordings of these files located in a relative directory under the required language:

ie: allison/digits , fr/isabelle/letters, en/manuel/phonetics and so on

I tried as best to avoid touching any internals and decided to use a channel variable called VOCAB_RELATIVE_PATH to enable this feature (simply call the dialplan Set function to enable it). If this variable exists, then the relative path defined by it is used, otherwise it defaults to it's usual location. If the file is not found under the relative path, it also defaults to it's usual location.

****** ADDITIONAL INFORMATION ******

Testing: All functions have been tested and work. I have simulated the following scenarios for english and french languages:

- No VOCAB_RELATIVE_PATH
- invalid VOCAB_RELATIVE_PATH
- valid VOCAB_RELATIVE_PATH - missing files
- valid VOCAB_RELATIVE_PATH - files present

Other notes:

- Does not affect files used in other modules, such as voicemail and meetme.
- if another type of channel can be used in order to avoid a channel lock each time a file is played, by all means, refer it to me or change it.

Compiling: Compiles on CentOS 4.X 64 bit.
Comments:By: Leif Madsen (lmadsen) 2009-03-29 12:41:12

Hmmmm... what about just creating a custom language and setting it prior to when you want to use the custom prompts, and resetting it back to your standard language after you've played the prompt you want?
By: Nicolas Chapleau (nicchap) 2009-03-29 13:12:18

I could. Let me see what the implications are and I'll get back to you shortly. Thanks for the suggestion.
By: Nicolas Chapleau (nicchap) 2009-03-31 06:22:36

Leif, that would work in that the vocab (digits) files would play but unfortunately that also breaks all the functions in say.c as they are also mapped to a specific language. For instance, if I create a language called frIsabelle, say_digits_str_full would need map frIsabbelle to french otherwise it defaults to english, any english based languages would be fine however.

So what's easier? Adding a language mapping in the c file and recompiling or adding logic based on a channel variable? Any other ideas?
By: Leif Madsen (lmadsen) 2009-04-01 13:10:12

Thanks for the feedback. I'll see if I can get a developer to comment on what they think would be the best approach.
By: Tzafrir Cohen (tzafrir) 2009-04-01 14:00:57

Hmm... I believe it would have been nice if the logic for 'es' would also apply to, say, es_allison and es_manuel .

At the moment this is not the case.

Language-specific logic is not only in say.c. It is also in app/app_voicemail.c and a bit in res/res_agi.c
By: Nicolas Chapleau (nicchap) 2009-04-01 14:36:18

The patch applies to ANY language already defined in say.c. We (our company) do not need to touch the app_voicemail and res_agi portions, simply all the Say* routines. So for instance, if VOCAB_RELATIVE_PATH channel variable is set to Manuel and LANGUAGE=es, all the Say* routines would get their files from:

es/Manuel/digits
es/Manuel/letters
es/Manuel/phonetic

If you would like to have this apply to app_voicemail and res_agi, I can always adapt them as well.
By: Tzafrir Cohen (tzafrir) 2009-04-01 14:45:52

There is already code in say.c that checks for either the case of 'pt_BR' or 'pt_PT' . I believe we'll have more such special cases in the future.
By: Nicolas Chapleau (nicchap) 2009-04-01 14:58:01

I agree that code exists that checks each language but these are for syntax (the way files play out dates, times, etc. for specific languages). I'm looking at having the defined languages (ie pt_BR, pt_PT, es, fr, ...) use different voices for the prompts. As it stands now, only one voice can be used for these prompts. This patch only affects where to look for the prompts, not how to "say" them.

BTW, your feedback and ideas are always appreciated. Thanks for taking the time.
By: Nicolas Chapleau (nicchap) 2009-05-20 07:25:43

Any other thoughts on this? Pinging to keep alive.
By: Leif Madsen (lmadsen) 2009-05-20 08:50:00

I guess the question is now, "Is this patch ready for testing?".

Is the patch doing what we want if it were accepted into the code base, or is there work still here to do?
By: Nicolas Chapleau (nicchap) 2009-05-20 08:54:32

I thorougly tested the app at my end with no issues, so I believe it's ready for open testing. I would however like to have some confirm that the following locking is required when retrieving a channel variable:

ast_channel_lock(chan);
if ((relpath = pbx_builtin_getvar_helper(chan, "VOCAB_RELATIVE_PATH"))) {
  relpath = ast_strdupa(relpath);
}
ast_channel_unlock(chan);

Thanks.
By: Leif Madsen (lmadsen) 2009-05-21 10:57:50

Assigned to dbrooks for testing!
By: David Brooks (dbrooks) 2009-05-27 09:49:04

I tested the patch on 1.6.0 SVN, and it functions appropriately. In the following scenarios:

- No VOCAB_RELATIVE_PATH
- invalid VOCAB_RELATIVE_PATH
- valid VOCAB_RELATIVE_PATH - missing files
- valid VOCAB_RELATIVE_PATH - files present

the patch behaves as expected. Looks good to me.
By: Nicolas Chapleau (nicchap) 2009-07-15 06:06:59

Would this patch be ready for branching into the latest release?
By: Leif Madsen (lmadsen) 2009-07-24 11:28:18

Well, into trunk since it is a feature -- never into a branch or release.
By: Nicolas Chapleau (nicchap) 2009-07-24 11:30:17

Ya, that's what I meant ;) Thanks. Nic.
By: Nguyen Dinh Trung (dinhtrung) 2009-07-31 15:33:07

Sorry for mess up the thread, but I misupload the file. Please remove the say.c.patch for me. :(
By: Leif Madsen (lmadsen) 2009-08-05 12:46:43

Removed! Will you be uploading a new file? Thanks!
By: Nicolas Chapleau (nicchap) 2009-08-06 06:37:46

I will NOT be uploading a new patch.
By: Leif Madsen (lmadsen) 2009-08-06 10:38:12

Marking this as Ready for Review so we can determine whether this should be merged. Thanks!
By: Tilghman Lesher (tilghman) 2009-08-06 16:59:43

Our recent discussion from an architectural level is that while we're interested in functionality like this, we plan to implement it in a different way, namely by using a multiple set of prefixes on the language element, such that we can failover in a sane way.  The present proposal looks like this:

If the language were set to "es_ES_m_manuel", with the "m" indicating a male voice, then language directories would be searched in the following order for a match:  "es_ES_m_manuel", "es_ES_m", "es_ES", "es".  If you were interested in modifying your patch to conform to this convention, that's the direction in which we'd like to go.
By: Nicolas Chapleau (nicchap) 2009-08-06 21:19:26

alrighty Mister T. You know best and I'm sure there is a reason why you prefer this method. I will update the best I can once I get back from holidays on Aug 17th.
By: Tzafrir Cohen (tzafrir) 2009-08-07 14:23:30

tilghman, where can I find more information about that discussion?

What prefixes? "m", "f"? But is this a really important characteristic that is worth hardcoding? Aren't there other such features?

This encoding is also incompatible with current names. Unless you expect the lack of 'm' to imply 'f'.
By: Tilghman Lesher (tilghman) 2009-08-08 08:17:49

tzafrir:  it was a private discussion within Digium was not publically archived.  Out of that discussion, came the conclusion that the language prefix should be:

languagecode[_territorycode[_gender[_variant]]]

And no, this encoding is completely compatible with the current schema.  The least specific names (e.g. "en") are the defaults.
By: Nicolas Chapleau (nicchap) 2009-08-20 07:15:00

tilghman: I'll do my best to explain why I'm doing this patch...

1) this only affects files that reside in /digits, /letters, /phonetics, which in "mostly" done through say.c (see tzafrir's comment above dated 2009-04-01 14:00)

2) It already uses the language prefix, that I do not touch, I only append to it, thus working with any language model you may adopt.

3) Consider the following sound file directory structure:

sounds/digits
sounds/letters
sounds/phonetics
sounds/customer1
sounds/customer2
sounds/customer3
sounds/customer1/digits
sounds/customer1/letters
sounds/customer1/phonetics

sounds/fr/digits
sounds/fr/letters
sounds/fr/phonetics
sounds/fr/customer1
sounds/fr/customer2
sounds/fr/customer3
sounds/fr/customer1/digits
sounds/fr/customer1/letters
sounds/fr/customer1/phonetics

What my patch does it that it tells which /digits, /letters, and /phonetics to use, based on existing language prefix. This permits me to be able to use a different subset for customer1 then that of customer2 or customer3. If no VOCAB_RELATIVE_PATH is specifed, it uses from the default language prefix, otherwise it appends to the language prefix.

Your suggestion to use languagecode[_territorycode[_gender[_variant]]] still does not "fix" how this module uses these files.

I guess you could say that this patch is equivalent to adding a relative path to play functions in the dialplan,

ie. playback(file1), playback(customer1/file1)

and this is not possible with any "say" functions.

Hope I'm clear.
By: Leif Madsen (lmadsen) 2009-08-20 14:52:45

Assigned to Tilghman for comments. Please set to appropriate status when complete. Thanks!
By: Tilghman Lesher (tilghman) 2009-08-20 16:13:11

1) I still don't see why you're doing this in say.c.  If this would be to be implemented at all, I'd want to see it in file.c, implemented for all files.

2) You need to document this behavior.

3) Since it alters how file paths are generated, it should probably be using a channel structure element to store the path (in a stringfield) and a dialplan function (probably CHANNEL) to get/set the value.
By: Tilghman Lesher (tilghman) 2009-08-20 16:18:28

russell also suggested that you write this up and present it on the asterisk-dev list, seeking comment.  This is a fairly major change, and I'd rather not be the single one approving it.
By: Nicolas Chapleau (nicchap) 2009-08-21 09:06:18

Posted to dev list for comments. Just a note, I don't consider this a major change as it it optional to use and in no way affects existing syntax of "say" functions, it also does not touch any playback functions or normal voice files.

I agree with you that implementing in file.c and using a channel structure element would be better albeit much more of a structural change.

Where, if this is adopted, should I document this?
By: Tilghman Lesher (tilghman) 2009-08-24 16:58:40

I think Kevin's comments on the -dev list have made it clear that this won't pass his veto.  If you want to discuss it more, please post to the -dev list.