[FFmpeg-devel] [PATCH] flite audio source

[Stefano Sabatini]

On date Wednesday 2012-07-25 10:56:15 +0200, Nicolas George encoded:
> L'octidi 8 thermidor, an CCXX, Stefano Sabatini a écrit :
[...] 
> > +Set the filename containing the text to speech.
> > +
> > + at item text
> > +Set the text to speech.
> 
> A native English speaker should confirm, but I believe it should rather be
> "speak". At least, it seems that "speech" is very rarely a verb.

Yes, changed to "text to speak", "utter" was another possibility but
seems less used.

[...]
> > +AVFILTER_DEFINE_CLASS(flite);
> > +
> > +static int flite_inited = 0;
> 
> Maybe "volatile", to be slightly more sure wrt threads.

OK.

> > +
> > +/* declare functions for all the supported voices */
> 
> > +#define DECLARE_REGISTER_VOICE_FN(name) cst_voice *register_cmu_us_## name(void *)
> > +
> > +DECLARE_REGISTER_VOICE_FN(awb);
> > +DECLARE_REGISTER_VOICE_FN(kal);
> > +DECLARE_REGISTER_VOICE_FN(kal16);
> > +DECLARE_REGISTER_VOICE_FN(rms);
> > +DECLARE_REGISTER_VOICE_FN(slt);
> > +
> > +struct voice_entry {
> > +    const char *name;
> > +    cst_voice * (*register_fn)(void *);
> > +} voice_entry;
> > +
> > +static cst_voice *select_voice(const char *voice_name)
> > +{
> > +    int i;
> > +    static struct voice_entry voice_entries[] = {
> > +        { "awb",   register_cmu_us_awb },
> > +        { "kal",   register_cmu_us_kal },
> > +        { "kal16", register_cmu_us_kal16 },
> > +        { "rms",   register_cmu_us_rms },
> > +        { "slt",   register_cmu_us_slt },
> > +    };
> 

> I wonder whether a smart use of macros would allow to avoid duplicating the
> list. But do not consider it a blocker.

I don't know, I think declaration and definition have to be done
apart.

Note in libflite there is a flite_voice_select(), but I can't find a
sane way to set it up the list on which it relies, should be probably
reported as a bug.

[...]
> > +    flite->voice = select_voice(flite->voice_str);
> 
> > +    if (!flite->voice) {
> > +        av_log(ctx, AV_LOG_ERROR, "Impossible to select voice '%s'\n", flite->voice_str);
> > +        return AVERROR(EINVAL);
> > +    }
> 
> Can register_fn fail? If not, "Unknown voice" (plus maybe the list of known
> voices) would be more accurate. If yes, distinguishing between unknown and
> failure would be better.

Yes, extended the logic for the failure handling.

> 
> > +
> > +    if (flite->textfile && flite->text) {
> > +        av_log(ctx, AV_LOG_ERROR,
> > +               "Both text and textfile options set. Only one must be specified\n");
> 
> Having a terminated sentence and then an unterminated one seems really
> strange. Maybe use a semicolon in the middle.

Changed.

> > +        return AVERROR(EINVAL);
> > +    }
> > +
> > +    if (flite->textfile) {
> > +        uint8_t *textbuf;
> > +        size_t textbuf_size;
> > +
> 
> > +        if (flite->text) {
> > +        }
> 
> Looks like a leftover of something.

Removed.

> 
> > +        if ((err = av_file_map(flite->textfile, &textbuf, &textbuf_size, 0, ctx)) < 0) {
> > +            av_log(ctx, AV_LOG_ERROR,
> > +                   "The text file '%s' could not be read\n", flite->textfile);
> 
> "... could not be read: %s", ..., av_err2str(ret)? The same message will
> _probably_ be displayed later, but the information belong here, and there is
> a message anyway.

Changed.

[...]
> > +static int config_props(AVFilterLink *outlink)
> > +{
> > +    AVFilterContext *ctx = outlink->src;
> > +    FliteContext *flite = ctx->priv;
> > +
> > +    outlink->sample_rate = flite->wave->sample_rate;
> > +    outlink->time_base = (AVRational){1, flite->wave->sample_rate};
> > +
> > +    av_log(ctx, AV_LOG_VERBOSE, "voice:%s fmt:%s sample_rate:%d\n",
> > +           flite->voice_str,
> > +           av_get_sample_fmt_name(outlink->format), outlink->sample_rate);
> > +    return 0;
> > +}
> > +
> 
> > +static int query_formats(AVFilterContext *ctx)
> 
> I believe it comes, logically, before config_props.

Yes.

[...]
> I did not grade the level of nitpicking. Most of the comments are minor.
> 
> Thanks for the work.

Added a list_voices option.

I'll apply in a few days if I read no more comments.

Thanks for the review.
-- 
FFmpeg = Fast and Formidable Mortal Political Elastic Gadget
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-lavfi-add-flite-audio-source.patch
Type: text/x-diff
Size: 14544 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120726/268e28f3/attachment.bin>