Re: [Linphone-developers] [Q] Bug of linphone-1.0.1?

[Simon Morlat]

Hi Clement,

Le Mercredi 1 Juin 2005 05:29, Clement Chen a écrit :
> Hi, Simon,
>     Thank you a lot for the great comments. Since I don't know well
> about the architecture for filters, readers, and writers, I'm sorry
> for breaking the object model by my patch. I will follow your
> suggestions to try a new patch in my leisure time but the schedule may
> not be quaranteed. (sorry again...^^;;;) I'm trying to patch linphonec
> to provide call hold and call transfer now... Because they are of
> higher priorities for our pbx test automation.

Great. Stay close to the cvs so that even if your patch arrives in two weeks, 
it still apply to the sources.

>     In line100~102 of sipomatic.c, the file to play is selected based
> on the payload type, but the CLI doesn't provide options to setup
> file_path16000hz, so I add the -f_16k option for it. The 'file_16k'
> variable in the patch should be of type "gchar", isn't it? If you
> meant the following function in linphonecore.c, the boolean variable
> was used to make two functions ( linphone_core_set_play_file_8k ,
> linphone_core_set_play_file_16k) a single one.
> void linphone_core_set_play_file(LinphoneCore *lc, gchar *file2play,
> gboolean is8000hz);
>     Yes, I found MSRingPlayer too. I'll see if it can be reused. :)
I think that a linphone_core_set_play_file(LinphoneCore *lc, const gchar 
*path); 
would be enough provided that the file is a wav file. Thus it is easy to see 
the rate of the sound file in the wav header. See how MSRingPlayer  does 
that. In fact the MSRingPlayer already has the looping function, and already 
has the callback for end-of-file signaling. But it deals with wav files, 
contrary to MSRead. So I think MSRingPlayer is exactly what you want.

Also, try to follow the const / non-const linphone's coding standarts:
void linphone_core_set_play_file(LinphoneCore *lc, const gchar *path);
in this function the path will be strdup()'d and path will not be modified.
in
gchar * linphone_core_get_play_file(LinphoneCore *lc);
the return gchar * is supposed to be a copy of the string stored in the 
LinphoneCore object; that means that the caller of this function is supposed 
to g_free() it when it does no more use it.

in 
const gchar * linphone_core_get_play_file(LinphoneCore *lc);
Here the return value is 'const' that means that it should never be modified 
or freed. This is the case if you directly return the gchar * hold in the 
LinphoneCore object.

These coding practices allows to easily know who is allocating the objects and 
who is supposed to free them. This avoid memory leaks and double frees.

>
>     With the file playing features, we can generate a lot of linphone
> robots to conduct performance tests for our pbx products. In addition,
> voice quality measurement and voice mail feature test automation
> become feasible now. ^_^

I'm sorry to be too much strict with coding practices... But it saves so much 
time in debugging when a patch follows them. I think it is worth to spend a 
little more time to check the patch so that the patch can be integrated 
directly, rather than submitting a quick (and sometimes dirty) patch that 
I'll have to reject because it requires some refactoring, and I unfortunately 
don't have time for that.
If the patch is ok, it is merged and it becomes easy to maintain; that means 
that the functionnality that has been contributed will still be working in 
1.2.0 or in 2.0.0 . I think this is good for everyone.

Simon

>
> Thanks a lot!
>
> Clement.
>
> On 6/1/05, Simon Morlat <address@hidden> wrote:
> > Hello Clement,
> >
> > Thanks for the patch. I took some time to review it. This is good and
> > very complete work ..... but there's only one little detail that hurts
> > me:
> >
> > extern gboolean play_file_stream;
> > extern gboolean endless_play;
> >
> > That we can find in several places in your patch. This really breaks the
> > object model followed by linphone. I think the place for those booleans
> > is in AudioStream. The endless_play should be passed as argument to the
> > MSRead using a ms_read_set_endless_mode() like function and may be stored
> > in the MSRead structure for use within the process function.
> > Then the LinphoneCore structure should own those boolean as well as the
> > play file paths, then should pass it to AudioStream functions.
> >
> > All MSFilter objects should be independant. It should be possible to
> > create a MSRead running in endless_play mode while another is not. Same
> > thing for AudioStream: you should be able to create two AudioStream and
> > run them simultaneously, each one with its own parameters.
> >
> > If you submit me another patch with this little problem fixed, I'll be
> > happy to merge it into the CVS. If I had more time I could fix it myself
> > but unfortunately this is not the case.
> >
> > Just one question: why did you add a file_16k boolean parameter while the
> > rate is usually stored within the wav header of the file ? Note that the
> > MSRingPlayer filter does the same thing as MSRead except that it is able
> > to read wav files and it is already able to run in endless loop.
> >
> >
> > Simon