| Message ID | AM7PR03MB666098E552DC40F2A180218D8FCD9@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Commit | 5312208f1274683610d1b677517b953603f48d2b |
| Headers | show |
| Series | [FFmpeg-devel] Revert "ffmpeg: force 128k default audio bitrate if nothing is specified and there is no specific default" | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Andreas Rheinhardt: > This reverts commit 628a73f8f3768513fa6152c98d54842cf2ae1aad. > > At the time of said commit there was talk of removing the audio bitrate > "ab" option to bring FFmpeg in line with what Libav has done in 2012 in > commit 041cd5a0c55e02dd3b9a2419644b03103819c3d3. By having different > option flags for the "ab" and the ordinay bitrate "b" option is is > possible to have different default bitrates for audio and video. In > order to maintain this behaviour and not break user scripts the commit > to be reverted added code to ffmpeg.c that set the bitrate value to the > audio default for audio codecs, but only if AVCodec.defaults didn't > exist (as in this case the default would be codec-default and not > affected by the "ab" removal). > > This had the downside of being an API violation, because > AVCodec.defaults is not a public field. Given that the "ab" option > and its audio-specific default value have never been removed, > said API violation can be simply fixed by reverting said commit. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > There are a few more instances of fftools violating the API besides > this one and the one in [1]: > a) ffprobe accesses AVProgram.start_time and AVProgram.end_time. > I only see two options here: Don't report these values or make > them public. > b) cmdutils accesses AVFilter.process_command. This could be fixed > by adding an AVFILTER_FLAG_SUPPORT_COMMANDS and checking for that > instead. > c) ffplay checks for AVInputFormat.read_seek and the seeking flags > to determine whether seeking is available. The way it does this seems > wrong: It checks for whether any (not all!) of the default seeking methods > (binary, generic, byte seek) is disabled and for whether there is > a dedicated read_seek function. This could be fixed by adding a flag > for AVInputFormats that tells whether seeking is supported > (we could even distinguish the cases of "files of this format are > usually well seekable" (e.g. because they usually contain an index) > and "seeking is not that good, but works" (because e.g. it is just > the generic seeking code that reads all packets linearly). > > Given that the seeking flags (with the exception of AVFMT_NO_BYTE_SEEK) > are actually implementation details that are probably only public > because there were no internal flags for AVInputFormat until recently > this should be done at the same time as deprecating these fields. > > [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/284647.html > > fftools/ffmpeg.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > index b0ce7c7c32..ecff156529 100644 > --- a/fftools/ffmpeg.c > +++ b/fftools/ffmpeg.c > @@ -3571,11 +3571,6 @@ static int init_output_stream(OutputStream *ost, AVFrame *frame, > } > if (!av_dict_get(ost->encoder_opts, "threads", NULL, 0)) > av_dict_set(&ost->encoder_opts, "threads", "auto", 0); > - if (ost->enc->type == AVMEDIA_TYPE_AUDIO && > - !codec->defaults && > - !av_dict_get(ost->encoder_opts, "b", NULL, 0) && > - !av_dict_get(ost->encoder_opts, "ab", NULL, 0)) > - av_dict_set(&ost->encoder_opts, "b", "128000", 0); > > ret = hw_device_setup_for_encode(ost); > if (ret < 0) { > Will apply this tomorrow unless there are objections. I am also still seeking opinions on the above mentioned issues. - Andreas
diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index b0ce7c7c32..ecff156529 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -3571,11 +3571,6 @@ static int init_output_stream(OutputStream *ost, AVFrame *frame, } if (!av_dict_get(ost->encoder_opts, "threads", NULL, 0)) av_dict_set(&ost->encoder_opts, "threads", "auto", 0); - if (ost->enc->type == AVMEDIA_TYPE_AUDIO && - !codec->defaults && - !av_dict_get(ost->encoder_opts, "b", NULL, 0) && - !av_dict_get(ost->encoder_opts, "ab", NULL, 0)) - av_dict_set(&ost->encoder_opts, "b", "128000", 0); ret = hw_device_setup_for_encode(ost); if (ret < 0) {
This reverts commit 628a73f8f3768513fa6152c98d54842cf2ae1aad. At the time of said commit there was talk of removing the audio bitrate "ab" option to bring FFmpeg in line with what Libav has done in 2012 in commit 041cd5a0c55e02dd3b9a2419644b03103819c3d3. By having different option flags for the "ab" and the ordinay bitrate "b" option is is possible to have different default bitrates for audio and video. In order to maintain this behaviour and not break user scripts the commit to be reverted added code to ffmpeg.c that set the bitrate value to the audio default for audio codecs, but only if AVCodec.defaults didn't exist (as in this case the default would be codec-default and not affected by the "ab" removal). This had the downside of being an API violation, because AVCodec.defaults is not a public field. Given that the "ab" option and its audio-specific default value have never been removed, said API violation can be simply fixed by reverting said commit. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- There are a few more instances of fftools violating the API besides this one and the one in [1]: a) ffprobe accesses AVProgram.start_time and AVProgram.end_time. I only see two options here: Don't report these values or make them public. b) cmdutils accesses AVFilter.process_command. This could be fixed by adding an AVFILTER_FLAG_SUPPORT_COMMANDS and checking for that instead. c) ffplay checks for AVInputFormat.read_seek and the seeking flags to determine whether seeking is available. The way it does this seems wrong: It checks for whether any (not all!) of the default seeking methods (binary, generic, byte seek) is disabled and for whether there is a dedicated read_seek function. This could be fixed by adding a flag for AVInputFormats that tells whether seeking is supported (we could even distinguish the cases of "files of this format are usually well seekable" (e.g. because they usually contain an index) and "seeking is not that good, but works" (because e.g. it is just the generic seeking code that reads all packets linearly). Given that the seeking flags (with the exception of AVFMT_NO_BYTE_SEEK) are actually implementation details that are probably only public because there were no internal flags for AVInputFormat until recently this should be done at the same time as deprecating these fields. [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/284647.html fftools/ffmpeg.c | 5 ----- 1 file changed, 5 deletions(-)