On Mon, Mar 16, 2020 at 3:40 PM Moritz Barsnick <barsn...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
