Summary:	ASTERISK-08151: music on hold not random
Reporter:	Michael Durian (durian)	Labels:
Date Opened:	2006-11-18 15:18:04.000-0600	Date Closed:	2006-11-27 11:32:35.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Resources/res_musiconhold
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20061120__bug8381.diff.txt ( 1) res_musiconhold.c.diff
Description:	Music on hold is not randomized, even if random=yes is specified in musiconhold.conf.  For some reason, res_musiconhold.c:moh_files_release() is called before ast_moh_files_next().  The leads to save_pos being set to pos + 1.  Since pos was just zeroed via memset in moh_files_alloc(), this means saved_pos is always 1 when ast_moh_files_next() is called for the first time. ast_moh_files_next() checks the value of save_pos to see if it is non-zero.  If it is non-zero, ast_moh_files_next() uses it as a file index instead of generating a new file index.  Since it doesn't try to generate a new file index, it never gets a chance to select a random file. This patch splits ast_moh_files_next() into two functions.  One that just generates the file index (pos) and the other that actually opens the file.  I have also modified moh_files_alloc() to call the function that generates the file index after the state is initialized.  This ensures that pos contains a valid (either first or random) file index prior to the unexplained call to moh_files_release().  When moh_files_release() gets called, it still sets save_pos to pos + 1, but since pos contains a valid, and possibly random value, save_pos will contain a valid, and possibly random value.  Then when ast_moh_files_next() is called for the first time, it will still use the save_pos value, but it will be a random file if we are so configured. ****** ADDITIONAL INFORMATION ****** I'm running asterisk on FreeBSD 6.1.
Comments:	By: Tilghman Lesher (tilghman) 2006-11-19 00:17:08.000-0600 Before we can even look at your patch, you need to have a disclaimer on file.  However, given your description of the problem, I think I have come up with a restructuring of ast_moh_files_next() that will produce the desired result. By: Michael Durian (durian) 2006-11-20 18:01:05.000-0600 I think there are a couple issues in 20061119__bug8381.diff.txt. * ++state->pos %= state->class->total_files;  Is not a valid construct.  The left hand side of the assignment cannot have the ++.   state->pos++;   state->pos %= state->class->total_files;  would work, as would (of course)   state->pos = (state->pos + 1) % state->class->total_files; * Since state->pos = 0 is a valid index, and state->save_pos can be set to  state->pos, the check if (state->save_pos) is no longer a valid way to  check if save_pos has been set. * chan->stream is now closed if it is open in ast_moh_files_next() even if  we are trying to play a saved position.  I might be wrong here, but I  think the idea is that we want to pick up where we left off in the  current music file if the far end is put on hold multiple times in the  same call.  By closing and reopening chan->stream each time, don't we  reset to the beginning of the file? * It would be a nice feature if a random file was selected every time we  need a new file, instead of just randomizing our initial selection and  then walking through all the files sequentially after that. By: Tilghman Lesher (tilghman) 2006-11-20 19:42:56.000-0600 1) Construct fixed. 2) Doesn't matter.  If save_pos is zero, then pos won't get changed, so it stays with whatever it was previously.  In the case of initial value, pos is already zero.  In the case of random selection, it doesn't make any difference, as it will get set randomly.  Really, I think save_pos could go away entirely, as it does nothing to help. 3) If we played another stream in the meantime, the file stream pointer would be wrong anyway.  We can't play from the same file and same position consistently (especially across a reload, where the list of files might change). 4) The patch I uploaded does randomly select a new file each time. By: Michael Durian (durian) 2006-11-20 19:59:13.000-0600 Your latest patch works for me.  I concur about getting rid of save_pos.  Given my hypothesis on what it was used for was incorrect, I can't see what it does. By: Tilghman Lesher (tilghman) 2006-11-27 11:32:35.000-0600 Committed, revision 48050.