| Message ID | 20240120152408.606235-1-stefasab@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] lavf/dvenc: improve error messaging | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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.
Quoting Stefano Sabatini (2024-01-21 11:30:27) > > > - 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. More complicated than duplicating 5 lines? I wouldn't say so, not to mention the string also has to be duplicated in the binary. Also, can the second case even trigger? Seems like the block above ensures n_ast is never larger than 2. > > > - " 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 It can be.
On date Sunday 2024-01-21 18:39:19 +0100, Anton Khirnov wrote: > Quoting Stefano Sabatini (2024-01-21 11:30:27) [...] > Also, can the second case even trigger? Seems like the block above > ensures n_ast is never larger than 2. Yes, this seems a miss from commit eafa8e859297813dcf11110e6b43e85720be0a5f.
On date Sunday 2024-01-21 20:18:28 +0100, Stefano Sabatini wrote: > On date Sunday 2024-01-21 18:39:19 +0100, Anton Khirnov wrote: > > Quoting Stefano Sabatini (2024-01-21 11:30:27) > [...] > > Also, can the second case even trigger? Seems like the block above > > ensures n_ast is never larger than 2. > > Yes, this seems a miss from commit > eafa8e859297813dcf11110e6b43e85720be0a5f. Updated, keeping the somehow faulty logic as unrelated to the change (should be fixed separately). Adding Baptiste in CC: for the issue with the number of channels. The obvious fix seems to restore the limit of max 3 channels, and delete ast[2] and ast[3], but maybe Baptiste can hint at the proper fix.
diff --git a/libavformat/dvenc.c b/libavformat/dvenc.c index 29d2dc47ac..9a89b8e4f5 100644 --- a/libavformat/dvenc.c +++ b/libavformat/dvenc.c @@ -39,6 +39,7 @@ #include "libavutil/fifo.h" #include "libavutil/mathematics.h" #include "libavutil/intreadwrite.h" +#include "libavutil/pixdesc.h" #include "libavutil/timecode.h" #define MAX_AUDIO_FRAME_SIZE 192000 // 1 second of 48khz 32-bit audio @@ -308,62 +309,112 @@ static int dv_assemble_frame(AVFormatContext *s, return 0; } -static DVMuxContext* dv_init_mux(AVFormatContext* s) +static int dv_init_mux(AVFormatContext* s) { DVMuxContext *c = s->priv_data; AVStream *vst = NULL; int i; - /* we support at most 1 video and 2 audio streams */ - if (s->nb_streams > 5) - return NULL; + if (s->nb_streams > 5) { + av_log(s, AV_LOG_ERROR, + "Invalid number of streams %d, the muxer supports at most 1 video channel and 4 audio channels.\n", + s->nb_streams); + return AVERROR_INVALIDDATA; + } /* We have to sort out where audio and where video stream is */ for (i=0; i<s->nb_streams; i++) { AVStream *st = s->streams[i]; switch (st->codecpar->codec_type) { case AVMEDIA_TYPE_VIDEO: - if (vst) return NULL; - if (st->codecpar->codec_id != AV_CODEC_ID_DVVIDEO) - goto bail_out; + if (vst) { + av_log(s, AV_LOG_ERROR, + "More than one video stream found, only one is accepted.\n"); + return AVERROR_INVALIDDATA; + } + if (st->codecpar->codec_id != AV_CODEC_ID_DVVIDEO) { + av_log(s, AV_LOG_ERROR, + "Invalid codec for video stream, only DVVIDEO is supported.\n"); + return AVERROR_INVALIDDATA; + } vst = st; break; case AVMEDIA_TYPE_AUDIO: - if (c->n_ast > 1) return NULL; + if (c->n_ast > 1) { + av_log(s, AV_LOG_ERROR, + "More than two audio streams found, at most 2 are accepted.\n"); + return AVERROR_INVALIDDATA; + } /* Some checks -- DV format is very picky about its incoming streams */ - if(st->codecpar->codec_id != AV_CODEC_ID_PCM_S16LE || - st->codecpar->ch_layout.nb_channels != 2) - goto bail_out; + if (st->codecpar->codec_id != AV_CODEC_ID_PCM_S16LE) { + av_log(s, AV_LOG_ERROR, + "Invalid codec for stream %d, only PCM_S16LE is supported\n.", i); + return AVERROR_INVALIDDATA; + } + if (st->codecpar->ch_layout.nb_channels != 2) { + av_log(s, AV_LOG_ERROR, + "Invalid number of audio channels %d for stream %d, only 2 channels are supported\n.", + st->codecpar->ch_layout.nb_channels, i); + return AVERROR_INVALIDDATA; + } if (st->codecpar->sample_rate != 48000 && st->codecpar->sample_rate != 44100 && - st->codecpar->sample_rate != 32000 ) - goto bail_out; + st->codecpar->sample_rate != 32000) { + av_log(s, AV_LOG_ERROR, + "Invalid audio sample rate %d for stream %d, only 32000, 44100, and 48000 are supported.\n", + st->codecpar->sample_rate, i); + return AVERROR_INVALIDDATA; + } c->ast[c->n_ast++] = st; break; default: - goto bail_out; + av_log(s, AV_LOG_ERROR, + "Invalid media type for stream %d, only audio and video are supported.\n", i); + return AVERROR_INVALIDDATA; } } - if (!vst) - goto bail_out; + if (!vst) { + av_log(s, AV_LOG_ERROR, + "Missing video stream, must be present\n"); + return AVERROR_INVALIDDATA; + } c->sys = av_dv_codec_profile2(vst->codecpar->width, vst->codecpar->height, vst->codecpar->format, vst->time_base); - if (!c->sys) - goto bail_out; + if (!c->sys) { + av_log(s, AV_LOG_ERROR, + "Could not find a valid video profile for size:%dx%d format:%s tb:%d%d\n", + vst->codecpar->width, vst->codecpar->height, + av_get_pix_fmt_name(vst->codecpar->format), + vst->time_base.num, vst->time_base.den); + return AVERROR_INVALIDDATA; + } 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; + + for (j = 0; j < 2; j++) { + if (c->ast[j] && c->ast[j]->codecpar->sample_rate != 48000) { + av_log(s, AV_LOG_ERROR, + "Invalid sample rate %d for audio stream #%d for this video profile, must be 48000.\n", + c->ast[j]->codecpar->sample_rate, j); + return AVERROR_INVALIDDATA; + } + } } - 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; } /* 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"); + 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" - " video: 25fps or 29.97fps, audio: 2ch/48|44|32kHz/PCM\n" + " video: 25fps or 29.97fps, audio: 2ch/48000|44100|32000Hz/PCM\n" " (50Mbps allows an optional second audio stream)\n"); return -1; }