diff mbox series

[FFmpeg-devel] lavu: make AV_TIME_BASE_Q work in C++ code

Message ID 20200624190958.47004-1-rcombs@rcombs.me
State New
Headers show
Series [FFmpeg-devel] lavu: make AV_TIME_BASE_Q work in C++ code
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

rcombs June 24, 2020, 7:09 p.m. UTC
---
 libavutil/avutil.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Carl Eugen Hoyos June 24, 2020, 7:22 p.m. UTC | #1
Am Mi., 24. Juni 2020 um 21:10 Uhr schrieb rcombs <rcombs@rcombs.me>:
>
> ---
>  libavutil/avutil.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> index 4d633156d1..c11b33f466 100644
> --- a/libavutil/avutil.h
> +++ b/libavutil/avutil.h
> @@ -257,7 +257,11 @@ const char *av_get_media_type_string(enum AVMediaType media_type);
>   * Internal time base represented as fractional value
>   */
>
> +#ifdef __cplusplus
> +#define AV_TIME_BASE_Q          AVRational{1, AV_TIME_BASE}
> +#else
>  #define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
> +#endif

The problem with this patch is that it gives C++ users of the libraries
the feeling that we would care about them and that we would indeed
fix issues.
Given that the contrary is true (at least in the past) and that we do
not do any specific C++ testing, I believe it is better to let users
work around this issue (from their point of you).

Carl Eugen
Tomas Härdin June 26, 2020, 9:30 a.m. UTC | #2
ons 2020-06-24 klockan 21:22 +0200 skrev Carl Eugen Hoyos:
> Am Mi., 24. Juni 2020 um 21:10 Uhr schrieb rcombs <rcombs@rcombs.me>:
> > ---
> >  libavutil/avutil.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> > index 4d633156d1..c11b33f466 100644
> > --- a/libavutil/avutil.h
> > +++ b/libavutil/avutil.h
> > @@ -257,7 +257,11 @@ const char *av_get_media_type_string(enum AVMediaType media_type);
> >   * Internal time base represented as fractional value
> >   */
> > 
> > +#ifdef __cplusplus
> > +#define AV_TIME_BASE_Q          AVRational{1, AV_TIME_BASE}
> > +#else
> >  #define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
> > +#endif
> 
> The problem with this patch is that it gives C++ users of the libraries
> the feeling that we would care about them and that we would indeed
> fix issues.
> Given that the contrary is true (at least in the past) and that we do
> not do any specific C++ testing, I believe it is better to let users
> work around this issue (from their point of you).

Maybe we should put in a #warning to that effect? Inside an #ifdef
__cplusplus

/Tomas
Hendrik Leppkes June 26, 2020, 9:36 a.m. UTC | #3
On Fri, Jun 26, 2020 at 11:31 AM Tomas Härdin <tjoppen@acc.umu.se> wrote:
>
> ons 2020-06-24 klockan 21:22 +0200 skrev Carl Eugen Hoyos:
> > Am Mi., 24. Juni 2020 um 21:10 Uhr schrieb rcombs <rcombs@rcombs.me>:
> > > ---
> > >  libavutil/avutil.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> > > index 4d633156d1..c11b33f466 100644
> > > --- a/libavutil/avutil.h
> > > +++ b/libavutil/avutil.h
> > > @@ -257,7 +257,11 @@ const char *av_get_media_type_string(enum AVMediaType media_type);
> > >   * Internal time base represented as fractional value
> > >   */
> > >
> > > +#ifdef __cplusplus
> > > +#define AV_TIME_BASE_Q          AVRational{1, AV_TIME_BASE}
> > > +#else
> > >  #define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
> > > +#endif
> >
> > The problem with this patch is that it gives C++ users of the libraries
> > the feeling that we would care about them and that we would indeed
> > fix issues.
> > Given that the contrary is true (at least in the past) and that we do
> > not do any specific C++ testing, I believe it is better to let users
> > work around this issue (from their point of you).
>
> Maybe we should put in a #warning to that effect? Inside an #ifdef
> __cplusplus
>

Now that would just be annoying. 99% of the headers work just fine in
C++, spewing a warning at us for using them would not serve anything.

- Hendrik
Tomas Härdin June 27, 2020, 9:46 a.m. UTC | #4
fre 2020-06-26 klockan 11:36 +0200 skrev Hendrik Leppkes:
> On Fri, Jun 26, 2020 at 11:31 AM Tomas Härdin <tjoppen@acc.umu.se> wrote:
> > ons 2020-06-24 klockan 21:22 +0200 skrev Carl Eugen Hoyos:
> > > Am Mi., 24. Juni 2020 um 21:10 Uhr schrieb rcombs <rcombs@rcombs.me>:
> > > > ---
> > > >  libavutil/avutil.h | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> > > > index 4d633156d1..c11b33f466 100644
> > > > --- a/libavutil/avutil.h
> > > > +++ b/libavutil/avutil.h
> > > > @@ -257,7 +257,11 @@ const char *av_get_media_type_string(enum AVMediaType media_type);
> > > >   * Internal time base represented as fractional value
> > > >   */
> > > > 
> > > > +#ifdef __cplusplus
> > > > +#define AV_TIME_BASE_Q          AVRational{1, AV_TIME_BASE}
> > > > +#else
> > > >  #define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
> > > > +#endif
> > > 
> > > The problem with this patch is that it gives C++ users of the libraries
> > > the feeling that we would care about them and that we would indeed
> > > fix issues.
> > > Given that the contrary is true (at least in the past) and that we do
> > > not do any specific C++ testing, I believe it is better to let users
> > > work around this issue (from their point of you).
> > 
> > Maybe we should put in a #warning to that effect? Inside an #ifdef
> > __cplusplus
> > 
> 
> Now that would just be annoying. 99% of the headers work just fine in
> C++, spewing a warning at us for using them would not serve anything.

Well, if you volunteer to maintain C++ support then sure. Most devs in
this project are C guys. Admittedly this is a bit of a Måns-ism, but
he's no longer involved so.. I don't think anyone is principally
opposed, it's just a question of where to spend developer effort.

/Tomas
Carl Eugen Hoyos June 27, 2020, 9:51 a.m. UTC | #5
Am Sa., 27. Juni 2020 um 11:47 Uhr schrieb Tomas Härdin <tjoppen@acc.umu.se>:
>
> fre 2020-06-26 klockan 11:36 +0200 skrev Hendrik Leppkes:
> > On Fri, Jun 26, 2020 at 11:31 AM Tomas Härdin <tjoppen@acc.umu.se> wrote:
> > > ons 2020-06-24 klockan 21:22 +0200 skrev Carl Eugen Hoyos:
> > > > Am Mi., 24. Juni 2020 um 21:10 Uhr schrieb rcombs <rcombs@rcombs.me>:
> > > > > ---
> > > > >  libavutil/avutil.h | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> > > > > index 4d633156d1..c11b33f466 100644
> > > > > --- a/libavutil/avutil.h
> > > > > +++ b/libavutil/avutil.h
> > > > > @@ -257,7 +257,11 @@ const char *av_get_media_type_string(enum AVMediaType media_type);
> > > > >   * Internal time base represented as fractional value
> > > > >   */
> > > > >
> > > > > +#ifdef __cplusplus
> > > > > +#define AV_TIME_BASE_Q          AVRational{1, AV_TIME_BASE}
> > > > > +#else
> > > > >  #define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
> > > > > +#endif
> > > >
> > > > The problem with this patch is that it gives C++ users of the libraries
> > > > the feeling that we would care about them and that we would indeed
> > > > fix issues.
> > > > Given that the contrary is true (at least in the past) and that we do
> > > > not do any specific C++ testing, I believe it is better to let users
> > > > work around this issue (from their point of you).
> > >
> > > Maybe we should put in a #warning to that effect? Inside an #ifdef
> > > __cplusplus
> > >
> >
> > Now that would just be annoying. 99% of the headers work just fine in
> > C++, spewing a warning at us for using them would not serve anything.
>
> Well, if you volunteer to maintain C++ support then sure. Most devs in
> this project are C guys. Admittedly this is a bit of a Måns-ism, but
> he's no longer involved so.. I don't think anyone is principally
> opposed, it's just a question of where to spend developer effort.

Not sure why you believe we need a warning:
Iiuc, currently AV_TIME_BASE_Q cannot be used from C++ programs,
any C++ developer will immediately be made aware of this, no?

Carl Eugen
Tomas Härdin June 30, 2020, 8:24 a.m. UTC | #6
lör 2020-06-27 klockan 11:51 +0200 skrev Carl Eugen Hoyos:
> Am Sa., 27. Juni 2020 um 11:47 Uhr schrieb Tomas Härdin <tjoppen@acc.umu.se>:
> > fre 2020-06-26 klockan 11:36 +0200 skrev Hendrik Leppkes:
> > > On Fri, Jun 26, 2020 at 11:31 AM Tomas Härdin <tjoppen@acc.umu.se> wrote:
> > > > ons 2020-06-24 klockan 21:22 +0200 skrev Carl Eugen Hoyos:
> > > > > Am Mi., 24. Juni 2020 um 21:10 Uhr schrieb rcombs <rcombs@rcombs.me>:
> > > > > > ---
> > > > > >  libavutil/avutil.h | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> > > > > > index 4d633156d1..c11b33f466 100644
> > > > > > --- a/libavutil/avutil.h
> > > > > > +++ b/libavutil/avutil.h
> > > > > > @@ -257,7 +257,11 @@ const char *av_get_media_type_string(enum AVMediaType media_type);
> > > > > >   * Internal time base represented as fractional value
> > > > > >   */
> > > > > > 
> > > > > > +#ifdef __cplusplus
> > > > > > +#define AV_TIME_BASE_Q          AVRational{1, AV_TIME_BASE}
> > > > > > +#else
> > > > > >  #define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
> > > > > > +#endif
> > > > > 
> > > > > The problem with this patch is that it gives C++ users of the libraries
> > > > > the feeling that we would care about them and that we would indeed
> > > > > fix issues.
> > > > > Given that the contrary is true (at least in the past) and that we do
> > > > > not do any specific C++ testing, I believe it is better to let users
> > > > > work around this issue (from their point of you).
> > > > 
> > > > Maybe we should put in a #warning to that effect? Inside an #ifdef
> > > > __cplusplus
> > > > 
> > > 
> > > Now that would just be annoying. 99% of the headers work just fine in
> > > C++, spewing a warning at us for using them would not serve anything.
> > 
> > Well, if you volunteer to maintain C++ support then sure. Most devs in
> > this project are C guys. Admittedly this is a bit of a Måns-ism, but
> > he's no longer involved so.. I don't think anyone is principally
> > opposed, it's just a question of where to spend developer effort.
> 
> Not sure why you believe we need a warning:
> Iiuc, currently AV_TIME_BASE_Q cannot be used from C++ programs,
> any C++ developer will immediately be made aware of this, no?

True, but it's generating threads on the list like this one on a
regular basis.

/Tomas
Paul B Mahol June 30, 2020, 10 a.m. UTC | #7
On 6/30/20, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> lör 2020-06-27 klockan 11:51 +0200 skrev Carl Eugen Hoyos:
>> Am Sa., 27. Juni 2020 um 11:47 Uhr schrieb Tomas Härdin
>> <tjoppen@acc.umu.se>:
>> > fre 2020-06-26 klockan 11:36 +0200 skrev Hendrik Leppkes:
>> > > On Fri, Jun 26, 2020 at 11:31 AM Tomas Härdin <tjoppen@acc.umu.se>
>> > > wrote:
>> > > > ons 2020-06-24 klockan 21:22 +0200 skrev Carl Eugen Hoyos:
>> > > > > Am Mi., 24. Juni 2020 um 21:10 Uhr schrieb rcombs
>> > > > > <rcombs@rcombs.me>:
>> > > > > > ---
>> > > > > >  libavutil/avutil.h | 4 ++++
>> > > > > >  1 file changed, 4 insertions(+)
>> > > > > >
>> > > > > > diff --git a/libavutil/avutil.h b/libavutil/avutil.h
>> > > > > > index 4d633156d1..c11b33f466 100644
>> > > > > > --- a/libavutil/avutil.h
>> > > > > > +++ b/libavutil/avutil.h
>> > > > > > @@ -257,7 +257,11 @@ const char *av_get_media_type_string(enum
>> > > > > > AVMediaType media_type);
>> > > > > >   * Internal time base represented as fractional value
>> > > > > >   */
>> > > > > >
>> > > > > > +#ifdef __cplusplus
>> > > > > > +#define AV_TIME_BASE_Q          AVRational{1, AV_TIME_BASE}
>> > > > > > +#else
>> > > > > >  #define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
>> > > > > > +#endif
>> > > > >
>> > > > > The problem with this patch is that it gives C++ users of the
>> > > > > libraries
>> > > > > the feeling that we would care about them and that we would indeed
>> > > > > fix issues.
>> > > > > Given that the contrary is true (at least in the past) and that we
>> > > > > do
>> > > > > not do any specific C++ testing, I believe it is better to let
>> > > > > users
>> > > > > work around this issue (from their point of you).
>> > > >
>> > > > Maybe we should put in a #warning to that effect? Inside an #ifdef
>> > > > __cplusplus
>> > > >
>> > >
>> > > Now that would just be annoying. 99% of the headers work just fine in
>> > > C++, spewing a warning at us for using them would not serve anything.
>> >
>> > Well, if you volunteer to maintain C++ support then sure. Most devs in
>> > this project are C guys. Admittedly this is a bit of a Måns-ism, but
>> > he's no longer involved so.. I don't think anyone is principally
>> > opposed, it's just a question of where to spend developer effort.
>>
>> Not sure why you believe we need a warning:
>> Iiuc, currently AV_TIME_BASE_Q cannot be used from C++ programs,
>> any C++ developer will immediately be made aware of this, no?
>
> True, but it's generating threads on the list like this one on a
> regular basis.
>

So we should suffer instead by applying this patch?

> /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".
Steinar H. Gunderson June 30, 2020, 10:22 a.m. UTC | #8
On Tue, Jun 30, 2020 at 12:00:24PM +0200, Paul B Mahol wrote:
> So we should suffer instead by applying this patch?

The thousands of projects that include FFmpeg from C++, directly or
indirectly, suggests that the amount of suffering on FFmpeg's part
by supporting inclusion from C++ is not going to be large.
If you add some #ifndef __cplusplus extern "C" ... on each header and then
apply the given patch, I would be surprised if this creates a large
maintenance burden on your behalf.

/* Steinar */
diff mbox series

Patch

diff --git a/libavutil/avutil.h b/libavutil/avutil.h
index 4d633156d1..c11b33f466 100644
--- a/libavutil/avutil.h
+++ b/libavutil/avutil.h
@@ -257,7 +257,11 @@  const char *av_get_media_type_string(enum AVMediaType media_type);
  * Internal time base represented as fractional value
  */
 
+#ifdef __cplusplus
+#define AV_TIME_BASE_Q          AVRational{1, AV_TIME_BASE}
+#else
 #define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
+#endif
 
 /**
  * @}