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

[Stefano Sabatini]

On date Sunday 2024-01-21 07:36:48 +0100, Anton Khirnov wrote:
> 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.

fixed locally

> > -    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.

Yes, but this would complicate the logic for small gain.

> 
> >      }
> >  
> >      /* 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.

Dropped.

> 
> > +            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.

Yes, probably it's better to drop this entirely since we have more
puntual reporting now (and "exactly two stream" is wrong) 

> 
> > -                    "     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.

44kHz != 44100

I could use 44.1 but this is not the unit used when setting the
option, so better to be explicit.

Thanks.