diff mbox

[FFmpeg-devel] libavcodec: add editpts bitstream filter [v4]

Message ID MLpZt0KMOfe1y7U5V0NUi_dqFYwWi-AORjfMWbyzIgBA1dAvjpHoiQQuPe1gNBr0MjFVzFfm3S24qSfWUglu2Mrh7f35yjb9Nt6qDkzuRnA=@protonmail.com
State New
Headers show

Commit Message

Andreas Håkon Aug. 16, 2019, 10:19 a.m. UTC
Hi,

The latest version ready to merge of the bitstream filter "editpts".
Implements all requests.

This supersedes:

https://patchwork.ffmpeg.org/patch/14302/

https://patchwork.ffmpeg.org/patch/14195/

https://patchwork.ffmpeg.org/patch/13743/

https://patchwork.ffmpeg.org/patch/13699/

I hope it will be accepted soon.
Regards.
A.H.

---
From 1cc8b06cdb7e2d38c528897d9f9f19b9cbcaf81a Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@protonmail.com>
Date: Fri, 16 Aug 2019 12:11:01 +0200
Subject: [PATCH] libavcodec: add editpts bitstream filter [v4]

This implements the new editpts 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/editpts_bsf.c       |   87 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+)
 create mode 100644 libavcodec/editpts_bsf.c

Comments

Andreas Håkon Aug. 20, 2019, 7:07 a.m. UTC | #1
Ping!

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 16 de August de 2019 12:19, Andreas Håkon <andreas.hakon@protonmail.com> wrote:

> Hi,
>
> The latest version ready to merge of the bitstream filter "editpts".
> Implements all requests.
>
> This supersedes:
>
> https://patchwork.ffmpeg.org/patch/14302/
>
> https://patchwork.ffmpeg.org/patch/14195/
>
> https://patchwork.ffmpeg.org/patch/13743/
>
> https://patchwork.ffmpeg.org/patch/13699/

And I explain the reason for this filter:
- When there is a slight misalignment in some stream, for example a lipsync failure,
it's possible to compensate such misalignment without having to recompress any stream.
Just by applying this filter you can do all the work.

Regards.
A.H.

---
Andreas Håkon Aug. 27, 2019, 9:02 a.m. UTC | #2
Hi,

Ping!

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

> On Friday, 16 de August de 2019 12:19, Andreas Håkon <andreas.hakon@protonmail.com> wrote:
>
>> Hi,
>>
>> The latest version ready to merge of the bitstream filter "editpts".
>> Implements all requests.
>>
>> This supersedes:
>>
>> https://patchwork.ffmpeg.org/patch/14302/
>>
>> https://patchwork.ffmpeg.org/patch/14195/
>>
>> https://patchwork.ffmpeg.org/patch/13743/
>>
>> https://patchwork.ffmpeg.org/patch/13699/
>
> And I explain the reason for this filter:
> - When there is a slight misalignment in some stream, for example a lipsync failure,
> it's possible to compensate such misalignment without having to recompress any stream.
> Just by applying this filter you can do all the work.
>
> Regards.
> A.H.

After many comments for versions v1, v2 and v3, I don't see any comments for this last version.
I hope it will be accepted soon.

Regards.
A.H.

---
Andreas Håkon Sept. 3, 2019, 7:06 a.m. UTC | #3
Hi,

Ping, ping! (another time).

Please take note of these three points:

1. Lip-Syncing is a relevant topic in the AV editing area. Almost all professional video editors have
the functionality to realign audio with video. And at time the ffmpeg project lacks for a lipsync
bitstream filter. For this reason, this new filter is relevant. Although it can also be used for other purposes.

2. This is the fourth iteration of the patch, and it incorporates all suggested requests. So it's ready
for review and acceptance.

3. Although the current implementation can be improved (using, for example, values expressed in
nano-seconds), it's preferable to first add it and then improve it.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 27 de August de 2019 11:02, Andreas Håkon <andreas.hakon@protonmail.com> wrote:

> Hi,
>
> Ping!
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>
>> On Friday, 16 de August de 2019 12:19, Andreas Håkon <andreas.hakon@protonmail.com> wrote:
>>
>>> Hi,
>>>
>>> The latest version ready to merge of the bitstream filter "editpts".
>>> Implements all requests.
>>>
>>> This supersedes:
>>>
>>> https://patchwork.ffmpeg.org/patch/14302/
>>>
>>> https://patchwork.ffmpeg.org/patch/14195/
>>>
>>> https://patchwork.ffmpeg.org/patch/13743/
>>>
>>> https://patchwork.ffmpeg.org/patch/13699/
>>
>> And I explain the reason for this filter:
>> - When there is a slight misalignment in some stream, for example a lipsync failure,
>> it's possible to compensate such misalignment without having to recompress any stream.
>> Just by applying this filter you can do all the work.

I hope it will be reviewed and accepted soon.
Regards.
A.H.

---
Marton Balint Sept. 3, 2019, 7:57 a.m. UTC | #4
On Tue, 3 Sep 2019, Andreas Håkon wrote:

> Hi,
>
> Ping, ping! (another time).
>
> Please take note of these three points:
>
> 1. Lip-Syncing is a relevant topic in the AV editing area. Almost all professional video editors have
> the functionality to realign audio with video. And at time the ffmpeg project lacks for a lipsync
> bitstream filter. For this reason, this new filter is relevant. Although it can also be used for other purposes.
>
> 2. This is the fourth iteration of the patch, and it incorporates all suggested requests. So it's ready
> for review and acceptance.
>
> 3. Although the current implementation can be improved (using, for example, values expressed in
> nano-seconds), it's preferable to first add it and then improve it.

Some things can't be improved later, because they become part of the 
public interface so we can no longer just remove them, we'd have to 
deprecate them first.

Also ffmpeg tries to be consistent with other parts of the code, use names 
based on the same logic, or use similar concepts throughout the libraries.

Avoiding duplicated functionality is also a strong requirement for us, if 
something can be implemented in a more generic way (even if that takes a 
little more work), we should aim for that.

That is why I suggest you implement a more generic approach, a setpts like 
filter (setts) with evaluation. As others pointed out, you typically need 
to modify both PTS and DTS, so the expression should affect both. 
Additional parameters can be introduced if somebody wants different 
expressions for PTS and DTS.

Regards,
Marton
Paul B Mahol Sept. 3, 2019, 8:11 a.m. UTC | #5
On 9/3/19, Marton Balint <cus@passwd.hu> wrote:
>
>
> On Tue, 3 Sep 2019, Andreas Håkon wrote:
>
>> Hi,
>>
>> Ping, ping! (another time).
>>
>> Please take note of these three points:
>>
>> 1. Lip-Syncing is a relevant topic in the AV editing area. Almost all
>> professional video editors have
>> the functionality to realign audio with video. And at time the ffmpeg
>> project lacks for a lipsync
>> bitstream filter. For this reason, this new filter is relevant. Although
>> it can also be used for other purposes.
>>
>> 2. This is the fourth iteration of the patch, and it incorporates all
>> suggested requests. So it's ready
>> for review and acceptance.
>>
>> 3. Although the current implementation can be improved (using, for
>> example, values expressed in
>> nano-seconds), it's preferable to first add it and then improve it.
>
> Some things can't be improved later, because they become part of the
> public interface so we can no longer just remove them, we'd have to
> deprecate them first.
>
> Also ffmpeg tries to be consistent with other parts of the code, use names
> based on the same logic, or use similar concepts throughout the libraries.
>
> Avoiding duplicated functionality is also a strong requirement for us, if
> something can be implemented in a more generic way (even if that takes a
> little more work), we should aim for that.
>
> That is why I suggest you implement a more generic approach, a setpts like
> filter (setts) with evaluation. As others pointed out, you typically need
> to modify both PTS and DTS, so the expression should affect both.
> Additional parameters can be introduced if somebody wants different
> expressions for PTS and DTS.

Agree with Marton here, looking at source code of bitstream filter it
is very limited in functionality.

>
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Håkon Sept. 3, 2019, 12:31 p.m. UTC | #6
Hi Marton,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 3 de September de 2019 9:57, Marton Balint <cus@passwd.hu> wrote:

> On Tue, 3 Sep 2019, Andreas Håkon wrote:
>
> > Hi,
> > Ping, ping! (another time).
> > Please take note of these three points:
> >
> > 1.  Lip-Syncing is a relevant topic in the AV editing area. Almost all professional video editors have
> >     the functionality to realign audio with video. And at time the ffmpeg project lacks for a lipsync
> >     bitstream filter. For this reason, this new filter is relevant. Although it can also be used for other purposes.
> >
> > 2.  This is the fourth iteration of the patch, and it incorporates all suggested requests. So it's ready
> >     for review and acceptance.
> >
> > 3.  Although the current implementation can be improved (using, for example, values expressed in
> >     nano-seconds), it's preferable to first add it and then improve it.
> >
>
> Some things can't be improved later, because they become part of the
> public interface so we can no longer just remove them, we'd have to
> deprecate them first.
>
> Also ffmpeg tries to be consistent with other parts of the code, use names
> based on the same logic, or use similar concepts throughout the libraries.
>
> Avoiding duplicated functionality is also a strong requirement for us, if
> something can be implemented in a more generic way (even if that takes a
> little more work), we should aim for that.
>
> That is why I suggest you implement a more generic approach, a setpts like
> filter (setts) with evaluation. As others pointed out, you typically need
> to modify both PTS and DTS, so the expression should affect both.
> Additional parameters can be introduced if somebody wants different
> expressions for PTS and DTS.

After looking at the "libavfilter/setpts.c" and the documentation I don't see what you want to copy from it.
The "expression" functionality can be useful, but the number of constants compatible with a bitstream filter
are very limited. From my point of view only some of them have sense: PTS and T in fact.

Any suggestion?
Perhaps you should describe the functionality of the "setpts_bsf" filter.

Regards.
A.H.

---
Marton Balint Sept. 3, 2019, 8:59 p.m. UTC | #7
On Tue, 3 Sep 2019, Andreas Håkon wrote:

> Hi Marton,
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Tuesday, 3 de September de 2019 9:57, Marton Balint <cus@passwd.hu> wrote:
>
>> On Tue, 3 Sep 2019, Andreas Håkon wrote:
>>
>> > Hi,
>> > Ping, ping! (another time).
>> > Please take note of these three points:
>> >
>> > 1.  Lip-Syncing is a relevant topic in the AV editing area. Almost all professional video editors have
>> >     the functionality to realign audio with video. And at time the ffmpeg project lacks for a lipsync
>> >     bitstream filter. For this reason, this new filter is relevant. Although it can also be used for other purposes.
>> >
>> > 2.  This is the fourth iteration of the patch, and it incorporates all suggested requests. So it's ready
>> >     for review and acceptance.
>> >
>> > 3.  Although the current implementation can be improved (using, for example, values expressed in
>> >     nano-seconds), it's preferable to first add it and then improve it.
>> >
>>
>> Some things can't be improved later, because they become part of the
>> public interface so we can no longer just remove them, we'd have to
>> deprecate them first.
>>
>> Also ffmpeg tries to be consistent with other parts of the code, use names
>> based on the same logic, or use similar concepts throughout the libraries.
>>
>> Avoiding duplicated functionality is also a strong requirement for us, if
>> something can be implemented in a more generic way (even if that takes a
>> little more work), we should aim for that.
>>
>> That is why I suggest you implement a more generic approach, a setpts like
>> filter (setts) with evaluation. As others pointed out, you typically need
>> to modify both PTS and DTS, so the expression should affect both.
>> Additional parameters can be introduced if somebody wants different
>> expressions for PTS and DTS.
>
> After looking at the "libavfilter/setpts.c" and the documentation I don't see what you want to copy from it.
> The "expression" functionality can be useful, but the number of constants compatible with a bitstream filter
> are very limited. From my point of view only some of them have sense: PTS and T in fact.

- PTS
- DTS
- STARTDTS
- TB - stream time base
- TS - when a single expression is used, TS is pts when pts is evaluated, 
TS is dts when dts is evaluated.

>
> Any suggestion?
> Perhaps you should describe the functionality of the "setpts_bsf" filter.

name is "setts".

options:
ts=<string>
dts=<string>
pts=<string>

<string> is an expression to be evaluated. Either the ts expression or 
both pts and dts expressions are defined.

If only ts expression is defined then it is evaluated for both PTS and 
DTS.

If pts and dts expressions are defined, they are used instead of the 
common expression.

Regards,
Marton
diff mbox

Patch

diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
index 50a1679..dc73d20 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 editpts
+
+Change the values of the PTS/DTS timestamps.
+
+@table @option
+@item offset
+An 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..f4cc2be 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_EDITPTS_BSF)                += editpts_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..59d48ef 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_editpts_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/editpts_bsf.c b/libavcodec/editpts_bsf.c
new file mode 100644
index 0000000..8355dac
--- /dev/null
+++ b/libavcodec/editpts_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 changes the PTS/DTS timestamps.
+ *
+ */
+
+#include "avcodec.h"
+#include "bsf.h"
+
+#include "libavutil/opt.h"
+
+#define FILTER_MAX_PTS 0x200000000  // (2^33)
+
+typedef struct EditptsContext {
+    const AVClass *class;
+    int offset;
+    int first_debug_done;
+} EditptsContext;
+
+static int editpts_filter(AVBSFContext *ctx, AVPacket *pkt)
+{
+    int ret;
+    EditptsContext *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(EditptsContext, 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 editpts_class = {
+    .class_name = "editpts_bsf",
+    .item_name  = av_default_item_name,
+    .option     = options,
+    .version    = LIBAVUTIL_VERSION_INT,
+};
+
+const AVBitStreamFilter ff_editpts_bsf = {
+    .name           = "editpts",
+    .priv_data_size = sizeof(EditptsContext),
+    .priv_class     = &editpts_class,
+    .filter         = editpts_filter,
+};