[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 post might be inappropriate. Click to display it.

[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