diff mbox series

[FFmpeg-devel] ffmpeg: default hwaccel_output_format to cuda when hwaccel is cuvid

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
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Timo Rothenpieler March 6, 2020, 1:35 p.m. UTC
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(-)

Comments

James Almer March 6, 2020, 2:45 p.m. UTC | #1
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;
>
James Almer March 6, 2020, 10:17 p.m. UTC | #2
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;
>>
>
Timo Rothenpieler March 6, 2020, 10:34 p.m. UTC | #3
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.
>
Timo Rothenpieler March 6, 2020, 10:35 p.m. UTC | #4
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.
James Almer March 6, 2020, 10:39 p.m. UTC | #5
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".
Timo Rothenpieler March 6, 2020, 10:42 p.m. UTC | #6
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.
James Almer March 6, 2020, 10:58 p.m. UTC | #7
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 mbox series

Patch

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;