[RFC] lavu PTS printing utils
Hi, I'm thinking to implement some convenience utils: static inline size_t av_get_ts_string(char *buf, size_t buf_size, int64_t ts) { return ts == (AV_NOPTS_VALUE) ? snprintf(buf, buf_size, "NOPTS") : snprintf(buf, buf_size, "%"PRId64"", ts); } static inline size_t av_get_ts_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) { return ts == (AV_NOPTS_VALUE) ? snprintf(buf, buf_size, "NOPTS") : snprintf(buf, buf_size, "%f", avq2d(tb) * ts; } since it seems I need to use them again and again (debugging timestamp issues in various components). Do you think this would be a good idea? If yes, where should I put them (libavutil/printutils.h may be an option)? -- FFmpeg = Forgiving Free Mastering Philosofic Everlasting Guide
On Sun, Jan 15, 2012 at 04:07:27PM +0100, Stefano Sabatini wrote:
Hi,
I'm thinking to implement some convenience utils:
static inline size_t av_get_ts_string(char *buf, size_t buf_size, int64_t ts) { return ts == (AV_NOPTS_VALUE) ? snprintf(buf, buf_size, "NOPTS") : snprintf(buf, buf_size, "%"PRId64"", ts);
At least leave out the pointless () and "".
since it seems I need to use them again and again (debugging timestamp issues in various components).
Quite a few places want them aligned though, and those won't work there.
Do you think this would be a good idea? If yes, where should I put them (libavutil/printutils.h may be an option)?
I'd say most likely only if it is flexible enough to replace (almost) all of the code doing this and it still is an improvement.
On Sun, Jan 15, 2012 at 04:17:53PM +0100, Reimar Döffinger wrote:
On Sun, Jan 15, 2012 at 04:07:27PM +0100, Stefano Sabatini wrote:
Hi,
I'm thinking to implement some convenience utils:
static inline size_t av_get_ts_string(char *buf, size_t buf_size, int64_t ts) { return ts == (AV_NOPTS_VALUE) ? snprintf(buf, buf_size, "NOPTS") : snprintf(buf, buf_size, "%"PRId64"", ts);
At least leave out the pointless () and "".
since it seems I need to use them again and again (debugging timestamp issues in various components).
Quite a few places want them aligned though, and those won't work there.
it should be possible to handle this with a macro instead of a funct [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
Le sextidi 26 nivôse, an CCXX, Stefano Sabatini a écrit :
return ts == (AV_NOPTS_VALUE) ? snprintf(buf, buf_size, "NOPTS") : snprintf(buf, buf_size, "%"PRId64"", ts);
Note that the return value is not the room taken in the buffer but the size that the text would have used if there was enough room.
Do you think this would be a good idea? If yes, where should I put them (libavutil/printutils.h may be an option)?
As Reimer points out, I fear it would lack flexibility for some purposes. But for quick debugging it may be useful. As a side note, I would like to share the following trick that I have used in some programs of mine: char *foo2str_buf(char *buf, size_t buf_size, foo_t foo) { <convert> return buf; } #define foo2str(foo) foo2str_buf((char[42]){ 0 }, 42, foo) with 42 a constant chosen to be large enough: that way, I can write: printf("a = %s\n", foo2str(a)); and the buffer to convert is allocated with automatic storage. Regards, -- Nicolas George
On date Sunday 2012-01-15 16:36:35 +0100, Nicolas George encoded:
Le sextidi 26 nivôse, an CCXX, Stefano Sabatini a écrit :
return ts == (AV_NOPTS_VALUE) ? snprintf(buf, buf_size, "NOPTS") : snprintf(buf, buf_size, "%"PRId64"", ts);
Note that the return value is not the room taken in the buffer but the size that the text would have used if there was enough room.
Do you think this would be a good idea? If yes, where should I put them (libavutil/printutils.h may be an option)?
As Reimer points out, I fear it would lack flexibility for some purposes. But for quick debugging it may be useful.
As a side note, I would like to share the following trick that I have used in some programs of mine:
char *foo2str_buf(char *buf, size_t buf_size, foo_t foo) { <convert> return buf; }
#define foo2str(foo) foo2str_buf((char[42]){ 0 }, 42, foo)
with 42 a constant chosen to be large enough: that way, I can write:
printf("a = %s\n", foo2str(a));
and the buffer to convert is allocated with automatic storage.
Updated based on this, with example code. -- FFmpeg = Frenzy Freak MultiPurpose Extravagant Guide
Hi, Stefano Sabatini wrote:
+ tsutils.h \
I don't know why we don't use _ in file names anymore, but at least it is in line with a few other headers in lavu. Anyway I think concatenation of abbreviations can be quite unreadable and the effect will be strengthened as more and more files with that naming style are added. Please excuse the rant... I wanted to say this many times before. [...]
+ +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%"PRId64"", ts); + return buf; +} + +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts) + +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts); + return buf; +} + +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
The naming of the macros is a bit risky. Especially considering how often we append numbers to names to introduce a successor. av_time2str2 would be rather confusing. Nit: The verb "get" in the function names is IMHO a bit confusing, as you pass a buffer in and don't really get any resource. Maybe "make" would be better fit. What do you think? I still would like to hear a few more voices? Who wants to have these in lavu? I am still not sure myself, though it seems small and useful enough. Alexander
On Tue, Jan 17, 2012 at 10:44:21PM +0100, Alexander Strasser wrote:
[...]
+ +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%"PRId64"", ts); + return buf; +} + +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts) + +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts); + return buf; +} + +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
The naming of the macros is a bit risky. Especially considering how often we append numbers to names to introduce a successor. av_time2str2 would be rather confusing.
Also while I don't care too much about C++, the (char[42]){ 0 } syntax has not made it into even the latest C++ version, so I'd strongly suggest to think at least thrice before adding such a thing to the public API (even though a macro means it won't cause compile errors unless used - but if we want to take advantage of that it should probably be documented). Actually, I think it might be a right-out bad idea to use this at all in public API, because I don't think anyone would accept that char *a; a = av_time2str(b, c); printf(a); would suddenly become invalid code when changed to char *a; { a = av_time2str(b, c); } printf(a);
On date Tuesday 2012-01-17 22:56:35 +0100, Reimar Döffinger encoded:
On Tue, Jan 17, 2012 at 10:44:21PM +0100, Alexander Strasser wrote:
[...]
+ +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%"PRId64"", ts); + return buf; +} + +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts) + +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts); + return buf; +} + +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
The naming of the macros is a bit risky. Especially considering how often we append numbers to names to introduce a successor. av_time2str2 would be rather confusing.
Also while I don't care too much about C++, the (char[42]){ 0 } syntax has not made it into even the latest C++ version, so I'd strongly suggest to think at least thrice before adding such a thing to the public API (even though a macro means it won't cause compile errors unless used - but if we want to take advantage of that it should probably be documented). Actually, I think it might be a right-out bad idea to use this at all in public API, because I don't think anyone would accept that
Agreed, also I don't think that: printf("pts1_time:%s pts2_time:%s\n", av_time2str(pts1, tb1), av_time2str(pts2, tb2)) has defined behavior in this case (evaluation order of arguments is undefined in C, and the construct may return an address in the stack which to a buffer which may be overwritten by the other function call). Actually there is also another problem, the size of a double number representation doesn't have a well defined size, check for example: http://stackoverflow.com/questions/1701055/what-is-the-maximum-length-in-cha... we may format the string to a limited size and add a define #define AV_TS_MAX_SIZE ... or the time representation may result truncated. In this case the usage would be: char buf1[AV_TS_MAX_SIZE], buf2[AV_TS_MAX_SIZE]; printf("pts1_time:%s pts2_time:%s\n", av_time2str(buf1, pts1, tb1), av_time2str(buf2, pts2, tb2)); which is not as convenient but should be safe. -- FFmpeg = Formidable & Faithless Murdering Philosofic Elfic Ghost
On Fri, Jan 20, 2012 at 04:13:52PM +0100, Stefano Sabatini wrote:
On date Tuesday 2012-01-17 22:56:35 +0100, Reimar Döffinger encoded:
On Tue, Jan 17, 2012 at 10:44:21PM +0100, Alexander Strasser wrote:
[...]
+ +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%"PRId64"", ts); + return buf; +} + +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts) + +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts); + return buf; +} + +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
The naming of the macros is a bit risky. Especially considering how often we append numbers to names to introduce a successor. av_time2str2 would be rather confusing.
Also while I don't care too much about C++, the (char[42]){ 0 } syntax has not made it into even the latest C++ version, so I'd strongly suggest to think at least thrice before adding such a thing to the public API (even though a macro means it won't cause compile errors unless used - but if we want to take advantage of that it should probably be documented). Actually, I think it might be a right-out bad idea to use this at all in public API, because I don't think anyone would accept that
Agreed, also I don't think that: printf("pts1_time:%s pts2_time:%s\n", av_time2str(pts1, tb1), av_time2str(pts2, tb2))
has defined behavior in this case (evaluation order of arguments is undefined in C, and the construct may return an address in the stack which to a buffer which may be overwritten by the other function call).
Actually there is also another problem, the size of a double number representation doesn't have a well defined size, check for example: http://stackoverflow.com/questions/1701055/what-is-the-maximum-length-in-cha...
its not that bad because 32bit/32bit * 64bit is more restricted than an arbitrary double [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange
On Fri, Jan 20, 2012 at 04:13:52PM +0100, Stefano Sabatini wrote:
On date Tuesday 2012-01-17 22:56:35 +0100, Reimar Döffinger encoded:
On Tue, Jan 17, 2012 at 10:44:21PM +0100, Alexander Strasser wrote:
[...]
+ +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%"PRId64"", ts); + return buf; +} + +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts) + +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts); + return buf; +} + +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
The naming of the macros is a bit risky. Especially considering how often we append numbers to names to introduce a successor. av_time2str2 would be rather confusing.
Also while I don't care too much about C++, the (char[42]){ 0 } syntax has not made it into even the latest C++ version, so I'd strongly suggest to think at least thrice before adding such a thing to the public API (even though a macro means it won't cause compile errors unless used - but if we want to take advantage of that it should probably be documented). Actually, I think it might be a right-out bad idea to use this at all in public API, because I don't think anyone would accept that
Agreed, also I don't think that: printf("pts1_time:%s pts2_time:%s\n", av_time2str(pts1, tb1), av_time2str(pts2, tb2))
has defined behavior in this case (evaluation order of arguments is undefined in C, and the construct may return an address in the stack which to a buffer which may be overwritten by the other function call).
No, I don't think that is a problem. They are different variables which both have the same scope (the current block) so the compiler can't place them any more at the same place that if you used char a[42],b[42];
char buf1[AV_TS_MAX_SIZE], buf2[AV_TS_MAX_SIZE];
Not much of an opinion otherwise, but _TS_ is not that great a name due to the confusion potential with (MPEG-)TS.
On date Friday 2012-01-20 20:21:08 +0100, Reimar Döffinger encoded:
On Fri, Jan 20, 2012 at 04:13:52PM +0100, Stefano Sabatini wrote:
On date Tuesday 2012-01-17 22:56:35 +0100, Reimar Döffinger encoded:
On Tue, Jan 17, 2012 at 10:44:21PM +0100, Alexander Strasser wrote:
[...]
+ +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%"PRId64"", ts); + return buf; +} + +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts) + +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts); + return buf; +} + +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
The naming of the macros is a bit risky. Especially considering how often we append numbers to names to introduce a successor. av_time2str2 would be rather confusing.
Also while I don't care too much about C++, the (char[42]){ 0 } syntax has not made it into even the latest C++ version, so I'd strongly suggest to think at least thrice before adding such a thing to the public API (even though a macro means it won't cause compile errors unless used - but if we want to take advantage of that it should probably be documented). Actually, I think it might be a right-out bad idea to use this at all in public API, because I don't think anyone would accept that
Agreed, also I don't think that: printf("pts1_time:%s pts2_time:%s\n", av_time2str(pts1, tb1), av_time2str(pts2, tb2))
has defined behavior in this case (evaluation order of arguments is undefined in C, and the construct may return an address in the stack which to a buffer which may be overwritten by the other function call).
No, I don't think that is a problem. They are different variables which both have the same scope (the current block) so the compiler can't place them any more at the same place that if you used char a[42],b[42];
char buf1[AV_TS_MAX_SIZE], buf2[AV_TS_MAX_SIZE];
Not much of an opinion otherwise, but _TS_ is not that great a name due to the confusion potential with (MPEG-)TS.
Updated patch with sample ffmpeg patch, comments: * av_ts2str(), av_ts2time2str() are retained for their usefulness, although they should never be used in C++, we assume that C++ programmers using FFmpeg code have a clue regarding C/C++ incompatibilites, and they can still use the long av_ts_make* functions * double precision is reduced to 6 digits, which looks enough for A/V applications, could not be enough for scientific computing but again given the application domain I don't think this is a much serious issue * "ts" is favored over "timestamp" for brevity/convenience reasons * "ts2str" naming is kept in the macro for brevity/convenience reasons Please comment. -- FFmpeg = Fast and Funny Muttering Pitiless Embarassing Glue
On Fri, Jan 27, 2012 at 01:35:06PM +0100, Stefano Sabatini wrote:
On date Friday 2012-01-20 20:21:08 +0100, Reimar Döffinger encoded:
On Fri, Jan 20, 2012 at 04:13:52PM +0100, Stefano Sabatini wrote:
On date Tuesday 2012-01-17 22:56:35 +0100, Reimar Döffinger encoded:
On Tue, Jan 17, 2012 at 10:44:21PM +0100, Alexander Strasser wrote:
[...]
+ +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%"PRId64"", ts); + return buf; +} + +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts) + +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts); + return buf; +} + +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
The naming of the macros is a bit risky. Especially considering how often we append numbers to names to introduce a successor. av_time2str2 would be rather confusing.
Also while I don't care too much about C++, the (char[42]){ 0 } syntax has not made it into even the latest C++ version, so I'd strongly suggest to think at least thrice before adding such a thing to the public API (even though a macro means it won't cause compile errors unless used - but if we want to take advantage of that it should probably be documented). Actually, I think it might be a right-out bad idea to use this at all in public API, because I don't think anyone would accept that
Agreed, also I don't think that: printf("pts1_time:%s pts2_time:%s\n", av_time2str(pts1, tb1), av_time2str(pts2, tb2))
has defined behavior in this case (evaluation order of arguments is undefined in C, and the construct may return an address in the stack which to a buffer which may be overwritten by the other function call).
No, I don't think that is a problem. They are different variables which both have the same scope (the current block) so the compiler can't place them any more at the same place that if you used char a[42],b[42];
char buf1[AV_TS_MAX_SIZE], buf2[AV_TS_MAX_SIZE];
Not much of an opinion otherwise, but _TS_ is not that great a name due to the confusion potential with (MPEG-)TS.
Updated patch with sample ffmpeg patch, comments:
* av_ts2str(), av_ts2time2str() are retained for their usefulness, although they should never be used in C++, we assume that C++ programmers using FFmpeg code have a clue regarding C/C++ incompatibilites, and they can still use the long av_ts_make* functions
C++ developers are used to work with a limited subset of the C functionalities, let's not be concerned to much about this :)
* double precision is reduced to 6 digits, which looks enough for A/V applications, could not be enough for scientific computing but again given the application domain I don't think this is a much serious issue
Isn't the way of printing a floating value up to a certain precision depending on the printf implementation anyway?
* "ts" is favored over "timestamp" for brevity/convenience reasons
* "ts2str" naming is kept in the macro for brevity/convenience reasons
I'm OK with this. [...]
From f7ba7daa66ed3cb79ab425d0494889be8ad685f7 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefasab@gmail.com> Date: Fri, 20 Jan 2012 17:06:26 +0100 Subject: [PATCH] ffmpeg.c: use timestamp.h utilities
--- ffmpeg.c | 33 ++++++++++++++++++++++++++------- 1 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/ffmpeg.c b/ffmpeg.c index 88a4761..1243df6 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -23,6 +23,8 @@ * multimedia converter based on the FFmpeg libraries */
+#define DEBUG_TS +
I'm not really fond of this; why not use the -debug pts (see FF_DEBUG_PTS) or a new -debug ts instead? [...] -- Clément B.
On date Friday 2012-01-27 19:09:38 +0100, Clément Bœsch encoded:
On Fri, Jan 27, 2012 at 01:35:06PM +0100, Stefano Sabatini wrote: [...]
* double precision is reduced to 6 digits, which looks enough for A/V applications, could not be enough for scientific computing but again given the application domain I don't think this is a much serious issue
Isn't the way of printing a floating value up to a certain precision depending on the printf implementation anyway?
Yes, but the %g representation should guarantee that the maximum size is limited to a well defined size.
* "ts" is favored over "timestamp" for brevity/convenience reasons
* "ts2str" naming is kept in the macro for brevity/convenience reasons
I'm OK with this.
[...]
From f7ba7daa66ed3cb79ab425d0494889be8ad685f7 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefasab@gmail.com> Date: Fri, 20 Jan 2012 17:06:26 +0100 Subject: [PATCH] ffmpeg.c: use timestamp.h utilities
--- ffmpeg.c | 33 ++++++++++++++++++++++++++------- 1 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/ffmpeg.c b/ffmpeg.c index 88a4761..1243df6 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -23,6 +23,8 @@ * multimedia converter based on the FFmpeg libraries */
+#define DEBUG_TS +
I'm not really fond of this; why not use the -debug pts (see FF_DEBUG_PTS) or a new -debug ts instead?
This is just a quick hack for showing how to use how to use the new utils, we can add an ffmpeg -debug ts option for that. -- FFmpeg = Fierce and Faithless Monstrous Puristic Elastic Gigant
On date Saturday 2012-01-28 12:14:08 +0100, Stefano Sabatini encoded:
On date Friday 2012-01-27 19:09:38 +0100, Clément Bœsch encoded:
On Fri, Jan 27, 2012 at 01:35:06PM +0100, Stefano Sabatini wrote: [...]
* double precision is reduced to 6 digits, which looks enough for A/V applications, could not be enough for scientific computing but again given the application domain I don't think this is a much serious issue
Isn't the way of printing a floating value up to a certain precision depending on the printf implementation anyway?
Yes, but the %g representation should guarantee that the maximum size is limited to a well defined size.
* "ts" is favored over "timestamp" for brevity/convenience reasons
* "ts2str" naming is kept in the macro for brevity/convenience reasons
I'm OK with this.
[...]
From f7ba7daa66ed3cb79ab425d0494889be8ad685f7 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefasab@gmail.com> Date: Fri, 20 Jan 2012 17:06:26 +0100 Subject: [PATCH] ffmpeg.c: use timestamp.h utilities
--- ffmpeg.c | 33 ++++++++++++++++++++++++++------- 1 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/ffmpeg.c b/ffmpeg.c index 88a4761..1243df6 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -23,6 +23,8 @@ * multimedia converter based on the FFmpeg libraries */
+#define DEBUG_TS +
I'm not really fond of this; why not use the -debug pts (see FF_DEBUG_PTS) or a new -debug ts instead?
This is just a quick hack for showing how to use how to use the new utils, we can add an ffmpeg -debug ts option for that. -- FFmpeg = Fierce and Faithless Monstrous Puristic Elastic Gigant
From af61c764221783a4bd603385d5f29f589cb898b5 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefasab@gmail.com> Date: Tue, 17 Jan 2012 17:12:42 +0100 Subject: [PATCH] lavu: add timestamp.h header with convenience timestamp utilities
I'd like to push this in a few days, ping (Michael)? -- FFmpeg = Fancy & Freak Muttering Portable Exuberant Gorilla
On date Tuesday 2012-01-31 15:43:21 +0100, Stefano Sabatini encoded:
On date Saturday 2012-01-28 12:14:08 +0100, Stefano Sabatini encoded: [...]
From af61c764221783a4bd603385d5f29f589cb898b5 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefasab@gmail.com> Date: Tue, 17 Jan 2012 17:12:42 +0100 Subject: [PATCH] lavu: add timestamp.h header with convenience timestamp utilities
I'd like to push this in a few days, ping (Michael)?
Patch updated. I'll push the patch tomorrow if I read no more comments. -- FFmpeg = Fundamentalist and Fundamental Magnificient Prodigious Extended Genius
On date Monday 2012-02-06 13:22:54 +0100, Stefano Sabatini encoded:
On date Tuesday 2012-01-31 15:43:21 +0100, Stefano Sabatini encoded:
On date Saturday 2012-01-28 12:14:08 +0100, Stefano Sabatini encoded: [...]
From af61c764221783a4bd603385d5f29f589cb898b5 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefasab@gmail.com> Date: Tue, 17 Jan 2012 17:12:42 +0100 Subject: [PATCH] lavu: add timestamp.h header with convenience timestamp utilities
I'd like to push this in a few days, ping (Michael)?
Patch updated. I'll push the patch tomorrow if I read no more comments. -- FFmpeg = Fundamentalist and Fundamental Magnificient Prodigious Extended Genius
From e4d28ecb1eed1479227c4dce69e14edd28bdf9bc Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefasab@gmail.com> Date: Tue, 17 Jan 2012 17:12:42 +0100 Subject: [PATCH] lavu: add timestamp.h header with convenience timestamp utilities
--- libavutil/Makefile | 1 + libavutil/timestamp.h | 75 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 0 deletions(-) create mode 100644 libavutil/timestamp.h
Applied. -- FFmpeg = Faithless and Frightening Merciless Pacific Ephemeral Gospel
Hi Stefano, Stefano Sabatini wrote: [...]
Updated patch with sample ffmpeg patch, comments:
* av_ts2str(), av_ts2time2str() are retained for their usefulness, although they should never be used in C++, we assume that C++ programmers using FFmpeg code have a clue regarding C/C++ incompatibilites, and they can still use the long av_ts_make* functions
As they are macros and are for convenience only (functionality is still available for C++ programmers), I am OK with that.
* double precision is reduced to 6 digits, which looks enough for A/V applications, could not be enough for scientific computing but again given the application domain I don't think this is a much serious issue
* "ts" is favored over "timestamp" for brevity/convenience reasons
* "ts2str" naming is kept in the macro for brevity/convenience reasons
Fine if you prefer it this way. [...]
+static inline char *av_ts_make_string(char *buf, int64_t ts) [...] +static inline char *av_ts_make_time_string(char *buf, int64_t ts, AVRational *tb)
NIT: Here and before I would prefer ts as the first argument, it is also consistent with the naming which is av_ts_*. Like it is saying these functions are acting on a timestamp (I know they can't and don't need to modify it as they only get a copy). IMHO following that convention makes the order more easy to remember. I would be glad to hear your opinion. [...] Alexander
On date Friday 2012-01-27 22:14:35 +0100, Alexander Strasser encoded:
Hi Stefano,
Stefano Sabatini wrote: [...]
Updated patch with sample ffmpeg patch, comments:
* av_ts2str(), av_ts2time2str() are retained for their usefulness, although they should never be used in C++, we assume that C++ programmers using FFmpeg code have a clue regarding C/C++ incompatibilites, and they can still use the long av_ts_make* functions
As they are macros and are for convenience only (functionality is still available for C++ programmers), I am OK with that.
* double precision is reduced to 6 digits, which looks enough for A/V applications, could not be enough for scientific computing but again given the application domain I don't think this is a much serious issue
* "ts" is favored over "timestamp" for brevity/convenience reasons
* "ts2str" naming is kept in the macro for brevity/convenience reasons
Fine if you prefer it this way.
[...]
+static inline char *av_ts_make_string(char *buf, int64_t ts) [...] +static inline char *av_ts_make_time_string(char *buf, int64_t ts, AVRational *tb)
NIT: Here and before I would prefer ts as the first argument, it is also consistent with the naming which is av_ts_*. Like it is saying these functions are acting on a timestamp (I know they can't and don't need to modify it as they only get a copy).
IMHO following that convention makes the order more easy to remember. I would be glad to hear your opinion.
Procedural C style convenctions usually demand this order of arguments: make_thing(dst, src); so in this case that would be: av_ts_make_string(char *buf, int64_t ts) which is interpreted as: buf <= ts i.e. like an assignment. In case of: static inline char *av_ts_make_time_string(char *buf, int64_t ts, AVRational *tb); here the timestamp can't be represented by a single entity (indeed it needs a tb in order to be interpreted), so I think that preferring: static inline char *av_ts_make_time_string(int64_t ts, AVRational *tb, char *buf) would be mostly confusing. Note that here "_ts_" is used like a namespace rather than for indicating the "object" of the "operator" denoted by the function, in case you're adopting an object-oriented programming style, for which: int av_bike_set_shed_color(AVBike *bike, char *color_str); would have made much sense. -- FFmpeg = Frenzy and Forgiving Multimedia Powered Erroneous Gospel
Stefano Sabatini wrote:
On date Friday 2012-01-27 22:14:35 +0100, Alexander Strasser encoded:
Stefano Sabatini wrote: [...]
Updated patch with sample ffmpeg patch, comments:
* av_ts2str(), av_ts2time2str() are retained for their usefulness, although they should never be used in C++, we assume that C++ programmers using FFmpeg code have a clue regarding C/C++ incompatibilites, and they can still use the long av_ts_make* functions
As they are macros and are for convenience only (functionality is still available for C++ programmers), I am OK with that.
* double precision is reduced to 6 digits, which looks enough for A/V applications, could not be enough for scientific computing but again given the application domain I don't think this is a much serious issue
* "ts" is favored over "timestamp" for brevity/convenience reasons
* "ts2str" naming is kept in the macro for brevity/convenience reasons
Fine if you prefer it this way.
[...]
+static inline char *av_ts_make_string(char *buf, int64_t ts) [...] +static inline char *av_ts_make_time_string(char *buf, int64_t ts, AVRational *tb)
NIT: Here and before I would prefer ts as the first argument, it is also consistent with the naming which is av_ts_*. Like it is saying these functions are acting on a timestamp (I know they can't and don't need to modify it as they only get a copy).
IMHO following that convention makes the order more easy to remember. I would be glad to hear your opinion.
Procedural C style convenctions usually demand this order of arguments:
make_thing(dst, src);
This differs often. Also there is no clear solution then to settle on one convention and use it consistently. But for me this is an instance of a different problem, which applies if you have two equal entities like registers or byte arrays or other arrays or structures of the same type.
so in this case that would be: av_ts_make_string(char *buf, int64_t ts)
which is interpreted as: buf <= ts
i.e. like an assignment.
In case of: static inline char *av_ts_make_time_string(char *buf, int64_t ts, AVRational *tb);
here the timestamp can't be represented by a single entity (indeed it needs a tb in order to be interpreted), so I think that preferring:
You could also say you need a tb to produce an absolute time representation from a ts.
static inline char *av_ts_make_time_string(int64_t ts, AVRational *tb, char *buf)
would be mostly confusing.
I would have preferred that order.
Note that here "_ts_" is used like a namespace rather than for indicating the "object" of the "operator" denoted by the function, in case you're adopting an object-oriented programming style, for which:
int av_bike_set_shed_color(AVBike *bike, char *color_str);
would have made much sense.
The borderline is always kind of fuzzy. Depends on your viewing angle. I think I could explain myself a bit better but it is getting off topic. Anyway I won't insist on anything in this corner case. So it is up to you to decide. Alexander
On 27 Jan 2012, at 13:35, Stefano Sabatini <stefasab@gmail.com> wrote:
* av_ts2str(), av_ts2time2str() are retained for their usefulness, although they should never be used in C++, we assume that C++ programmers using FFmpeg code have a clue regarding C/C++ incompatibilites, and they can still use the long av_ts_make* functions
The bigger risk is C programmers not understanding their limitations, leading to random crashes or randomly corrupted data. It might be advisable to recommend to use them only directly in function arguments but never stand-alone, which avoids the issue.
On date Tuesday 2012-01-17 22:44:21 +0100, Alexander Strasser encoded:
Hi,
Stefano Sabatini wrote:
+ tsutils.h \
I don't know why we don't use _ in file names anymore, but at least it is in line with a few other headers in lavu.
Anyway I think concatenation of abbreviations can be quite unreadable and the effect will be strengthened as more and more files with that naming style are added.
Please excuse the rant... I wanted to say this many times before.
Sure, timestamp.h is perfectly fine with me.
[...]
+ +static inline char *av_get_ts_string(char *buf, size_t buf_size, int64_t ts) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%"PRId64"", ts); + return buf; +} + +#define av_ts2str(ts) av_get_ts_string((char[42]){ 0 }, 42, ts) + +static inline char* av_get_time_string(char *buf, size_t buf_size, int64_t ts, AVRational *tb) +{ + if (ts == AV_NOPTS_VALUE) snprintf(buf, buf_size, "NOPTS"); + else snprintf(buf, buf_size, "%f", av_q2d(*tb) * ts); + return buf; +} + +#define av_time2str(ts, tb) av_get_time_string((char[42]){ 0 }, 42, ts, tb)
The naming of the macros is a bit risky. Especially considering how often we append numbers to names to introduce a successor. av_time2str2 would be rather confusing.
Uhm, the point is keeping the name as short as possible, that's why time2str is better than time_to_str(), but the macro has also other problems so I'll likely drop it.
Nit: The verb "get" in the function names is IMHO a bit confusing, as you pass a buffer in and don't really get any resource. Maybe "make" would be better fit. What do you think?
Works for me.
I still would like to hear a few more voices? Who wants to have these in lavu? I am still not sure myself, though it seems small and useful enough. -- FFmpeg = Fostering & Foolish Mega Plastic Enchanting Goblin
On date Friday 2012-01-20 16:00:18 +0100, Stefano Sabatini encoded:
On date Tuesday 2012-01-17 22:44:21 +0100, Alexander Strasser encoded:
Hi,
Stefano Sabatini wrote:
+ tsutils.h \
I don't know why we don't use _ in file names anymore, but at least it is in line with a few other headers in lavu.
Anyway I think concatenation of abbreviations can be quite unreadable and the effect will be strengthened as more and more files with that naming style are added.
Please excuse the rant... I wanted to say this many times before.
Sure, timestamp.h is perfectly fine with me. [...]
Updated work in progress, with usage example. -- FFmpeg = Friendly and Fundamentalist Magnificient Prodigious Elected Gadget
participants (6)
-
Alexander Strasser -
Clément Bœsch -
Michael Niedermayer -
Nicolas George -
Reimar Döffinger -
Stefano Sabatini