diff mbox series

[FFmpeg-devel,3/3] avcodec/videotoolboxenc: Fix variable type of AV_OPT_TYPE_BOOL

Message ID tencent_B27DC2AC8A395DC3F9B77069EE00DD69CE0A@qq.com
State New
Headers show
Series None | expand

Commit Message

Zhao Zhili July 16, 2024, 2:46 p.m. UTC
From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavcodec/videotoolboxenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Rémi Denis-Courmont July 16, 2024, 2:53 p.m. UTC | #1
Le tiistaina 16. heinäkuuta 2024, 17.46.53 EEST Zhao Zhili a écrit :
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> ---
>  libavcodec/videotoolboxenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> index 747681fd05..78ef474d7a 100644
> --- a/libavcodec/videotoolboxenc.c
> +++ b/libavcodec/videotoolboxenc.c
> @@ -262,7 +262,7 @@ typedef struct VTEncContext {
>      int realtime;
>      int frames_before;
>      int frames_after;
> -    bool constant_bit_rate;
> +    int constant_bit_rate;
> 
>      int allow_sw;
>      int require_sw;

Shouldn't it be fixed the opposite way, actually assigning to or evaluating as 
a boolean?
James Almer July 16, 2024, 3:10 p.m. UTC | #2
On 7/16/2024 11:53 AM, Rémi Denis-Courmont wrote:
> Le tiistaina 16. heinäkuuta 2024, 17.46.53 EEST Zhao Zhili a écrit :
>> From: Zhao Zhili <zhilizhao@tencent.com>
>>
>> ---
>>   libavcodec/videotoolboxenc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
>> index 747681fd05..78ef474d7a 100644
>> --- a/libavcodec/videotoolboxenc.c
>> +++ b/libavcodec/videotoolboxenc.c
>> @@ -262,7 +262,7 @@ typedef struct VTEncContext {
>>       int realtime;
>>       int frames_before;
>>       int frames_after;
>> -    bool constant_bit_rate;
>> +    int constant_bit_rate;
>>
>>       int allow_sw;
>>       int require_sw;
> 
> Shouldn't it be fixed the opposite way, actually assigning to or evaluating as
> a boolean?

No, because -1 is allowed for AV_OPT_TYPE_BOOL, which is used as 
"undefined" in plenty of options.
And we don't use bool type anywhere as it's not really portable, or at 
least didn't use to. Dunno the current state ever since we moved to c11.
Rémi Denis-Courmont July 16, 2024, 3:15 p.m. UTC | #3
Le tiistaina 16. heinäkuuta 2024, 18.10.28 EEST James Almer a écrit :
> No, because -1 is allowed for AV_OPT_TYPE_BOOL, which is used as
> "undefined" in plenty of options.
> And we don't use bool type anywhere as it's not really portable, or at
> least didn't use to. Dunno the current state ever since we moved to c11.

stdbool.h is mandatory in C11. Using booleans is generally better since it 
enables better inference for optimisation and diagnostic passes.
Zhao Zhili July 16, 2024, 3:18 p.m. UTC | #4
> On Jul 16, 2024, at 23:10, James Almer <jamrial@gmail.com> wrote:
> 
> On 7/16/2024 11:53 AM, Rémi Denis-Courmont wrote:
>> Le tiistaina 16. heinäkuuta 2024, 17.46.53 EEST Zhao Zhili a écrit :
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>> 
>>> ---
>>>  libavcodec/videotoolboxenc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
>>> index 747681fd05..78ef474d7a 100644
>>> --- a/libavcodec/videotoolboxenc.c
>>> +++ b/libavcodec/videotoolboxenc.c
>>> @@ -262,7 +262,7 @@ typedef struct VTEncContext {
>>>      int realtime;
>>>      int frames_before;
>>>      int frames_after;
>>> -    bool constant_bit_rate;
>>> +    int constant_bit_rate;
>>> 
>>>      int allow_sw;
>>>      int require_sw;
>> Shouldn't it be fixed the opposite way, actually assigning to or evaluating as
>> a boolean?
> 
> No, because -1 is allowed for AV_OPT_TYPE_BOOL, which is used as "undefined" in plenty of options.
> And we don't use bool type anywhere as it's not really portable, or at least didn't use to. Dunno the current state ever since we moved to c11.

Yes, it’s unlikely to update opt.c to use bool as AV_OPT_TYPE_BOOL.
And maybe it’s still a rule to not use bool in public header file.
For other cases, I see no reason to forbidden it.

> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
index 747681fd05..78ef474d7a 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -262,7 +262,7 @@  typedef struct VTEncContext {
     int realtime;
     int frames_before;
     int frames_after;
-    bool constant_bit_rate;
+    int constant_bit_rate;
 
     int allow_sw;
     int require_sw;