diff mbox

[FFmpeg-devel] libavcodec: add timer bitstream filter [v2]

Message ID LLkcXIUui8Sk98nsBMs-u01h58EcsnAkRLkDHuroEt65QJLLH56WwahnlxoJas4oUQXW_WXz8HliZRMwF_P0jznXcRLdwkmAgw6etDV1Ws4=@protonmail.com
State New
Headers show

Commit Message

Andreas Håkon Aug. 2, 2019, 1:03 p.m. UTC
Hi,

This new version completes the documentation.
Supersedes: https://patchwork.ffmpeg.org/patch/13743/

Regards.
A.H.

---
From 9d115c9c049c5a1141ea83174794bf773359ebd0 Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@protonmail.com>
Date: Fri, 2 Aug 2019 13:55:48 +0100
Subject: [PATCH] libavcodec: add timer bitstream filter [v2]

This implements the new timer bitstream filter.

Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com>
---
 doc/bitstream_filters.texi     |   10 +++++
 libavcodec/Makefile            |    1 +
 libavcodec/bitstream_filters.c |    1 +
 libavcodec/timer_bsf.c         |   86 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 98 insertions(+)
 create mode 100644 libavcodec/timer_bsf.c

Comments

Moritz Barsnick Aug. 5, 2019, 3:31 p.m. UTC | #1
Hi Andreas,

On Fri, Aug 02, 2019 at 13:03:03 +0000, Andreas Håkon wrote:

While trying to figure out how to add features I'd like to see into
this ;-), some remarks:

> +Apply an offset to the PTS/DTS timestamps.
> +
> +@table @option
> +@item offset
> +The offset value to apply to the PTS/DTS timestamps.

For the unknowing, it might be useful to point out what sort of
increments this is (i.e. not an actual "time" stamp or seconds).

> +// TODO: Control time wrapping
[...]
> +    int offset;
[...]
> +    { "offset", NULL, OFFSET(offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, FLAGS },

Considering PTS/DTS is int64, wouldn't it be useful for the offset also
being int64 (and using limits INT64_MIN, INT64_MAX)?

> +// TODO: Instead of using absolute timestamp offsets, use frame numbers

Alternatively? Or perhaps also AV_OPT_TYPE_DURATION? TODO is for later,
anyway...

> +    if (!s->first_debug_done) {
> +        av_log(ctx, AV_LOG_DEBUG, "Updated PTS/DTS (%"PRId64"/%"PRId64" : %"PRId64"/%"PRId64") with offset:%d\n",
> +                pkt->pts, pkt->dts, opts, odts, s->offset );
> +    }
> +    s->first_debug_done = 1;

You should set the variable inside the if() block. otherwise it gets
assigned on every packet.

> +static const AVClass timer_class = {
> +    .class_name = "timer",

In about half of the existing BSFs, this would be called "timer_bsf". I
don't care, I just look at other existing code. ;-)

Cheers,
Moritz
Andreas Håkon Aug. 5, 2019, 5:25 p.m. UTC | #2
Hi Moritz,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, 5 de August de 2019 17:31, Moritz Barsnick <barsnick@gmx.net> wrote:

> Hi Andreas,
>
> While trying to figure out how to add features I'd like to see into
> this ;-), some remarks:
>
> > +Apply an offset to the PTS/DTS timestamps.
> > +
> > +@table @option
> > +@item offset
> > +The offset value to apply to the PTS/DTS timestamps.
>
> For the unknowing, it might be useful to point out what sort of
> increments this is (i.e. not an actual "time" stamp or seconds).

The value are native PTS/DTS ticks, so a resolution of 90kHz.
So, you agree with something like this?

@item offset
"The absolute offset value to apply to the PTS/DTS timestamps
with a 90kHz resolution."


> [...]
> > +   { "offset", NULL, OFFSET(offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, FLAGS },
>
> Considering PTS/DTS is int64, wouldn't it be useful for the offset also
> being int64 (and using limits INT64_MIN, INT64_MAX)?

Aah, I feel you like then to full cover all the possible values.
Then two changes are required:

1. Use int64 types.
2. Limit offset values from -((2^33)-1) to +((2^33)-1)

You agree with this?


> > +// TODO: Instead of using absolute timestamp offsets, use frame numbers
>
> Alternatively? Or perhaps also AV_OPT_TYPE_DURATION? TODO is for later,
> anyway...

Perhaps I'll remove the TODO comments. The reason to appear is because some
other developers in the mailing-list have pointed some interesting ideas for
this plugin in the future. I'm not sure to remove it or not.


> > +   if (!s->first_debug_done) {
> > +          av_log(ctx, AV_LOG_DEBUG, "Updated PTS/DTS (%"PRId64"/%"PRId64" : %"PRId64"/%"PRId64") with offset:%d\\n",
> > +                  pkt->pts, pkt->dts, opts, odts, s->offset );
> > +   }
> > +   s->first_debug_done = 1;
>
> You should set the variable inside the if() block. otherwise it gets
> assigned on every packet.

Yes, a small optimization.


> > +static const AVClass timer_class = {
> > +   .class_name = "timer",
>
> In about half of the existing BSFs, this would be called "timer_bsf". I
> don't care, I just look at other existing code. ;-)
>

Good point! Other bitstream filters have this. I'll update it.
Thank you!


Regards.
A.H.

---
Alexander Strasser Aug. 6, 2019, 10:24 p.m. UTC | #3
Hi Andreas!

On 2019-08-05 17:25 +0000, Andreas Håkon wrote:
> On Monday, 5 de August de 2019 17:31, Moritz Barsnick <barsnick@gmx.net> wrote:
>
[...]

> > > +static const AVClass timer_class = {
> > > +   .class_name = "timer",
> >
> > In about half of the existing BSFs, this would be called "timer_bsf". I
> > don't care, I just look at other existing code. ;-)
> >
>
> Good point! Other bitstream filters have this. I'll update it.
> Thank you!

Sorry for the noise. IIRC the naming was previously discussed.
Can't remember the details right now. Unfortunately I don't have
any good name suggestions too :(

This is no objections, but at least to me the name timer sounds
like some autonomous unit/process that can interrupt/call your
process on defined/configured/subscribed intervals. Maybe it's
just me...

...what do others think?


  Alexander
Andreas Håkon Aug. 7, 2019, 8:01 a.m. UTC | #4
Hi Alexander,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, 7 de August de 2019 0:24, Alexander Strasser <eclipse7@gmx.net> wrote:

> Hi Andreas!
>
> On 2019-08-05 17:25 +0000, Andreas Håkon wrote:
>
> > On Monday, 5 de August de 2019 17:31, Moritz Barsnick barsnick@gmx.net wrote:
>
> [...]
>
> > > > +static const AVClass timer_class = {
> > > >
> > > > -   .class_name = "timer",
> > >
> > > In about half of the existing BSFs, this would be called "timer_bsf". I
> > > don't care, I just look at other existing code. ;-)
> >
> > Good point! Other bitstream filters have this. I'll update it.
> > Thank you!
>
> Sorry for the noise. IIRC the naming was previously discussed.
> Can't remember the details right now. Unfortunately I don't have
> any good name suggestions too :(
>
> This is no objections, but at least to me the name timer sounds
> like some autonomous unit/process that can interrupt/call your
> process on defined/configured/subscribed intervals. Maybe it's
> just me...
>
> ...what do others think?

It's nitial name was "retimer". After a suggestion I changed to "timer".
Other options can be "timestamp", "synchronizer", etc.
However, they're all a little vague.

Therefore, I ask someone to present an appropriate name taking
into account the objective: shifting the PTS/DTS marks of a stream.

You like "timeshift"?

Regards.
A.H.

---
Andreas Håkon Aug. 7, 2019, 3:52 p.m. UTC | #5
Hi Alexander,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, 7 de August de 2019 10:01, Andreas Håkon <andreas.hakon@protonmail.com> wrote:

> Hi Alexander,
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Wednesday, 7 de August de 2019 0:24, Alexander Strasser eclipse7@gmx.net wrote:
>
> > Hi Andreas!
> > On 2019-08-05 17:25 +0000, Andreas Håkon wrote:
> >
> > > On Monday, 5 de August de 2019 17:31, Moritz Barsnick barsnick@gmx.net wrote:
> >
> > [...]
> >
> > Sorry for the noise. IIRC the naming was previously discussed.
> > Can't remember the details right now. Unfortunately I don't have
> > any good name suggestions too :(
> > This is no objections, but at least to me the name timer sounds
> > like some autonomous unit/process that can interrupt/call your
> > process on defined/configured/subscribed intervals. Maybe it's
> > just me...
> > ...what do others think?
>
> It's nitial name was "retimer". After a suggestion I changed to "timer".
> Other options can be "timestamp", "synchronizer", etc.
> However, they're all a little vague.
>
> Therefore, I ask someone to present an appropriate name taking
> into account the objective: shifting the PTS/DTS marks of a stream.
>
> You like "timeshift"?

Check new version:
https://patchwork.ffmpeg.org/patch/14302/

Regards.
A.H.

---
diff mbox

Patch

diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
index 50a1679..2d92eeb 100644
--- a/doc/bitstream_filters.texi
+++ b/doc/bitstream_filters.texi
@@ -644,6 +644,16 @@  codec) with metadata headers.
 
 See also the @ref{mov2textsub} filter.
 
+@section timer
+
+Apply an offset to the PTS/DTS timestamps.
+
+@table @option
+@item offset
+The offset value to apply to the PTS/DTS timestamps.
+
+@end table
+
 @section trace_headers
 
 Log trace output containing all syntax elements in the coded stream
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3cd73fb..2c2f018 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -1096,6 +1096,7 @@  OBJS-$(CONFIG_NULL_BSF)                   += null_bsf.o
 OBJS-$(CONFIG_PRORES_METADATA_BSF)        += prores_metadata_bsf.o
 OBJS-$(CONFIG_REMOVE_EXTRADATA_BSF)       += remove_extradata_bsf.o
 OBJS-$(CONFIG_TEXT2MOVSUB_BSF)            += movsub_bsf.o
+OBJS-$(CONFIG_TIMER_BSF)                  += timer_bsf.o
 OBJS-$(CONFIG_TRACE_HEADERS_BSF)          += trace_headers_bsf.o
 OBJS-$(CONFIG_TRUEHD_CORE_BSF)            += truehd_core_bsf.o mlp_parse.o mlp.o
 OBJS-$(CONFIG_VP9_METADATA_BSF)           += vp9_metadata_bsf.o
diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c
index 4630039..be6af05 100644
--- a/libavcodec/bitstream_filters.c
+++ b/libavcodec/bitstream_filters.c
@@ -51,6 +51,7 @@  extern const AVBitStreamFilter ff_null_bsf;
 extern const AVBitStreamFilter ff_prores_metadata_bsf;
 extern const AVBitStreamFilter ff_remove_extradata_bsf;
 extern const AVBitStreamFilter ff_text2movsub_bsf;
+extern const AVBitStreamFilter ff_timer_bsf;
 extern const AVBitStreamFilter ff_trace_headers_bsf;
 extern const AVBitStreamFilter ff_truehd_core_bsf;
 extern const AVBitStreamFilter ff_vp9_metadata_bsf;
diff --git a/libavcodec/timer_bsf.c b/libavcodec/timer_bsf.c
new file mode 100644
index 0000000..3f1c7f6
--- /dev/null
+++ b/libavcodec/timer_bsf.c
@@ -0,0 +1,86 @@ 
+/*
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @file
+ * This bitstream filter applies an offset to the PTS/DTS timestamps.
+ *
+ */
+
+// TODO: Control time wrapping
+// TODO: Instead of using absolute timestamp offsets, use frame numbers
+
+#include "avcodec.h"
+#include "bsf.h"
+
+#include "libavutil/opt.h"
+
+typedef struct TimerContext {
+    const AVClass *class;
+    int offset;
+    int first_debug_done;
+} TimerContext;
+
+static int timer_filter(AVBSFContext *ctx, AVPacket *pkt)
+{
+    int ret;
+    TimerContext *s = ctx->priv_data;
+    int64_t opts,odts;
+
+    ret = ff_bsf_get_packet_ref(ctx, pkt);
+    if (ret < 0)
+        return ret;
+
+    opts = pkt->pts;
+    if (pkt->pts != AV_NOPTS_VALUE) {
+        pkt->pts += s->offset;
+    }
+    odts = pkt->dts;
+    if (pkt->dts != AV_NOPTS_VALUE) {
+        pkt->dts += s->offset;
+    }
+
+    if (!s->first_debug_done) {
+        av_log(ctx, AV_LOG_DEBUG, "Updated PTS/DTS (%"PRId64"/%"PRId64" : %"PRId64"/%"PRId64") with offset:%d\n",
+                pkt->pts, pkt->dts, opts, odts, s->offset );
+    }
+    s->first_debug_done = 1;
+
+    return 0;
+}
+
+#define OFFSET(x) offsetof(TimerContext, x)
+#define FLAGS (AV_OPT_FLAG_VIDEO_PARAM|AV_OPT_FLAG_AUDIO_PARAM|AV_OPT_FLAG_SUBTITLE_PARAM|AV_OPT_FLAG_BSF_PARAM)
+static const AVOption options[] = {
+    { "offset", NULL, OFFSET(offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, FLAGS },
+    { NULL },
+};
+
+static const AVClass timer_class = {
+    .class_name = "timer",
+    .item_name  = av_default_item_name,
+    .option     = options,
+    .version    = LIBAVUTIL_VERSION_INT,
+};
+
+const AVBitStreamFilter ff_timer_bsf = {
+    .name           = "timer",
+    .priv_data_size = sizeof(TimerContext),
+    .priv_class     = &timer_class,
+    .filter         = timer_filter,
+};