[FFmpeg-devel] [PATCH 1/2] lavf/dvenc: improve error messaging

[Anton Khirnov]

Quoting Stefano Sabatini (2024-01-20 16:24:07)
>      if ((c->sys->time_base.den != 25 && c->sys->time_base.den != 50) || c->sys->time_base.num != 1) {
> -        if (c->ast[0] && c->ast[0]->codecpar->sample_rate != 48000)
> -            goto bail_out;
> -        if (c->ast[1] && c->ast[1]->codecpar->sample_rate != 48000)
> -            goto bail_out;
> +        int j;

No need to declare a loop variable outside of the loop. Also, there's
already i.

> -    if (((c->n_ast > 1) && (c->sys->n_difchan < 2)) ||
> -        ((c->n_ast > 2) && (c->sys->n_difchan < 4))) {
> -        /* only 2 stereo pairs allowed in 50Mbps mode */
> -        goto bail_out;
> +    if ((c->n_ast > 1) && (c->sys->n_difchan < 2)) {
> +        av_log(s, AV_LOG_ERROR,
> +               "Invalid number of channels %d, only 1 stereo pairs is allowed in 25Mps mode.\n",
> +               c->n_ast);
> +        return AVERROR_INVALIDDATA;
> +    }
> +    if ((c->n_ast > 2) && (c->sys->n_difchan < 4)) {
> +        av_log(s, AV_LOG_ERROR,
> +               "Invalid number of channels %d, only 2 stereo pairs are allowed in 50Mps mode.\n",
> +               c->n_ast);
> +        return AVERROR_INVALIDDATA;

Surely this can be done in one log statement.

>      }
>  
>      /* Ok, everything seems to be in working order */
> @@ -376,14 +427,14 @@ static DVMuxContext* dv_init_mux(AVFormatContext* s)
>          if (!c->ast[i])
>             continue;
>          c->audio_data[i] = av_fifo_alloc2(100 * MAX_AUDIO_FRAME_SIZE, 1, 0);
> -        if (!c->audio_data[i])
> -            goto bail_out;
> +        if (!c->audio_data[i]) {
> +            av_log(s, AV_LOG_ERROR,
> +                   "Failed to allocate internal buffer.\n");

Dedicated log messages for small malloc failures are useless bloat.

> +            return AVERROR(ENOMEM);
> +        }
>      }
>  
> -    return c;
> -
> -bail_out:
> -    return NULL;
> +    return 0;
>  }
>  
>  static int dv_write_header(AVFormatContext *s)
> @@ -392,10 +443,10 @@ static int dv_write_header(AVFormatContext *s)
>      DVMuxContext *dvc = s->priv_data;
>      AVDictionaryEntry *tcr = av_dict_get(s->metadata, "timecode", NULL, 0);
>  
> -    if (!dv_init_mux(s)) {
> +    if (dv_init_mux(s) < 0) {
>          av_log(s, AV_LOG_ERROR, "Can't initialize DV format!\n"
>                      "Make sure that you supply exactly two streams:\n"

This seems inconsistent with the other checks.

> -                    "     video: 25fps or 29.97fps, audio: 2ch/48|44|32kHz/PCM\n"
> +                    "     video: 25fps or 29.97fps, audio: 2ch/48000|44100|32000Hz/PCM\n"

This does not seem like an improvement.

-- 
Anton Khirnov