diff mbox

[FFmpeg-devel] lavf/spdifenc: Automatically insert truehd_core bitstream filter

Message ID CAB0OVGp00OZWdpQp6+qDDDhOpihe_N-+DKQxZpjXVh=GbG3BGA@mail.gmail.com
State Rejected
Headers show

Commit Message

Carl Eugen Hoyos Feb. 12, 2019, 11:28 a.m. UTC 
Hi!

Attached patch intends to fix ticket #7731.

Please comment, Carl Eugen

Comments

Hendrik Leppkes Feb. 12, 2019, 11:37 a.m. UTC | #1 
On Tue, Feb 12, 2019 at 12:28 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Hi!
>
> Attached patch intends to fix ticket #7731.
>

This is not a fix. The vast majority of TrueHD Atmos tracks work just
fine with the current limitations, and would needlessly drop the Atmos
information for every stream.

- Hendrik
Carl Eugen Hoyos Feb. 12, 2019, 11:54 a.m. UTC | #2 
2019-02-12 12:37 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> On Tue, Feb 12, 2019 at 12:28 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>>
>> Hi!
>>
>> Attached patch intends to fix ticket #7731.
>
> This is not a fix. The vast majority of TrueHD Atmos tracks work just
> fine with the current limitations, and would needlessly drop the Atmos
> information for every stream.

Is it possible to detect if the Atmos core has to be dropped?

Carl Eugen
Hendrik Leppkes Feb. 12, 2019, 11:47 p.m. UTC | #3 
On Tue, Feb 12, 2019 at 12:54 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2019-02-12 12:37 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> > On Tue, Feb 12, 2019 at 12:28 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> > wrote:
> >>
> >> Hi!
> >>
> >> Attached patch intends to fix ticket #7731.
> >
> > This is not a fix. The vast majority of TrueHD Atmos tracks work just
> > fine with the current limitations, and would needlessly drop the Atmos
> > information for every stream.
>
> Is it possible to detect if the Atmos core has to be dropped?
>

Not beforehand, since the size of future frames is of course unknown,
and there are no indications in the bitstream.
One could consider starting to drop Atmos data after it happened once,
but it'll probably still glitch out audio at that point.

Ideally the truehd spdif muxer should be revised to support a more
flexible buffering model, but its a tad bit complicated with the way
the spdif muxer is setup, and I've only recently learned myself how
its presumably supposed to be done, since the specifications are not
openly available.

- Hendrik
Carl Eugen Hoyos Feb. 13, 2019, 1:56 a.m. UTC | #4 
2019-02-13 0:47 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> On Tue, Feb 12, 2019 at 12:54 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>>
>> 2019-02-12 12:37 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
>> > On Tue, Feb 12, 2019 at 12:28 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> > wrote:
>> >>
>> >> Hi!
>> >>
>> >> Attached patch intends to fix ticket #7731.
>> >
>> > This is not a fix. The vast majority of TrueHD Atmos tracks work just
>> > fine with the current limitations, and would needlessly drop the Atmos
>> > information for every stream.
>>
>> Is it possible to detect if the Atmos core has to be dropped?
>
> Not beforehand, since the size of future frames is of course unknown,
> and there are no indications in the bitstream.
> One could consider starting to drop Atmos data after it happened once,
> but it'll probably still glitch out audio at that point.
>
> Ideally the truehd spdif muxer should be revised to support a more
> flexible buffering model, but its a tad bit complicated with the way
> the spdif muxer is setup, and I've only recently learned myself how
> its presumably supposed to be done, since the specifications are not
> openly available.

I did a few experiments before reading your mail:
If I use a burst rate of significantly less than 2000 bytes
audio gets broken with my receiver.
Can you confirm that in any way?
Otoh, it does not seem to help to insert memset(0)
between frames if the burstrate is too low.
("burstrate": gap between beginnings of thd frames)

It is not necessary to put 12 frames in both half-MAT frames,
15 and 9 work fine here.

My receiver always fails / produces hickups if the thd stream
contains atmos data: Are you sure it is supposed to work?
Can you already provide a test stream for the audio stream
from the ticket?

Carl Eugen
Hendrik Leppkes Feb. 13, 2019, 7:10 a.m. UTC | #5 
On Wed, Feb 13, 2019 at 2:57 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2019-02-13 0:47 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> > On Tue, Feb 12, 2019 at 12:54 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> > wrote:
> >>
> >> 2019-02-12 12:37 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> >> > On Tue, Feb 12, 2019 at 12:28 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> >> > wrote:
> >> >>
> >> >> Hi!
> >> >>
> >> >> Attached patch intends to fix ticket #7731.
> >> >
> >> > This is not a fix. The vast majority of TrueHD Atmos tracks work just
> >> > fine with the current limitations, and would needlessly drop the Atmos
> >> > information for every stream.
> >>
> >> Is it possible to detect if the Atmos core has to be dropped?
> >
> > Not beforehand, since the size of future frames is of course unknown,
> > and there are no indications in the bitstream.
> > One could consider starting to drop Atmos data after it happened once,
> > but it'll probably still glitch out audio at that point.
> >
> > Ideally the truehd spdif muxer should be revised to support a more
> > flexible buffering model, but its a tad bit complicated with the way
> > the spdif muxer is setup, and I've only recently learned myself how
> > its presumably supposed to be done, since the specifications are not
> > openly available.
>
> I did a few experiments before reading your mail:
> If I use a burst rate of significantly less than 2000 bytes
> audio gets broken with my receiver.
> Can you confirm that in any way?
> Otoh, it does not seem to help to insert memset(0)
> between frames if the burstrate is too low.
> ("burstrate": gap between beginnings of thd frames)
>
> It is not necessary to put 12 frames in both half-MAT frames,
> 15 and 9 work fine here.
>
> My receiver always fails / produces hickups if the thd stream
> contains atmos data: Are you sure it is supposed to work?

With every hardware? Who knows. My receiver does not support Atmos
either and it plays streams that exceed the 2560 size just fine with
correct muxing into MAT frames - although I haven't tested that one in
the ticket specifically I don't think, and I'm not near that receiver
until next week.

> Can you already provide a test stream for the audio stream
> from the ticket?
>

Sure, why not, although I have no idea how one would play this, since
you would need to make sure full MAT frames are always read and output
as one (ie. every 61440 bytes), and unfortunately our raw PCM demuxer
does not support specifying a wanted packet size, oh well.
https://files.1f0.de/tmp/truehd_11mbit_bug.spdif

Otherwise it should fit the typical TrueHD bitstreaming criteria, ie.
192kHz 8ch 16-bit "PCM", 61440 bytes frame size.

- Hendrik
Carl Eugen Hoyos Feb. 14, 2019, 7:12 p.m. UTC | #6 
2019-02-13 8:10 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> On Wed, Feb 13, 2019 at 2:57 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>> 2019-02-13 0:47 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
>> > On Tue, Feb 12, 2019 at 12:54 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> > wrote:
>> >>
>> >> 2019-02-12 12:37 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
>> >> > On Tue, Feb 12, 2019 at 12:28 PM Carl Eugen Hoyos
>> >> > <ceffmpeg@gmail.com>
>> >> > wrote:
>> >> >>
>> >> >> Hi!
>> >> >>
>> >> >> Attached patch intends to fix ticket #7731.
>> >> >
>> >> > This is not a fix. The vast majority of TrueHD Atmos tracks work just
>> >> > fine with the current limitations, and would needlessly drop the
>> >> > Atmos
>> >> > information for every stream.
>> >>
>> >> Is it possible to detect if the Atmos core has to be dropped?
>> >
>> > Not beforehand, since the size of future frames is of course unknown,
>> > and there are no indications in the bitstream.
>> > One could consider starting to drop Atmos data after it happened once,
>> > but it'll probably still glitch out audio at that point.
>> >
>> > Ideally the truehd spdif muxer should be revised to support a more
>> > flexible buffering model, but its a tad bit complicated with the way
>> > the spdif muxer is setup, and I've only recently learned myself how
>> > its presumably supposed to be done, since the specifications are not
>> > openly available.
>>
>> I did a few experiments before reading your mail:
>> If I use a burst rate of significantly less than 2000 bytes
>> audio gets broken with my receiver.
>> Can you confirm that in any way?
>> Otoh, it does not seem to help to insert memset(0)
>> between frames if the burstrate is too low.
>> ("burstrate": gap between beginnings of thd frames)
>>
>> It is not necessary to put 12 frames in both half-MAT frames,
>> 15 and 9 work fine here.
>>
>> My receiver always fails / produces hickups if the thd stream
>> contains atmos data: Are you sure it is supposed to work?
>
> With every hardware? Who knows. My receiver does not support Atmos
> either and it plays streams that exceed the 2560 size just fine with
> correct muxing into MAT frames - although I haven't tested that one in
> the ticket specifically I don't think, and I'm not near that receiver
> until next week.
>
>> Can you already provide a test stream for the audio stream
>> from the ticket?
>>
>
> Sure, why not, although I have no idea how one would play this, since
> you would need to make sure full MAT frames are always read and output
> as one (ie. every 61440 bytes), and unfortunately our raw PCM demuxer
> does not support specifying a wanted packet size, oh well.
> https://files.1f0.de/tmp/truehd_11mbit_bug.spdif

This stream works with my receiver that does not support Atmon,
my patch creates a bitexact output.

Thank you, Carl Eugen
diff mbox

Patch

From bf68eb44a9a27ca8c9e832e8f0cbd08a0d0b5281 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Tue, 12 Feb 2019 12:15:02 +0100
Subject: [PATCH] lavf/spdifenc: Automatically insert truehd_core bitstream
 filter.

Fixes ticket #7731.
---
 libavformat/spdifenc.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/libavformat/spdifenc.c b/libavformat/spdifenc.c
index 9514ff8..ecfe28c 100644
--- a/libavformat/spdifenc.c
+++ b/libavformat/spdifenc.c
@@ -49,6 +49,7 @@ 
 #include "avformat.h"
 #include "avio_internal.h"
 #include "spdif.h"
+#include "internal.h"
 #include "libavcodec/ac3.h"
 #include "libavcodec/adts_parser.h"
 #include "libavcodec/dca.h"
@@ -546,6 +547,18 @@  static int spdif_write_packet(struct AVFormatContext *s, AVPacket *pkt)
     return 0;
 }
 
+static int spdif_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)
+{
+    int ret = 1;
+    AVStream *st = s->streams[pkt->stream_index];
+
+    if (st->codecpar->codec_id == AV_CODEC_ID_TRUEHD) {
+        ret = ff_stream_add_bitstream_filter(st, "truehd_core", NULL);
+    }
+
+    return ret;
+}
+
 AVOutputFormat ff_spdif_muxer = {
     .name              = "spdif",
     .long_name         = NULL_IF_CONFIG_SMALL("IEC 61937 (used on S/PDIF - IEC958)"),
@@ -556,6 +569,7 @@  AVOutputFormat ff_spdif_muxer = {
     .write_header      = spdif_write_header,
     .write_packet      = spdif_write_packet,
     .write_trailer     = spdif_write_trailer,
+    .check_bitstream   = spdif_check_bitstream,
     .flags             = AVFMT_NOTIMESTAMPS,
     .priv_class        = &spdif_class,
 };
-- 
1.7.10.4