[Home]

Summary:ASTERISK-11554: MoH file playback is broken
Reporter:pj (pj)Labels:
Date Opened:2008-03-01 15:50:31.000-0600Date Closed:2008-03-17 10:15:19
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) fileexists.patch
( 1) v2-fileexists.patch
Description:this bug was introduced in commit r104594, last working revision is r104592

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

/var/lib/asterisk/moh/fpm-world-mix.wav: RIFF (little-endian) data, WAVE audio, Microsoft PCM, 16 bit, mono 8000 Hz


[Mar  1 21:30:05] WARNING[28190]: file.c:557 ast_openstream_full: File /var/lib/asterisk/moh/fpm-world-mix does not exist in any format
[Mar  1 21:30:05] WARNING[28190]: res_musiconhold.c:268 ast_moh_files_next: Unable to open file '/var/lib/asterisk/moh/fpm-world-mix': No such file or directory
[Mar  1 21:30:05]     -- Stopped music on hold on SIP/324-b7603748
Comments:By: Dmitry Andrianov (dimas) 2008-03-01 18:39:13.000-0600

Attached patch which fixes the issue. However kpflemming who commited patch for 11831 which caused the problem needs to review it. For some reason I cannot re-open 11831.

What patch does is:
1. It does not try ANY language prefixes if function is passed absolute filename. This never worked for internal_timing=yes anyway. (It was trying to open /LANG//var/lib/asterisk/moh/fpm-world-mix)

2. It now checks both "en" language and null language when cannot find with specified language. Namely for langprefix="ru_ru" and filename="test/hello" it checks for

ru_ru/test/hello
ru/test/hello
en/test/hello
test/hello

for languageprefix=yes and

test/ru_ru/hello
test/ru/hello
test/en/hello
test/hello

for languageprefix=no.

3. I personally dislike buffer maths (like strcpy(buff + offset1, ...), strcpy(buf + offset1 + ofset2...)) so I redone these with snprintfs.

4. I removed dynamic buffer allocation when the buffer passed is not long enough. This was wrong because function documentation says it puts final filename into the passed buffer while nothing was passed back when dynamic buffer was allocated. (After all it is not possible anyway - we allocate a buffer when the passed one is too small and cannot fit the file name). This should not be a problem because both two callers of fileexists_core allocate buffer of proper size.
By: pj (pj) 2008-03-02 04:09:06.000-0600

I can confirm, that your patch solved this issue.
tested with SVN-trunk-r105509
By: Jason Parker (jparker) 2008-03-03 11:15:36.000-0600

Would it be difficult to make it look in the following order?

ru_ru/test/
ru/test/
test/
en/test/
By: Dmitry Andrianov (dimas) 2008-03-03 11:31:12.000-0600

Not difficult at all. But what is the point? As I understand, the default installation (when using trunk) creates en/ directory and puts files there so they should be taken from there. This is basically what patch for 11831 tried to achieve.
Am I wrong ?


By: David Van Ginneken (davevg) 2008-03-05 13:44:29.000-0600

I tested the patch and it resolved my issue on 1.6.0 r106140:

Both Playback and BackgroundDetect did not allow you to pass the full filename (minus extension) to the program.

exten => 3000,1,Answer()
exten => 3000,n,BackgroundDetect(/var/lib/asterisk/sounds/tt-monkeys)

or

exten => 3000,1,Answer()
exten => 3000,n,Playback(/var/lib/asterisk/sounds/tt-monkeys)
By: Jason Parker (jparker) 2008-03-05 13:51:30.000-0600

dimas: If somebody has language set to ru, but has sounds in ./ and en/ but not ru/, I would expect that it would play the files from ./ rather than en/.

I think it should be possible to set a "default" language by just putting the sounds in ./, and allowing an override by setting lang=.

Maybe that would cause other unforeseen breakage though.  Thoughts?
By: Dmitry Andrianov (dimas) 2008-03-05 15:25:21.000-0600

Qwell,
from what you are saying it looks like that the best way is to remove attempt to use "en" directory at all. That means directories for language=ru_ru will be checked in the following order:

./ru_ru
./ru
./

Is that what you want? To my understanding this is exactly the behavior Asterisk had BEFORE the patch 11831.
Please confirm.
By: Jason Parker (jparker) 2008-03-05 15:42:27.000-0600

No, I think the intent of IgorG's patch was correct.  The implementation just wasn't.

I think it should eventually hit en/ - the question is when.

So, in 1.2, the order would have been ru_ru/, ru/, ./ - which was fine, because the en/ dir did not exist.  I believe 1.4 was more or less the same, it just switched the order of dirs.  In 1.6 however, the sounds were moved into the en/ dir.  For new installs, this would be fine, as make install would put the english prompts in the en/ dir.

The issue (besides the MoH stuff reported here, which we can all agree is a bug), in my opinion, is that older installs that were upgraded from 1.4 may still expect to have sounds played from ./, even though en/ now exists.

Am I making any sense to anybody?  I think there's a very small communication gap here.  If I'm still not making sense, I can try to give a few examples.
By: Dmitry Andrianov (dimas) 2008-03-05 17:56:20.000-0600

Made new patch as per IRC discussion.

Rewrote the function once again. The loop for a bit confusing so now the main code is linear.
By: Jason Parker (jparker) 2008-03-06 12:17:14.000-0600

Patch looks good.  Could a few people test it and report back?
By: David Van Ginneken (davevg) 2008-03-06 12:32:49.000-0600

Tested the new patch using Playback and BackgroundDetect with sound files in both the default directory and using the full path.  It worked for me.
By: Digium Subversion (svnbot) 2008-03-06 16:07:53.000-0600

Repository: asterisk
Revision: 106439

U   trunk/main/file.c

------------------------------------------------------------------------
r106439 | qwell | 2008-03-06 16:07:52 -0600 (Thu, 06 Mar 2008) | 8 lines

Fix file playback in many cases.

(closes issue ASTERISK-11554)
Reported by: pj
Patches:
     v2-fileexists.patch uploaded by dimas (license 88) (with modifications by me)
Tested by: dimas, qwell, russell

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=106439
By: Digium Subversion (svnbot) 2008-03-06 16:11:19.000-0600

Repository: asterisk
Revision: 106440

_U  branches/1.6.0/
U   branches/1.6.0/main/file.c

------------------------------------------------------------------------
r106440 | qwell | 2008-03-06 16:11:19 -0600 (Thu, 06 Mar 2008) | 16 lines

Merged revisions 106439 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
r106439 | qwell | 2008-03-06 16:11:30 -0600 (Thu, 06 Mar 2008) | 8 lines

Fix file playback in many cases.

(closes issue ASTERISK-11554)
Reported by: pj
Patches:
     v2-fileexists.patch uploaded by dimas (license 88) (with modifications by me)
Tested by: dimas, qwell, russell

........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=106440