[libav-devel] [PATCH 0/2] Fix avutil.h usage from C++

[Derek Buitenhuis]

Derek Buitenhuis (2):
avprobe: Don't rely on internal API knowledge
avutil: Add av_get_time_base_q() and use it for AV_TIME_BASE_Q

avprobe.c | 18 +++++++++---------
doc/APIchanges | 3 +++
libavutil/avutil.h | 8 +++++++-
libavutil/utils.c | 5 +++++
libavutil/version.h | 2 +-
5 files changed, 25 insertions(+), 11 deletions(-)

[Derek Buitenhuis]

Don't rely on the fact that AV_TIME_BASE_Q is a define
with a compound literal. No such guarantee is made for our
API and the assumption is not a valid one, as it relies on
internal knowledge.

Modifytime_value_string to take a value instead of a reference.
It is not clear why this took a reference in the first place,
as this fact is never actually used in the function.

Signed-off-by: Derek Buitenhuis <***@gmail.com>
---
avprobe.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/avprobe.c b/avprobe.c
index c7b3d39..9ed19c4 100644
--- a/avprobe.c
+++ b/avprobe.c
@@ -491,12 +491,12 @@ static char *value_string(char *buf, int buf_size, double val, const char *unit)
}

static char *time_value_string(char *buf, int buf_size, int64_t val,
- const AVRational *time_base)
+ const AVRational time_base)
{
if (val == AV_NOPTS_VALUE) {
snprintf(buf, buf_size, "N/A");
} else {
- value_string(buf, buf_size, val * av_q2d(*time_base), unit_second_str);
+ value_string(buf, buf_size, val * av_q2d(time_base), unit_second_str);
}

return buf;
@@ -536,15 +536,15 @@ static void show_packet(AVFormatContext *fmt_ctx, AVPacket *pkt)
probe_int("stream_index", pkt->stream_index);
probe_str("pts", ts_value_string(val_str, sizeof(val_str), pkt->pts));
probe_str("pts_time", time_value_string(val_str, sizeof(val_str),
- pkt->pts, &st->time_base));
+ pkt->pts, st->time_base));
probe_str("dts", ts_value_string(val_str, sizeof(val_str), pkt->dts));
probe_str("dts_time", time_value_string(val_str, sizeof(val_str),
- pkt->dts, &st->time_base));
+ pkt->dts, st->time_base));
probe_str("duration", ts_value_string(val_str, sizeof(val_str),
pkt->duration));
probe_str("duration_time", time_value_string(val_str, sizeof(val_str),
pkt->duration,
- &st->time_base));
+ st->time_base));
probe_str("size", value_string(val_str, sizeof(val_str),
pkt->size, unit_byte_str));
probe_int("pos", pkt->pos);
@@ -653,10 +653,10 @@ static void show_stream(AVFormatContext *fmt_ctx, int stream_idx)
&stream->time_base));
probe_str("start_time",
time_value_string(val_str, sizeof(val_str),
- stream->start_time, &stream->time_base));
+ stream->start_time, stream->time_base));
probe_str("duration",
time_value_string(val_str, sizeof(val_str),
- stream->duration, &stream->time_base));
+ stream->duration, stream->time_base));
if (stream->nb_frames)
probe_int("nb_frames", stream->nb_frames);

@@ -677,10 +677,10 @@ static void show_format(AVFormatContext *fmt_ctx)
probe_str("format_long_name", fmt_ctx->iformat->long_name);
probe_str("start_time",
time_value_string(val_str, sizeof(val_str),
- fmt_ctx->start_time, &AV_TIME_BASE_Q));
+ fmt_ctx->start_time, AV_TIME_BASE_Q));
probe_str("duration",
time_value_string(val_str, sizeof(val_str),
- fmt_ctx->duration, &AV_TIME_BASE_Q));
+ fmt_ctx->duration, AV_TIME_BASE_Q));
probe_str("size",
size >= 0 ? value_string(val_str, sizeof(val_str),
size, unit_byte_str)

[Martin Storsjö]

Post by Derek Buitenhuis
Don't rely on the fact that AV_TIME_BASE_Q is a define
with a compound literal. No such guarantee is made for our
API and the assumption is not a valid one, as it relies on
internal knowledge.
Modifytime_value_string to take a value instead of a reference.
It is not clear why this took a reference in the first place,
as this fact is never actually used in the function.
---
avprobe.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/avprobe.c b/avprobe.c
index c7b3d39..9ed19c4 100644
--- a/avprobe.c
+++ b/avprobe.c
@@ -491,12 +491,12 @@ static char *value_string(char *buf, int buf_size, double val, const char *unit)
}
static char *time_value_string(char *buf, int buf_size, int64_t val,
- const AVRational *time_base)
+ const AVRational time_base)

There's not much point in keeping the const here as well (although it
doesn't hurt).

The patch itself is ok with me.

// Martin

[Derek Buitenhuis]

This fixes usage of AV_TIME_BASE_Q in C++ applications, which
cannot use compound literals directly in their code.

Signed-off-by: Derek Buitenhuis <***@gmail.com>
---
doc/APIchanges | 3 +++
libavutil/avutil.h | 8 +++++++-
libavutil/utils.c | 5 +++++
libavutil/version.h | 2 +-
4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index 1e380e9..0a380f3 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -13,6 +13,9 @@ libavutil: 2012-10-22

API changes, most recent first:

+2013-12-xx - xxxxxxx - lavu 52.20.0 - avutil.h
+ Add av_get_time_base_q().
+
2013-11-xx - xxxxxxx- - lavu 52.19.0 - frame.h
Add AV_FRAME_DATA_A53_CC value to the AVFrameSideDataType enum, which
identifies ATSC A53 Part 4 Closed Captions data.
diff --git a/libavutil/avutil.h b/libavutil/avutil.h
index 6bb5510..94b909a 100644
--- a/libavutil/avutil.h
+++ b/libavutil/avutil.h
@@ -268,7 +268,7 @@ enum AVMediaType {
* Internal time base represented as fractional value
*/

-#define AV_TIME_BASE_Q (AVRational){1, AV_TIME_BASE}
+#define AV_TIME_BASE_Q av_get_time_base_q()

/**
* @}
@@ -304,6 +304,7 @@ char av_get_picture_type_char(enum AVPictureType pict_type);
*/

#include "error.h"
+#include "rational.h"
#include "version.h"

/**
@@ -311,4 +312,9 @@ char av_get_picture_type_char(enum AVPictureType pict_type);
* @}
*/

+/**
+ * Returns the fractional representation of the internal time base.
+ */
+AVRational av_get_time_base_q(void);
+
#endif /* AVUTIL_AVUTIL_H */
diff --git a/libavutil/utils.c b/libavutil/utils.c
index 9b18c97..d5428fd 100644
--- a/libavutil/utils.c
+++ b/libavutil/utils.c
@@ -53,3 +53,8 @@ char av_get_picture_type_char(enum AVPictureType pict_type)
default: return '?';
}
}
+
+av_always_inline AVRational av_get_time_base_q(void)
+{
+ return (AVRational){1, AV_TIME_BASE};
+}
diff --git a/libavutil/version.h b/libavutil/version.h
index fa1f53b..dad8e2f 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -37,7 +37,7 @@
*/

#define LIBAVUTIL_VERSION_MAJOR 52
-#define LIBAVUTIL_VERSION_MINOR 19
+#define LIBAVUTIL_VERSION_MINOR 20
#define LIBAVUTIL_VERSION_MICRO 0

#define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \

[Luca Barbato]

Post by Derek Buitenhuis
+av_always_inline AVRational av_get_time_base_q(void)
+{
+ return (AVRational){1, AV_TIME_BASE};
+}

Shouldn't work. This is not Go =)

lu

[Derek Buitenhuis]

Post by Luca Barbato
Shouldn't work. This is not Go =)

Be more specific?

- Derek

[Luca Barbato]

Post by Derek Buitenhuis

Post by Luca Barbato
Shouldn't work. This is not Go =)

Be more specific?

I misread it, seems fine to me again =)

lu

[Derek Buitenhuis]

Post by Derek Buitenhuis
avprobe: Don't rely on internal API knowledge
avutil: Add av_get_time_base_q() and use it for AV_TIME_BASE_Q

Latter would be an API break, so I propose that we deprecate AV_TIME_BASE_Q instead.

- Derek

[Luca Barbato]

Post by Derek Buitenhuis

Post by Derek Buitenhuis
avprobe: Don't rely on internal API knowledge
avutil: Add av_get_time_base_q() and use it for AV_TIME_BASE_Q

Latter would be an API break, so I propose that we deprecate AV_TIME_BASE_Q instead.

I'd just keep it as is and let foreign languages user just represent it
using AV_TIME_BASE or your useful function.

[Derek Buitenhuis]

Post by Luca Barbato
I'd just keep it as is and let foreign languages user just represent it
using AV_TIME_BASE or your useful function.

I disagree. A proper *portable* library should not use such features.

"Don't use it" is not a valid way to work around this, and reeks of
C99-master-master-ism.

Similarly, it is silly to have two things that do the same thing.

What logical basis is there for not deprecating/replacing it other
than "but it's C99 and nice"?

- Derek

[Derek Buitenhuis]

Post by Derek Buitenhuis
I disagree. A proper *portable* library should not use such features.

In case I was unclear here: I mean portable API defines.

- Derek

[Luca Barbato]

Post by Derek Buitenhuis

Post by Derek Buitenhuis
I disagree. A proper *portable* library should not use such features.

In case I was unclear here: I mean portable API defines.

To make it working you should define a static variable and then expose
to the headers as extern AVRational, and probably wouldn't be a bad idea
all in all.

lu

[Derek Buitenhuis]

Post by Luca Barbato
To make it working you should define a static variable and then expose
to the headers as extern AVRational, and probably wouldn't be a bad idea
all in all.

Well considering we just got through removing all the exported data, I don't
think people are keen on adding it back...

- Derek