diff mbox

[FFmpeg-devel] libavcodec: add timeshift bitstream filter [v3]

Message ID U7F4N-WTbw_wr2ajhVpHw2nRhvFZrm5FBFIMQ1BHg_jgXeNpO0b366cyrWUwXj3QlhPW9_7ZiVrcEh6KZJhzRwpltYwsyNTFNoOB7Tn48EI=@protonmail.com
State New
Headers show

Commit Message

Andreas Håkon Aug. 7, 2019, 3:51 p.m. UTC
Hi,

This new version changes the name of the filter, and implements all suggestions.
Supersedes:
https://patchwork.ffmpeg.org/patch/14195/
https://patchwork.ffmpeg.org/patch/13743/
https://patchwork.ffmpeg.org/patch/13699/

Regards.
A.H.

---
From 9e036e0a1dc20fd5c41686a48a5a0c2142f91de4 Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@protonmail.com>
Date: Wed, 7 Aug 2019 17:45:25 +0200
Subject: [PATCH] libavcodec: add timeshift bitstream filter [v3]

This implements the new timeshift bitstream filter.

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

Comments

Alexander Strasser Aug. 7, 2019, 11:43 p.m. UTC | #1
On 2019-08-07 15:51 +0000, Andreas Håkon wrote:
> Hi,
>
> This new version changes the name of the filter, and implements all suggestions.

Sorry, I couldn't reply earlier.

Thanks for renaming; "timeshift" sounds better to me, compared to
the previous "timer".

Other suggestions which I came up with:

* adjust_timestamps
* edit_timestamps

Instead of the _timestamps suffix _ts or _ptsdts could be used.

Maybe others have better suggestions. For me the current name
timeshift is acceptable. If you decide to rename it yet again,
I would recommend to not send new versions of the patch with
only name change for now. It's better to wait for potential
review comments.

To make it clear I did only read your patch and *didn't review*
it at all. I wanted to comment on the name in time, because it's
always significantly more effort to deal with user visible naming
once the code is merged.


  Alexander

> Supersedes:
> https://patchwork.ffmpeg.org/patch/14195/
> https://patchwork.ffmpeg.org/patch/13743/
> https://patchwork.ffmpeg.org/patch/13699/
>
> Regards.
> A.H.
>
> ---

> From 9e036e0a1dc20fd5c41686a48a5a0c2142f91de4 Mon Sep 17 00:00:00 2001
> From: Andreas Hakon <andreas.hakon@protonmail.com>
> Date: Wed, 7 Aug 2019 17:45:25 +0200
> Subject: [PATCH] libavcodec: add timeshift bitstream filter [v3]
>
> This implements the new timeshift bitstream filter.
>
> Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com>
> ---
>  doc/bitstream_filters.texi     |   11 +++++
>  libavcodec/Makefile            |    1 +
>  libavcodec/bitstream_filters.c |    1 +
>  libavcodec/timeshift_bsf.c     |   87 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 100 insertions(+)
>  create mode 100644 libavcodec/timeshift_bsf.c
>
[...]
Andreas Håkon Aug. 8, 2019, 5:54 a.m. UTC | #2
Hi Alexander,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, 8 de August de 2019 1:43, Alexander Strasser <eclipse7@gmx.net> wrote:

> On 2019-08-07 15:51 +0000, Andreas Håkon wrote:
>
> > Hi,
> > This new version changes the name of the filter, and implements all suggestions.
>
> Thanks for renaming; "timeshift" sounds better to me, compared to
> the previous "timer".
>
> Other suggestions which I came up with:
>
> -   adjust_timestamps
> -   edit_timestamps
>
>     Instead of the _timestamps suffix _ts or _ptsdts could be used.

I like the name "edit_ts", it's short and clear.
Opinions?


>     Maybe others have better suggestions. For me the current name
>     timeshift is acceptable. If you decide to rename it yet again,
>     I would recommend to not send new versions of the patch with
>     only name change for now. It's better to wait for potential
>     review comments.

OK.


>     To make it clear I did only read your patch and didn't review
>     it at all. I wanted to comment on the name in time, because it's
>     always significantly more effort to deal with user visible naming
>     once the code is merged.

I hope someone will review it soon.

Regards.
A.H.

---
Nicolas George Aug. 8, 2019, 8:46 a.m. UTC | #3
Alexander Strasser (12019-08-08):
> Maybe others have better suggestions.

setpts or setts, similar to lavfi.

Although in lavfi I have a vague intention of merging all the set*
filters into a single one.

Regards,
Andreas Håkon Aug. 8, 2019, 9:09 a.m. UTC | #4
Hi Nicolas,

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, 8 de August de 2019 10:46, Nicolas George <george@nsup.org> wrote:

> Alexander Strasser (12019-08-08):
>
> > Maybe others have better suggestions.
>
> setpts or setts, similar to lavfi.

Then I proposse these two alternatives:

1) "editpts_bsf"
2) "setpts_bsf"

The "_bsf" is not real, only to differenciate (here) from other filters.
Which one do you prefer?


> Although in lavfi I have a vague intention of merging all the set*
> filters into a single one.

OK.


Regards.
A.H.

---
Gyan Doshi Aug. 8, 2019, 9:15 a.m. UTC | #5
On 08-08-2019 02:16 PM, Nicolas George wrote:
> Alexander Strasser (12019-08-08):
>> Maybe others have better suggestions.
> setpts or setts, similar to lavfi.

This BSF only allows to apply a fixed offset whereas the setpts filter 
allows arbitrary expressions. Unless someone is planning to extend this 
BSF,  the current candidate is more apt.

Gyan
Alexander Strasser Aug. 9, 2019, 1:18 a.m. UTC | #6
Hi Nicolas!

On 2019-08-08 11:43 +0200, Nicolas George wrote:
> Andreas Håkon (12019-08-08):
> > > setpts or setts, similar to lavfi.
>
> > Then I proposse these two alternatives:
> >
> > 1) "editpts_bsf"
> > 2) "setpts_bsf"
> >
> > The "_bsf" is not real, only to differenciate (here) from other filters.
> > Which one do you prefer?
>
> editpts has no similarity with lavfi. Users would need to remember if it
> is filters <-> set and bsf <-> edit or filters <-> edit and bsf <-> set.
> Better have a single name.

Right, I thought about that too. It could get confusing. Still my initial
suggestions were on purpose not setpts. More on that below.


> Bitstream filters and lavfi are not in the same namespace, therefore the
> suffix is unneeded.
>
>
> Gyan (12019-08-08):
> > This BSF only allows to apply a fixed offset whereas the setpts filter
> > allows arbitrary expressions. Unless someone is planning to extend this
> > BSF,  the current candidate is more apt.

Like Gyan mentioned here, that was my reason to refrain from proposing
setpts as a name for this bsf. I too think, that one would expect being
able to use free expressions for the timestamp specification. And that
would cause similar confusions like the one you described above, where
users would think

    "ah, it's setpts I can just give it the offset I want"

vs

    "ah, it's setpts I can give it an expressions that evaluates
    to the time stamp I want to set"

Also if I'm not mistaken for the lavfi filter the name setpts is on
spot whereas for the bsf we would also deal with dts, which was not
a consideration when implementing the setpts filter and defining its
arguments, which is just a single argument named "expr".


> At some point, somebody will implement generic expression eval.

That might happen, but it's IMHO too vague to rely on for making
name decisions right in this moment.


> When
> that happens, it will be better if they can just add an option to the
> existing filter rather than creating a whole new filter, deprecating the
> existing one, etc.

That sounds a bit exaggerating to me:

1. Building a whole new bsf would not be that different from changing the
   existing one, except from having to copy and modify the boiler plate
   related to implementing bsfs in general. Maybe a couple of lines in
   the existing implementation of the bsf could be reused, maybe not.
2. The existing one wouldn't have to be deprecating, as it's so simple
   that it could stay there providing an alternative filter with a
   different interface and implementation. Or the code could be shared,
   which at the moment doesn't seem to have any benefits, but would also
   share the flaws for which there will be more opportunities in the bsf
   with expression evaluation.
3. If one would decide, one doesn't want to have the two achieving the
   same thing. One could replace the documentation of this bsf with
   "Only for backward compatibility. Use bsf setpts."

So yes it might be better if, but even then probably not much.

Also if it happens that all set filters of lavfi really get merged into
a single filter, we would have chosen the wrong name now.


> Key idea: long term planning.

I agree that long term planning is important, though I disagree with
your conclusion in this specific discussion.

The whole bsf framework is probably a nice long-term move. It's
flexibility of allowing bsfs as individual filters that are handled in
a generic fashion, provides a good ground for implementing a diverse,
useful and partially redundant set of bsfs.

Seeing it this way it eases long-term planning by removing the need for
detailed planning and allowing experimentation and evolution without
having to sacrifice compatibility.


> Therefore, I am for setpts (dts is just secondary), with a mention in
> the documentation:
>
> # Unlike the setpts filter in libavfilter, this filter currently can
> # only apply a constant offset.

Another name idea

* ts_offset

loosely in reference to -itsoffset and output_ts_offset (AVFormatContext).

And from the initial discussion there was:

* add_delay


Regarding setpts and other filters like addroi and so on, I find it a bit
unfortunate that they don't have an underscore. "set_" and "add_" is much
better for doing full text searches on the docs or the complete codebase.


If you still think setpts is the best name for that bsf, it's fine for
me; my main concern was to not name it "timer".


  Alexander
Andreas Håkon Aug. 9, 2019, 6:44 a.m. UTC | #7
Hi Gyan, Nicolas and Alexander,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 9 de August de 2019 3:18, Alexander Strasser <eclipse7@gmx.net> wrote:

>
> [...]
> Gyan
>
> [...]
>   Nicolas George
>
> [...]
>     Alexander

I appreciate your suggestions with the name. But, we need to select one.
So I propose a short list:

- "timeshift" (the current name)
- "pts_offset"
- "editpts"

I don't like "ts_offset" as "ts" can be understood as Transport Stream.
I don't want "add_delay" as it can add (aka delay) or substract (aka advance).
And I don't recommend "editpts" since at present it can't edit time stamps freely.

I expect your answers.
Regards.
A.H.

---
Nicolas George Aug. 10, 2019, 3:02 p.m. UTC | #8
Andreas Håkon (12019-08-09):
> I appreciate your suggestions with the name. But, we need to select one.
> So I propose a short list:
> 
> - "timeshift" (the current name)
> - "pts_offset"
> - "editpts"
> 
> I don't like "ts_offset" as "ts" can be understood as Transport Stream.
> I don't want "add_delay" as it can add (aka delay) or substract (aka advance).
> And I don't recommend "editpts" since at present it can't edit time stamps freely.

My arguments have been read and understood, counter-arguments have been
given. I trust you to make the final decision.

Regards,
Gyan Doshi Aug. 10, 2019, 3:11 p.m. UTC | #9
On 09-08-2019 12:14 PM, Andreas Håkon wrote:
> Hi Gyan, Nicolas and Alexander,
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Friday, 9 de August de 2019 3:18, Alexander Strasser <eclipse7@gmx.net> wrote:
>
>> [...]
>> Gyan
>>
>> [...]
>>    Nicolas George
>>
>> [...]
>>      Alexander
> I appreciate your suggestions with the name. But, we need to select one.
> So I propose a short list:
>
> - "timeshift" (the current name)
> - "pts_offset"
> - "editpts"
>
> I don't like "ts_offset" as "ts" can be understood as Transport Stream.
> I don't want "add_delay" as it can add (aka delay) or substract (aka advance).
> And I don't recommend "editpts" since at present it can't edit time stamps freely.
Since both pts and dts are altered, avoid pts only labels. We already 
have output_ts_offset without leading to confusion. ts_offset is fine; 
ts_edit if we're futureproofing.

Gyan
Nicolas George Aug. 10, 2019, 3:13 p.m. UTC | #10
Gyan (12019-08-10):
> Since both pts and dts are altered, avoid pts only labels.

Dissenting opinion: PTS is the important thing, and there are lot of
places where we says PTS because TS is ambiguous (Transport Stream?).
DTS is just an annoying technical tidbit that goes with PTS.

Regards,
Andreas Håkon Aug. 10, 2019, 6:05 p.m. UTC | #11
Hi Gyan and Nicolas,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, 10 de August de 2019 17:13, Nicolas George <george@nsup.org> wrote:

> Gyan (12019-08-10):
>
> > Since both pts and dts are altered, avoid pts only labels.
>
> Dissenting opinion: PTS is the important thing, and there are lot of
> places where we says PTS because TS is ambiguous (Transport Stream?).
> DTS is just an annoying technical tidbit that goes with PTS.
>
> Regards,

I agree with Nicolas: the change of the DTS value is a consequence of changing
the PTS value, as the second is correlated with the first. It doesn't have sense
to change only one (at least, as far as a bitstream filter is concerned).

So my preference is to overcome "TS" in the name of this filter.

In any case, I prefer to listening more comments from others: "timeshift",
"pts_offest" or "editpts"?

Regards.
A.H.

---
Andreas Håkon Aug. 16, 2019, 8:29 a.m. UTC | #12
Hi,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, 10 de August de 2019 20:05, Andreas Håkon <andreas.hakon@protonmail.com> wrote:

> Hi Gyan and Nicolas,
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Saturday, 10 de August de 2019 17:13, Nicolas George george@nsup.org wrote:
>
> > Gyan (12019-08-10):
> >
> > > Since both pts and dts are altered, avoid pts only labels.
> >
> > Dissenting opinion: PTS is the important thing, and there are lot of
> > places where we says PTS because TS is ambiguous (Transport Stream?).
> > DTS is just an annoying technical tidbit that goes with PTS.
> > Regards,
>
> I agree with Nicolas: the change of the DTS value is a consequence of changing
> the PTS value, as the second is correlated with the first. It doesn't have sense
> to change only one (at least, as far as a bitstream filter is concerned).
>
> So my preference is to overcome "TS" in the name of this filter.
>
> In any case, I prefer to listening more comments from others: "timeshift",
> "pts_offest" or "editpts"?
>

I finally chose to use the name "editpts".
I'll post the final patch soon.
Regards.
A.H.

---
diff mbox

Patch

diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
index 50a1679..f7fb0d4 100644
--- a/doc/bitstream_filters.texi
+++ b/doc/bitstream_filters.texi
@@ -644,6 +644,17 @@  codec) with metadata headers.
 
 See also the @ref{mov2textsub} filter.
 
+@section timeshift
+
+Apply an offset to the PTS/DTS timestamps.
+
+@table @option
+@item offset
+The absolute offset value to apply to the PTS/DTS timestamps with a
+90kHz resolution.
+
+@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..e929a99 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_TIMESHIFT_BSF)              += timeshift_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..d728185 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_timeshift_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/timeshift_bsf.c b/libavcodec/timeshift_bsf.c
new file mode 100644
index 0000000..1af9152
--- /dev/null
+++ b/libavcodec/timeshift_bsf.c
@@ -0,0 +1,87 @@ 
+/*
+ * 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.
+ *
+ */
+
+#include "avcodec.h"
+#include "bsf.h"
+
+#include "libavutil/opt.h"
+
+#define FILTER_MAX_PTS 0x200000000  // (2^33)
+
+typedef struct TimeshiftContext {
+    const AVClass *class;
+    int offset;
+    int first_debug_done;
+} TimeshiftContext;
+
+static int timeshift_filter(AVBSFContext *ctx, AVPacket *pkt)
+{
+    int ret;
+    TimeshiftContext *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;
+        pkt->pts &= FILTER_MAX_PTS;
+    }
+    odts = pkt->dts;
+    if (pkt->dts != AV_NOPTS_VALUE) {
+        pkt->dts += s->offset;
+        pkt->pts &= FILTER_MAX_PTS;
+    }
+
+    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(TimeshiftContext, 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_INT64, { .i64 = 0 }, 1-FILTER_MAX_PTS, FILTER_MAX_PTS, FLAGS },
+    { NULL },
+};
+
+static const AVClass timeshift_class = {
+    .class_name = "timeshift_bsf",
+    .item_name  = av_default_item_name,
+    .option     = options,
+    .version    = LIBAVUTIL_VERSION_INT,
+};
+
+const AVBitStreamFilter ff_timeshift_bsf = {
+    .name           = "timeshift",
+    .priv_data_size = sizeof(TimeshiftContext),
+    .priv_class     = &timeshift_class,
+    .filter         = timeshift_filter,
+};