[FFmpeg-devel] [PATCH][GSOC] avfilter: add atone filter

[Marshall Murmu]

On Mon, Mar 16, 2020 at 3:40 PM Moritz Barsnick <barsnick at gmx.net> wrote:

> Hi Marshall,
>
> On Sat, Mar 14, 2020 at 15:58:32 +0530, Marshall Murmu wrote:
>
> > +    {"soundfont",   "location of soundfont",
>  OFFSET(soundfont),   AV_OPT_TYPE_STRING,   {.str=NULL},     CHAR_MIN,
> CHAR_MAX,  FLAGS},
>
> I don't think we define ranges for AV_OPT_TYPE_STRING.
>
 I looked at other filters and found that some had defined the ranges as
(CHAR_MIN, CHAR_MAX) or (0,0)  or the range was not defined. So I didn't
think it would be an issue.

> +    if(fluidsynth->settings == NULL) {
>
> As Paul mentioned, whitespace style (throughout the complete source).
>
> Also, "if (!fluidsynth->settings)" is the preferred style.
>
Will fix it in next patch.

> +        av_log(ctx, AV_LOG_ERROR, "Failed to create fluidsynth
> settings\n");
> > +        return AVERROR_UNKNOWN;
>
> I'm not sure this is the correct error. If the library can only fail
> due to lack of memory, this should be AVERROR(ENOMEM). Else failing
> external library calls lead to AVERROR_EXTERNAL.
>
Fixing it.


> > +    if(fluidsynth->soundfont_id < 0) {
> > +        av_log(ctx, AV_LOG_ERROR, "Failed to load soundfont\n");
> > +    }
>
> Shouldn't this also lead to an error being returned?
> OTOH, if it isn't fatal, the log message should be a warning.
>
Yes, it should return an error. Fluidsynth requires a soundfont to
synthesize the tones. Without a soundfont it won't be able to generate any
sound. Will fix it in next patch.