[FFmpeg-devel] [PATCH] mlp_parser: fetch a new timestamp when major sync is found

[FFmpeg-devel] [PATCH] mlp_parser: fetch a new timestamp when major sync is found

[Jan Ekström]

From: Hendrik Leppkes <h.leppkes@gmail.com>

Decoding can only start on a major sync and the parser discards
any data received until that point. Therefore, previously received
timestamps are no longer relevant to the data being returned from
the parser.

Additionally, adds a test which remuxes an existing TrueHD sample
with the first keyframe dropped using copyinkf, and then demuxes
this remux with ffprobe. Previously this would have pushed the
first random access point packet out as pts=1, even though it would
have contained the data received from packet containing pts=13.

With this fix, the first data received from the parser properly
contains the pts of the packet in which it was contained.

Fixes #9428

Co-authored-by: Jan Ekström <jeebjp@gmail.com>
---
 libavcodec/mlp_parser.c                 |  1 +
 tests/fate/truehd.mak                   | 12 ++++++++++++
 tests/ref/fate/truehd-parser-timestamps | 26 +++++++++++++++++++++++++
 3 files changed, 39 insertions(+)
 create mode 100644 tests/ref/fate/truehd-parser-timestamps

diff --git a/libavcodec/mlp_parser.c b/libavcodec/mlp_parser.c
index 9fea7db955..70d7b3f601 100644
--- a/libavcodec/mlp_parser.c
+++ b/libavcodec/mlp_parser.c
@@ -96,6 +96,7 @@ static int mlp_parse(AVCodecParserContext *s,
                 return ret;
             }
 
+            s->fetch_timestamp = 1;
             return i - 7;
         }
 
diff --git a/tests/fate/truehd.mak b/tests/fate/truehd.mak
index 7da8c93cff..6f451cc327 100644
--- a/tests/fate/truehd.mak
+++ b/tests/fate/truehd.mak
@@ -13,5 +13,17 @@ fate-truehd-core-bsf: CMD = md5pipe -i $(TARGET_SAMPLES)/truehd/atmos.thd -c:a c
 fate-truehd-core-bsf: CMP = oneline
 fate-truehd-core-bsf: REF = 3aa5d0c7825051f3657b71fd6135183b
 
+# Tests that the result from reading a copyinkf remux with the first random
+# access point dropped will receive the correct timestamp for the first packet,
+# which is not the packet of the first packet read.
+FATE_TRUEHD-$(call ALLYES, FILE_PROTOCOL PIPE_PROTOCOL TRUEHD_DEMUXER \
+                           MLP_PARSER MATROSKA_MUXER NOISE_BSF) \
+                           += fate-truehd-parser-timestamps
+fate-truehd-parser-timestamps: CMD = stream_remux "truehd" \
+    "$(TARGET_SAMPLES)/lossless-audio/truehd_5.1.raw" "matroska" \
+    "-map 0:a -copyinkf -bsf:a noise=drop=not\(n\)*key -t 0.030" \
+    "-c copy -copyts"
+fate-truehd-parser-timestamps: CMP = diff
+
 FATE_SAMPLES_AUDIO += $(FATE_TRUEHD-yes)
 fate-truehd: $(FATE_TRUEHD-yes)
diff --git a/tests/ref/fate/truehd-parser-timestamps b/tests/ref/fate/truehd-parser-timestamps
new file mode 100644
index 0000000000..94d5a05640
--- /dev/null
+++ b/tests/ref/fate/truehd-parser-timestamps
@@ -0,0 +1,26 @@
+#tb 0: 1/1000
+#media_type 0: audio
+#codec_id 0: truehd
+#sample_rate 0: 48000
+#channel_layout 0: 60f
+#channel_layout_name 0: 5.1(side)
+0,         13,         13,        1,      368, 0x256aaaad
+0,         14,         14,        1,      190, 0x7b975b90, F=0x0
+0,         15,         15,        1,      178, 0xfdc85dc0, F=0x0
+0,         16,         16,        1,      184, 0x3a605a1c, F=0x0
+0,         17,         17,        1,      184, 0xf00e5aef, F=0x0
+0,         18,         18,        1,      186, 0x95125c74, F=0x0
+0,         18,         18,        1,      184, 0xc6ae624e, F=0x0
+0,         19,         19,        1,      186, 0x5f9e5de9, F=0x0
+0,         20,         20,        1,      188, 0x80135cc7, F=0x0
+0,         21,         21,        1,      188, 0x10dc5a3d, F=0x0
+0,         22,         22,        1,      188, 0x63ff5980, F=0x0
+0,         23,         23,        1,      198, 0x42265ec5, F=0x0
+0,         23,         23,        1,      210, 0x4d2f6d78, F=0x0
+0,         24,         24,        1,      202, 0x44b6654f, F=0x0
+0,         25,         25,        1,      192, 0x098e6271, F=0x0
+0,         26,         26,        1,      194, 0x090f691d, F=0x0
+0,         27,         27,        1,      382, 0x88b0ac79
+0,         28,         28,        1,      196, 0xf0765d20, F=0x0
+0,         28,         28,        1,      188, 0x72c65e8d, F=0x0
+0,         29,         29,        1,      186, 0x913a5fcb, F=0x0
-- 
2.35.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] mlp_parser: fetch a new timestamp when major sync is found

[Michael Niedermayer]

[-- Attachment #1.1.1: Type: text/plain, Size: 1535 bytes --]

On Mon, Mar 07, 2022 at 12:32:59AM +0200, Jan Ekström wrote:
> From: Hendrik Leppkes <h.leppkes@gmail.com>
> 
> Decoding can only start on a major sync

> and the parser discards
> any data received until that point.

Thats a bug and its inconsistant
For example video codec parsers do not drop frames until a keyframe
And droping that in a video codec would if someone suggested it
lead to people opposing as that would drop actual data
that can partly be decoded. Its better to drop it in the decoder
if its unwanted.
So why is this considered to be "ok" in audio codecs?
Because we dont visually see that data is lost ?

So let me show it vissually :)
i deleted frame 1, we have 3 decoder outputs
1. complete data
2. no frame 1 but instead frame 17 duplicated in frame 1s place
3. no frame 1

The decoder produces a few errors in case 2 but there seems more
data coming out which seems resembling the data that is droped
its not 1:1 identical but it looks like there is recoverable data
see attached screenshoot from audacity
Maybe its a coincidence

but i dont think we should just build on top of this droping
logic without some argumentation why this droped data can really
not be used ever in any way (it may become harder to fix it the
more is build on top)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri

[-- Attachment #1.1.2: truehd.ec1.png --]
[-- Type: image/png, Size: 69950 bytes --]

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] mlp_parser: fetch a new timestamp when major sync is found

[Hendrik Leppkes]

On Mon, Mar 7, 2022 at 6:10 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
> So why is this considered to be "ok" in audio codecs?
> Because we dont visually see that data is lost ?

This patch is not trying to argue that its "ok" to drop data, nor does
it introduce this behavior. Someone else did that years ago.

>
> but i dont think we should just build on top of this droping
> logic without some argumentation why this droped data can really
> not be used ever in any way (it may become harder to fix it the
> more is build on top)
>

There is no question that dropping the data inside a parser is not the
appropriate and documented behavior, but it is what it does, and what
it has done for ages.

Leaving an actual bug because some theoretical better fix might be
available is not rather productive, especially when the fix is one
short line - unless you actually provide said better fix in a
reasonable timeframe.
It is not adding the dropping behavior, that already existed afterall,
its just fixing the timestamp - in a rather trivial manner, too.

If you, or anyone else, wants to change the parsers logic to not drop
data, this patch does not hinder that in any way. It immediately fixes
the bug that was brought to my attention a while ago, that this case
breaks timestamps.
Dropping of data in front of a keyframe is another bug, even if they
are practically undecodable without some manual hackery, a parser
should not do that - but this patch does not even touch on that
behavior, other then mentioning it in the message.

- Hendrik
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] mlp_parser: fetch a new timestamp when major sync is found

[Jan Ekström]

On Mon, Mar 7, 2022 at 7:10 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Mon, Mar 07, 2022 at 12:32:59AM +0200, Jan Ekström wrote:
> > From: Hendrik Leppkes <h.leppkes@gmail.com>
> >
> > Decoding can only start on a major sync
>
> > and the parser discards
> > any data received until that point.
>
> Thats a bug and its inconsistant
> For example video codec parsers do not drop frames until a keyframe
> And droping that in a video codec would if someone suggested it
> lead to people opposing as that would drop actual data
> that can partly be decoded. Its better to drop it in the decoder
> if its unwanted.
> So why is this considered to be "ok" in audio codecs?
> Because we dont visually see that data is lost ?
>

For the record, I am not advocating for this behavior in this parser,
nor is this patch adding the dropping. The dropping is and has already
been there.

Rather this is just the result of the following steps:

1. I improve certain parser calling code in an API client (mpv) to
actually utilize the returned timestamps due to certain formats such
as E-AC-3 having buffering/delay.
2. Some of the samples I have at hand were badly cut matroska files
from early 2010s where the muxer expected all packets to be random
access points.
3. I notice that while the E-AC-3 samples' timestamps are now correct,
the TrueHD samples' timestamps are now such that you get a jump
between the first returned data, and the rest of the returned data.
4. After staring at the matroska packets' sizes and timestamps on the
container level, I figure out that the parser is just dropping packets
until the first random access point packet is found, and then failing
to update the returned timestamp.
5. Thus I open https://trac.ffmpeg.org/ticket/9428 .
6. nev is nice enough to figure out how to make the parser actually
update the returned timestamp.
7. After the copyinkf stuff gets applied to master, I write a test for
it and reword the commit message, send the fix to the mailing list.

I do not have experience with the parser, but at least this fixes an
existing bug that I noticed when properly utilizing the parsing API :)
.

> So let me show it vissually :)
> i deleted frame 1, we have 3 decoder outputs
> 1. complete data
> 2. no frame 1 but instead frame 17 duplicated in frame 1s place
> 3. no frame 1
>
> The decoder produces a few errors in case 2 but there seems more
> data coming out which seems resembling the data that is droped
> its not 1:1 identical but it looks like there is recoverable data
> see attached screenshoot from audacity
> Maybe its a coincidence
>
> but i dont think we should just build on top of this droping
> logic without some argumentation why this droped data can really
> not be used ever in any way (it may become harder to fix it the
> more is build on top)
>
> thx
>

I'm OK with the parser being changed in stead, but unfortunately I do
not have the brain power/time to start looking into it myself.

For now I was happy enough to get a fix for the returned timestamps
posted on the mailing list, and have written a test for it.

Jan
_______________________________________________
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".