[FFmpeg-devel] [PATCH v2] avutil: deprecate AVRational field inside AVOption::default_val

[FFmpeg-devel] [PATCH v2] avutil: deprecate AVRational field inside AVOption::default_val

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
---
 libavutil/opt.h     | 2 ++
 libavutil/version.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/libavutil/opt.h b/libavutil/opt.h
index 461b5d3b6b..46915754ea 100644
--- a/libavutil/opt.h
+++ b/libavutil/opt.h
@@ -271,8 +271,10 @@ typedef struct AVOption {
         int64_t i64;
         double dbl;
         const char *str;
+#if FF_API_AVOPTION_AVRATIONAL
         /* TODO those are unused now */
         AVRational q;
+#endif
     } default_val;
     double min;                 ///< minimum valid value for the option
     double max;                 ///< maximum valid value for the option
diff --git a/libavutil/version.h b/libavutil/version.h
index 40f92af055..56af5c09c4 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -115,6 +115,7 @@
 #define FF_API_FRAME_PICTURE_NUMBER     (LIBAVUTIL_VERSION_MAJOR < 59)
 #define FF_API_HDR_VIVID_THREE_SPLINE   (LIBAVUTIL_VERSION_MAJOR < 59)
 #define FF_API_FRAME_PKT                (LIBAVUTIL_VERSION_MAJOR < 59)
+#define FF_API_AVOPTION_AVRATIONAL      (LIBAVUTIL_VERSION_MAJOR < 59)
 
 /**
  * @}
-- 
2.40.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avutil: deprecate AVRational field inside AVOption::default_val

[Paul B Mahol]

NAK

It can be made useful.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avutil: deprecate AVRational field inside AVOption::default_val

[Tomas Härdin]

tis 2023-05-02 klockan 15:48 +0800 skrev Zhao Zhili:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
>  libavutil/opt.h     | 2 ++
>  libavutil/version.h | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 461b5d3b6b..46915754ea 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -271,8 +271,10 @@ typedef struct AVOption {
>          int64_t i64;
>          double dbl;
>          const char *str;
> +#if FF_API_AVOPTION_AVRATIONAL
>          /* TODO those are unused now */
>          AVRational q;
> +#endif

Surely rationals options are useful?

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avutil: deprecate AVRational field inside AVOption::default_val

[James Almer]

On 5/2/2023 8:34 AM, Tomas Härdin wrote:
> tis 2023-05-02 klockan 15:48 +0800 skrev Zhao Zhili:
>> From: Zhao Zhili <zhilizhao@tencent.com>
>>
>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
>> ---
>>   libavutil/opt.h     | 2 ++
>>   libavutil/version.h | 1 +
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/libavutil/opt.h b/libavutil/opt.h
>> index 461b5d3b6b..46915754ea 100644
>> --- a/libavutil/opt.h
>> +++ b/libavutil/opt.h
>> @@ -271,8 +271,10 @@ typedef struct AVOption {
>>           int64_t i64;
>>           double dbl;
>>           const char *str;
>> +#if FF_API_AVOPTION_AVRATIONAL
>>           /* TODO those are unused now */
>>           AVRational q;
>> +#endif
> 
> Surely rationals options are useful?

They are, but this union is where the default value is stored when you 
define an AVOption. At some point it seems it was decided that rational 
(and video_rate) type AVOptions should set dbl instead of q, which is 
then av_q2d()'d into an AVRational.

I'd still not remove it, as Paul said. It's harmless being there and in 
the future it could be used.

> 
> /Tomas
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avutil: deprecate AVRational field inside AVOption::default_val

[Paul B Mahol]

On Tue, May 2, 2023 at 1:40 PM James Almer <jamrial@gmail.com> wrote:

> On 5/2/2023 8:34 AM, Tomas Härdin wrote:
> > tis 2023-05-02 klockan 15:48 +0800 skrev Zhao Zhili:
> >> From: Zhao Zhili <zhilizhao@tencent.com>
> >>
> >> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> >> ---
> >>   libavutil/opt.h     | 2 ++
> >>   libavutil/version.h | 1 +
> >>   2 files changed, 3 insertions(+)
> >>
> >> diff --git a/libavutil/opt.h b/libavutil/opt.h
> >> index 461b5d3b6b..46915754ea 100644
> >> --- a/libavutil/opt.h
> >> +++ b/libavutil/opt.h
> >> @@ -271,8 +271,10 @@ typedef struct AVOption {
> >>           int64_t i64;
> >>           double dbl;
> >>           const char *str;
> >> +#if FF_API_AVOPTION_AVRATIONAL
> >>           /* TODO those are unused now */
> >>           AVRational q;
> >> +#endif
> >
> > Surely rationals options are useful?
>
> They are, but this union is where the default value is stored when you
> define an AVOption. At some point it seems it was decided that rational
> (and video_rate) type AVOptions should set dbl instead of q, which is
> then av_q2d()'d into an AVRational.
>
> I'd still not remove it, as Paul said. It's harmless being there and in
> the future it could be used.
>

Yes, I think that using double for AVRational is not always correct/precise.
Do anyone remember why it is double?
I can send patch to fix this. But I dunno if that may break API and/or ABI.


>
> >
> > /Tomas
> >
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
>
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avutil: deprecate AVRational field inside AVOption::default_val

[Zhao Zhili]

> 在 2023年5月2日，19:39，James Almer <jamrial@gmail.com> 写道：
> 
> On 5/2/2023 8:34 AM, Tomas Härdin wrote:
>> tis 2023-05-02 klockan 15:48 +0800 skrev Zhao Zhili:
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>> 
>>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
>>> ---
>>>  libavutil/opt.h     | 2 ++
>>>  libavutil/version.h | 1 +
>>>  2 files changed, 3 insertions(+)
>>> 
>>> diff --git a/libavutil/opt.h b/libavutil/opt.h
>>> index 461b5d3b6b..46915754ea 100644
>>> --- a/libavutil/opt.h
>>> +++ b/libavutil/opt.h
>>> @@ -271,8 +271,10 @@ typedef struct AVOption {
>>>          int64_t i64;
>>>          double dbl;
>>>          const char *str;
>>> +#if FF_API_AVOPTION_AVRATIONAL
>>>          /* TODO those are unused now */
>>>          AVRational q;
>>> +#endif
>> Surely rationals options are useful?
> 
> They are, but this union is where the default value is stored when you define an AVOption. At some point it seems it was decided that rational (and video_rate) type AVOptions should set dbl instead of q, which is then av_q2d()'d into an AVRational.
> 
> I'd still not remove it, as Paul said. It's harmless being there and in the future it could be used.

The field itself is harmless, but it makes big confusing. It takes me some efforts to figure out a rational type must use dbl to hold default value.

For backward compatibility, I don’t think it’s easy to switch to AVRational from dbl.

If we decide to keep it, I can add a comment to reduce the confusion.

> 
>> /Tomas
>> _______________________________________________
>> 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".
> _______________________________________________
> 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 sub

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avutil: deprecate AVRational field inside AVOption::default_val

[Tomas Härdin]

tis 2023-05-02 klockan 08:39 -0300 skrev James Almer:
> On 5/2/2023 8:34 AM, Tomas Härdin wrote:
> > tis 2023-05-02 klockan 15:48 +0800 skrev Zhao Zhili:
> > > From: Zhao Zhili <zhilizhao@tencent.com>
> > > 
> > > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> > > ---
> > >   libavutil/opt.h     | 2 ++
> > >   libavutil/version.h | 1 +
> > >   2 files changed, 3 insertions(+)
> > > 
> > > diff --git a/libavutil/opt.h b/libavutil/opt.h
> > > index 461b5d3b6b..46915754ea 100644
> > > --- a/libavutil/opt.h
> > > +++ b/libavutil/opt.h
> > > @@ -271,8 +271,10 @@ typedef struct AVOption {
> > >           int64_t i64;
> > >           double dbl;
> > >           const char *str;
> > > +#if FF_API_AVOPTION_AVRATIONAL
> > >           /* TODO those are unused now */
> > >           AVRational q;
> > > +#endif
> > 
> > Surely rationals options are useful?
> 
> They are, but this union is where the default value is stored when
> you 
> define an AVOption. At some point it seems it was decided that
> rational 
> (and video_rate) type AVOptions should set dbl instead of q, which is
> then av_q2d()'d into an AVRational.

Cursed and very wrong in many contexts

/Tomas

_______________________________________________
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".