| Message ID | 20200306133513.21478-1-timo@rothenpieler.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] ffmpeg: default hwaccel_output_format to cuda when hwaccel is cuvid | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On 3/6/2020 10:35 AM, Timo Rothenpieler wrote: > This ensures old commandlines using -hwaccel cuvid don't break due to > the recent removal of the the cuvid-specific hwaccel bringup. > --- > fftools/ffmpeg_opt.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c > index 1b721c4954..c8fe263730 100644 > --- a/fftools/ffmpeg_opt.c > +++ b/fftools/ffmpeg_opt.c > @@ -816,6 +816,20 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) > ist->top_field_first = -1; > MATCH_PER_STREAM_OPT(top_field_first, i, ist->top_field_first, ic, st); > > + MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, > + hwaccel_output_format, ic, st); > + if (!hwaccel_output_format && hwaccel && !strcmp(hwaccel, "cuvid")) { Isn't hwaccel first set in the MATCH_PER_STREAM_OPT() call bellow? > + ist->hwaccel_output_format = AV_PIX_FMT_CUDA; > + } else if (hwaccel_output_format) { > + ist->hwaccel_output_format = av_get_pix_fmt(hwaccel_output_format); > + if (ist->hwaccel_output_format == AV_PIX_FMT_NONE) { > + av_log(NULL, AV_LOG_FATAL, "Unrecognised hwaccel output " > + "format: %s", hwaccel_output_format); > + } > + } else { > + ist->hwaccel_output_format = AV_PIX_FMT_NONE; > + } > + > MATCH_PER_STREAM_OPT(hwaccels, str, hwaccel, ic, st); > if (hwaccel) { > // The NVDEC hwaccels use a CUDA device, so remap the name here. > @@ -868,18 +882,6 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) > exit_program(1); > } > > - MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, > - hwaccel_output_format, ic, st); > - if (hwaccel_output_format) { > - ist->hwaccel_output_format = av_get_pix_fmt(hwaccel_output_format); > - if (ist->hwaccel_output_format == AV_PIX_FMT_NONE) { > - av_log(NULL, AV_LOG_FATAL, "Unrecognised hwaccel output " > - "format: %s", hwaccel_output_format); > - } > - } else { > - ist->hwaccel_output_format = AV_PIX_FMT_NONE; > - } > - > ist->hwaccel_pix_fmt = AV_PIX_FMT_NONE; > > break; >
On 3/6/2020 11:45 AM, James Almer wrote: > On 3/6/2020 10:35 AM, Timo Rothenpieler wrote: >> This ensures old commandlines using -hwaccel cuvid don't break due to >> the recent removal of the the cuvid-specific hwaccel bringup. >> --- >> fftools/ffmpeg_opt.c | 26 ++++++++++++++------------ >> 1 file changed, 14 insertions(+), 12 deletions(-) >> >> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c >> index 1b721c4954..c8fe263730 100644 >> --- a/fftools/ffmpeg_opt.c >> +++ b/fftools/ffmpeg_opt.c >> @@ -816,6 +816,20 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) >> ist->top_field_first = -1; >> MATCH_PER_STREAM_OPT(top_field_first, i, ist->top_field_first, ic, st); >> >> + MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, >> + hwaccel_output_format, ic, st); >> + if (!hwaccel_output_format && hwaccel && !strcmp(hwaccel, "cuvid")) { > > Isn't hwaccel first set in the MATCH_PER_STREAM_OPT() call bellow? This change also needs both a code comment explaining why it's being done, and a log message informing the CLI user a hardware pix_fmt is forcefully being chosen for backwards compat reasons, that it's deprecated behavior, and that it will stop working after a while. > >> + ist->hwaccel_output_format = AV_PIX_FMT_CUDA; >> + } else if (hwaccel_output_format) { >> + ist->hwaccel_output_format = av_get_pix_fmt(hwaccel_output_format); >> + if (ist->hwaccel_output_format == AV_PIX_FMT_NONE) { >> + av_log(NULL, AV_LOG_FATAL, "Unrecognised hwaccel output " >> + "format: %s", hwaccel_output_format); >> + } >> + } else { >> + ist->hwaccel_output_format = AV_PIX_FMT_NONE; >> + } >> + >> MATCH_PER_STREAM_OPT(hwaccels, str, hwaccel, ic, st); >> if (hwaccel) { >> // The NVDEC hwaccels use a CUDA device, so remap the name here. >> @@ -868,18 +882,6 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) >> exit_program(1); >> } >> >> - MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, >> - hwaccel_output_format, ic, st); >> - if (hwaccel_output_format) { >> - ist->hwaccel_output_format = av_get_pix_fmt(hwaccel_output_format); >> - if (ist->hwaccel_output_format == AV_PIX_FMT_NONE) { >> - av_log(NULL, AV_LOG_FATAL, "Unrecognised hwaccel output " >> - "format: %s", hwaccel_output_format); >> - } >> - } else { >> - ist->hwaccel_output_format = AV_PIX_FMT_NONE; >> - } >> - >> ist->hwaccel_pix_fmt = AV_PIX_FMT_NONE; >> >> break; >> >
On 06.03.2020 15:45, James Almer wrote: > On 3/6/2020 10:35 AM, Timo Rothenpieler wrote: >> This ensures old commandlines using -hwaccel cuvid don't break due to >> the recent removal of the the cuvid-specific hwaccel bringup. >> --- >> fftools/ffmpeg_opt.c | 26 ++++++++++++++------------ >> 1 file changed, 14 insertions(+), 12 deletions(-) >> >> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c >> index 1b721c4954..c8fe263730 100644 >> --- a/fftools/ffmpeg_opt.c >> +++ b/fftools/ffmpeg_opt.c >> @@ -816,6 +816,20 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) >> ist->top_field_first = -1; >> MATCH_PER_STREAM_OPT(top_field_first, i, ist->top_field_first, ic, st); >> >> + MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, >> + hwaccel_output_format, ic, st); >> + if (!hwaccel_output_format && hwaccel && !strcmp(hwaccel, "cuvid")) { > > Isn't hwaccel first set in the MATCH_PER_STREAM_OPT() call bellow? Indeed, that also needs to move up. >
On 06.03.2020 23:17, James Almer wrote: > On 3/6/2020 11:45 AM, James Almer wrote: >> On 3/6/2020 10:35 AM, Timo Rothenpieler wrote: >>> This ensures old commandlines using -hwaccel cuvid don't break due to >>> the recent removal of the the cuvid-specific hwaccel bringup. >>> --- >>> fftools/ffmpeg_opt.c | 26 ++++++++++++++------------ >>> 1 file changed, 14 insertions(+), 12 deletions(-) >>> >>> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c >>> index 1b721c4954..c8fe263730 100644 >>> --- a/fftools/ffmpeg_opt.c >>> +++ b/fftools/ffmpeg_opt.c >>> @@ -816,6 +816,20 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) >>> ist->top_field_first = -1; >>> MATCH_PER_STREAM_OPT(top_field_first, i, ist->top_field_first, ic, st); >>> >>> + MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, >>> + hwaccel_output_format, ic, st); >>> + if (!hwaccel_output_format && hwaccel && !strcmp(hwaccel, "cuvid")) { >> >> Isn't hwaccel first set in the MATCH_PER_STREAM_OPT() call bellow? > > This change also needs both a code comment explaining why it's being > done, and a log message informing the CLI user a hardware pix_fmt is > forcefully being chosen for backwards compat reasons, that it's > deprecated behavior, and that it will stop working after a while. It can still be overridden by the user, but yeah, a deprecation warning is probably in order. For both cuvid and nvdec actually.
On 3/6/2020 7:35 PM, Timo Rothenpieler wrote: > On 06.03.2020 23:17, James Almer wrote: >> On 3/6/2020 11:45 AM, James Almer wrote: >>> On 3/6/2020 10:35 AM, Timo Rothenpieler wrote: >>>> This ensures old commandlines using -hwaccel cuvid don't break due to >>>> the recent removal of the the cuvid-specific hwaccel bringup. >>>> --- >>>> fftools/ffmpeg_opt.c | 26 ++++++++++++++------------ >>>> 1 file changed, 14 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c >>>> index 1b721c4954..c8fe263730 100644 >>>> --- a/fftools/ffmpeg_opt.c >>>> +++ b/fftools/ffmpeg_opt.c >>>> @@ -816,6 +816,20 @@ static void add_input_streams(OptionsContext >>>> *o, AVFormatContext *ic) >>>> ist->top_field_first = -1; >>>> MATCH_PER_STREAM_OPT(top_field_first, i, >>>> ist->top_field_first, ic, st); >>>> + MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, >>>> + hwaccel_output_format, ic, st); >>>> + if (!hwaccel_output_format && hwaccel && >>>> !strcmp(hwaccel, "cuvid")) { >>> >>> Isn't hwaccel first set in the MATCH_PER_STREAM_OPT() call bellow? >> >> This change also needs both a code comment explaining why it's being >> done, and a log message informing the CLI user a hardware pix_fmt is >> forcefully being chosen for backwards compat reasons, that it's >> deprecated behavior, and that it will stop working after a while. > > It can still be overridden by the user, but yeah, a deprecation warning > is probably in order. > For both cuvid and nvdec actually. nvdec didn't default to the cuda pixfmt, afaik, so not necessary for that one. Or you mean you want to deprecate the nvdec alias and keep only "cuda"? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 06.03.2020 23:39, James Almer wrote: > On 3/6/2020 7:35 PM, Timo Rothenpieler wrote: >> On 06.03.2020 23:17, James Almer wrote: >>> On 3/6/2020 11:45 AM, James Almer wrote: >>>> On 3/6/2020 10:35 AM, Timo Rothenpieler wrote: >>>>> This ensures old commandlines using -hwaccel cuvid don't break due to >>>>> the recent removal of the the cuvid-specific hwaccel bringup. >>>>> --- >>>>> fftools/ffmpeg_opt.c | 26 ++++++++++++++------------ >>>>> 1 file changed, 14 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c >>>>> index 1b721c4954..c8fe263730 100644 >>>>> --- a/fftools/ffmpeg_opt.c >>>>> +++ b/fftools/ffmpeg_opt.c >>>>> @@ -816,6 +816,20 @@ static void add_input_streams(OptionsContext >>>>> *o, AVFormatContext *ic) >>>>> ist->top_field_first = -1; >>>>> MATCH_PER_STREAM_OPT(top_field_first, i, >>>>> ist->top_field_first, ic, st); >>>>> + MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, >>>>> + hwaccel_output_format, ic, st); >>>>> + if (!hwaccel_output_format && hwaccel && >>>>> !strcmp(hwaccel, "cuvid")) { >>>> >>>> Isn't hwaccel first set in the MATCH_PER_STREAM_OPT() call bellow? >>> >>> This change also needs both a code comment explaining why it's being >>> done, and a log message informing the CLI user a hardware pix_fmt is >>> forcefully being chosen for backwards compat reasons, that it's >>> deprecated behavior, and that it will stop working after a while. >> >> It can still be overridden by the user, but yeah, a deprecation warning >> is probably in order. >> For both cuvid and nvdec actually. > > nvdec didn't default to the cuda pixfmt, afaik, so not necessary for > that one. Or you mean you want to deprecate the nvdec alias and keep > only "cuda"? Yes, as seen on recent github threads, the multitude of different -hwaccel names, which do the exact same thing, causes great confusion.
On 3/6/2020 7:42 PM, Timo Rothenpieler wrote: > On 06.03.2020 23:39, James Almer wrote: >> On 3/6/2020 7:35 PM, Timo Rothenpieler wrote: >>> On 06.03.2020 23:17, James Almer wrote: >>>> On 3/6/2020 11:45 AM, James Almer wrote: >>>>> On 3/6/2020 10:35 AM, Timo Rothenpieler wrote: >>>>>> This ensures old commandlines using -hwaccel cuvid don't break due to >>>>>> the recent removal of the the cuvid-specific hwaccel bringup. >>>>>> --- >>>>>> fftools/ffmpeg_opt.c | 26 ++++++++++++++------------ >>>>>> 1 file changed, 14 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c >>>>>> index 1b721c4954..c8fe263730 100644 >>>>>> --- a/fftools/ffmpeg_opt.c >>>>>> +++ b/fftools/ffmpeg_opt.c >>>>>> @@ -816,6 +816,20 @@ static void add_input_streams(OptionsContext >>>>>> *o, AVFormatContext *ic) >>>>>> ist->top_field_first = -1; >>>>>> MATCH_PER_STREAM_OPT(top_field_first, i, >>>>>> ist->top_field_first, ic, st); >>>>>> + MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, >>>>>> + hwaccel_output_format, ic, st); >>>>>> + if (!hwaccel_output_format && hwaccel && >>>>>> !strcmp(hwaccel, "cuvid")) { >>>>> >>>>> Isn't hwaccel first set in the MATCH_PER_STREAM_OPT() call bellow? >>>> >>>> This change also needs both a code comment explaining why it's being >>>> done, and a log message informing the CLI user a hardware pix_fmt is >>>> forcefully being chosen for backwards compat reasons, that it's >>>> deprecated behavior, and that it will stop working after a while. >>> >>> It can still be overridden by the user, but yeah, a deprecation warning >>> is probably in order. >>> For both cuvid and nvdec actually. >> >> nvdec didn't default to the cuda pixfmt, afaik, so not necessary for >> that one. Or you mean you want to deprecate the nvdec alias and keep >> only "cuda"? > > Yes, as seen on recent github threads, the multitude of different > -hwaccel names, which do the exact same thing, causes great confusion. Not sure if removing the nvdec alias is a good idea, considering the lavc hwaccels are called like that. If i configure ffmpeg with --enable-nvdec --enable-hwaccel=hevc_nvdec, expecting a command line like -hwaccel nvdec to work is not outlandish. How about making sure cuvid is a term left exclusively for the relevant lavc hardware decoders, and nvdec for the relevant lavc hwaccels? Although I'm not sure how the hardware based filters play into this.
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index 1b721c4954..c8fe263730 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -816,6 +816,20 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) ist->top_field_first = -1; MATCH_PER_STREAM_OPT(top_field_first, i, ist->top_field_first, ic, st); + MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, + hwaccel_output_format, ic, st); + if (!hwaccel_output_format && hwaccel && !strcmp(hwaccel, "cuvid")) { + ist->hwaccel_output_format = AV_PIX_FMT_CUDA; + } else if (hwaccel_output_format) { + ist->hwaccel_output_format = av_get_pix_fmt(hwaccel_output_format); + if (ist->hwaccel_output_format == AV_PIX_FMT_NONE) { + av_log(NULL, AV_LOG_FATAL, "Unrecognised hwaccel output " + "format: %s", hwaccel_output_format); + } + } else { + ist->hwaccel_output_format = AV_PIX_FMT_NONE; + } + MATCH_PER_STREAM_OPT(hwaccels, str, hwaccel, ic, st); if (hwaccel) { // The NVDEC hwaccels use a CUDA device, so remap the name here. @@ -868,18 +882,6 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) exit_program(1); } - MATCH_PER_STREAM_OPT(hwaccel_output_formats, str, - hwaccel_output_format, ic, st); - if (hwaccel_output_format) { - ist->hwaccel_output_format = av_get_pix_fmt(hwaccel_output_format); - if (ist->hwaccel_output_format == AV_PIX_FMT_NONE) { - av_log(NULL, AV_LOG_FATAL, "Unrecognised hwaccel output " - "format: %s", hwaccel_output_format); - } - } else { - ist->hwaccel_output_format = AV_PIX_FMT_NONE; - } - ist->hwaccel_pix_fmt = AV_PIX_FMT_NONE; break;