diff mbox series

[FFmpeg-devel] lavc/mlp_parse: Read wordlength from 0xba streams

Message ID CAB0OVGon-SR2WMQ98OS4zFYNu5dR_yhY5Ntkq8L-HwvYvRbnVA@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] lavc/mlp_parse: Read wordlength from 0xba streams
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Carl Eugen Hoyos Feb. 13, 2020, 11:29 p.m. UTC
Hi!

Attached patch allows detecting s16 truehd streams encoded with
FFmpeg, only tested with FFmpeg's encoder, I did not look into any
specification.

Please comment, Carl Eugen

Comments

Hendrik Leppkes Feb. 14, 2020, 12:20 a.m. UTC | #1
On Fri, Feb 14, 2020 at 12:29 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Hi!
>
> Attached patch allows detecting s16 truehd streams encoded with
> FFmpeg, only tested with FFmpeg's encoder, I did not look into any
> specification.
>

According to Dolbys Bitstream specification this read does not seem
right. It reads half of a reserved field and 3 single-bit control
fields - in a structure called "channel meaning", which otherwise only
includes fields on channel assignment and interpretation, so this
field being in there seems weird.
Also, why would they code a literal value, and not a lookup table with
fewer bits like the 0xbb case does?

Unless we can find an actual real-world sample from a licensed encoder
that can confirm the presence and accuracy of this field, I'm going to
assume its not correct. It looks to me like it may be writing a MLP
(ie. 0xbb) header, and not a TrueHD header - beyond the first
differences, anyway. The high-level bitstream specification was not
available when mlpenc.c was initially written.
Carl Eugen Hoyos Feb. 14, 2020, 10:36 a.m. UTC | #2
Am Fr., 14. Feb. 2020 um 01:26 Uhr schrieb Hendrik Leppkes
<h.leppkes@gmail.com>:
>
> On Fri, Feb 14, 2020 at 12:29 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >
> > Attached patch allows detecting s16 truehd streams encoded with
> > FFmpeg, only tested with FFmpeg's encoder, I did not look into any
> > specification.
>
> According to Dolbys Bitstream specification this read does not seem
> right. It reads half of a reserved field and 3 single-bit control
> fields - in a structure called "channel meaning", which otherwise only
> includes fields on channel assignment and interpretation, so this
> field being in there seems weird.
> Also, why would they code a literal value, and not a lookup table with
> fewer bits like the 0xbb case does?
>
> Unless we can find an actual real-world sample from a licensed encoder
> that can confirm the presence and accuracy of this field, I'm going to
> assume its not correct. It looks to me like it may be writing a MLP
> (ie. 0xbb) header, and not a TrueHD header - beyond the first
> differences, anyway. The high-level bitstream specification was not
> available when mlpenc.c was initially written.

Thank you for looking into this!
Is there a link to the high-level bitstream specification?

Carl Eugen
Hendrik Leppkes Feb. 14, 2020, 10:48 a.m. UTC | #3
On Fri, Feb 14, 2020 at 11:45 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Am Fr., 14. Feb. 2020 um 01:26 Uhr schrieb Hendrik Leppkes
> <h.leppkes@gmail.com>:
> >
> > On Fri, Feb 14, 2020 at 12:29 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > >
> > > Attached patch allows detecting s16 truehd streams encoded with
> > > FFmpeg, only tested with FFmpeg's encoder, I did not look into any
> > > specification.
> >
> > According to Dolbys Bitstream specification this read does not seem
> > right. It reads half of a reserved field and 3 single-bit control
> > fields - in a structure called "channel meaning", which otherwise only
> > includes fields on channel assignment and interpretation, so this
> > field being in there seems weird.
> > Also, why would they code a literal value, and not a lookup table with
> > fewer bits like the 0xbb case does?
> >
> > Unless we can find an actual real-world sample from a licensed encoder
> > that can confirm the presence and accuracy of this field, I'm going to
> > assume its not correct. It looks to me like it may be writing a MLP
> > (ie. 0xbb) header, and not a TrueHD header - beyond the first
> > differences, anyway. The high-level bitstream specification was not
> > available when mlpenc.c was initially written.
>
> Thank you for looking into this!
> Is there a link to the high-level bitstream specification?
>

Its here:
https://developer.dolby.com/globalassets/technology/dolby-truehd/dolbytruehdhighlevelbitstreamdescription.pdf

- Hendrik
Carl Eugen Hoyos Feb. 14, 2020, 11 a.m. UTC | #4
Am Fr., 14. Feb. 2020 um 11:48 Uhr schrieb Hendrik Leppkes
<h.leppkes@gmail.com>:
>
> On Fri, Feb 14, 2020 at 11:45 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >
> > Am Fr., 14. Feb. 2020 um 01:26 Uhr schrieb Hendrik Leppkes
> > <h.leppkes@gmail.com>:
> > >
> > > On Fri, Feb 14, 2020 at 12:29 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > > >
> > > > Attached patch allows detecting s16 truehd streams encoded with
> > > > FFmpeg, only tested with FFmpeg's encoder, I did not look into any
> > > > specification.
> > >
> > > According to Dolbys Bitstream specification this read does not seem
> > > right. It reads half of a reserved field and 3 single-bit control
> > > fields - in a structure called "channel meaning", which otherwise only
> > > includes fields on channel assignment and interpretation, so this
> > > field being in there seems weird.
> > > Also, why would they code a literal value, and not a lookup table with
> > > fewer bits like the 0xbb case does?
> > >
> > > Unless we can find an actual real-world sample from a licensed encoder
> > > that can confirm the presence and accuracy of this field, I'm going to
> > > assume its not correct. It looks to me like it may be writing a MLP
> > > (ie. 0xbb) header, and not a TrueHD header - beyond the first
> > > differences, anyway. The high-level bitstream specification was not
> > > available when mlpenc.c was initially written.
> >
> > Thank you for looking into this!
> > Is there a link to the high-level bitstream specification?
> >
>
> Its here:
> https://developer.dolby.com/globalassets/technology/dolby-truehd/dolbytruehdhighlevelbitstreamdescription.pdf

Thank you.
The way I read this document is that precision is not stored and that we need
a decoder option to force decoding less than 24 bits.

Carl Eugen
Paul B Mahol Feb. 14, 2020, 11:02 a.m. UTC | #5
On 2/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> Am Fr., 14. Feb. 2020 um 11:48 Uhr schrieb Hendrik Leppkes
> <h.leppkes@gmail.com>:
>>
>> On Fri, Feb 14, 2020 at 11:45 AM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> wrote:
>> >
>> > Am Fr., 14. Feb. 2020 um 01:26 Uhr schrieb Hendrik Leppkes
>> > <h.leppkes@gmail.com>:
>> > >
>> > > On Fri, Feb 14, 2020 at 12:29 AM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> > > wrote:
>> > > >
>> > > > Attached patch allows detecting s16 truehd streams encoded with
>> > > > FFmpeg, only tested with FFmpeg's encoder, I did not look into any
>> > > > specification.
>> > >
>> > > According to Dolbys Bitstream specification this read does not seem
>> > > right. It reads half of a reserved field and 3 single-bit control
>> > > fields - in a structure called "channel meaning", which otherwise only
>> > > includes fields on channel assignment and interpretation, so this
>> > > field being in there seems weird.
>> > > Also, why would they code a literal value, and not a lookup table with
>> > > fewer bits like the 0xbb case does?
>> > >
>> > > Unless we can find an actual real-world sample from a licensed encoder
>> > > that can confirm the presence and accuracy of this field, I'm going to
>> > > assume its not correct. It looks to me like it may be writing a MLP
>> > > (ie. 0xbb) header, and not a TrueHD header - beyond the first
>> > > differences, anyway. The high-level bitstream specification was not
>> > > available when mlpenc.c was initially written.
>> >
>> > Thank you for looking into this!
>> > Is there a link to the high-level bitstream specification?
>> >
>>
>> Its here:
>> https://developer.dolby.com/globalassets/technology/dolby-truehd/dolbytruehdhighlevelbitstreamdescription.pdf
>
> Thank you.
> The way I read this document is that precision is not stored and that we
> need
> a decoder option to force decoding less than 24 bits.

No, your patch is nonsense.

Encoder stores only 24 bit pcm, 16bit pcm is upconverted to 24 bit
inside encoder,
which is far from ideal.
Carl Eugen Hoyos Feb. 14, 2020, 11:04 a.m. UTC | #6
Am Fr., 14. Feb. 2020 um 12:02 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>
> On 2/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> > The way I read this document is that precision is not stored and that we
> > need a decoder option to force decoding less than 24 bits.
>
> No, your patch is nonsense.

Hendrik already explained this

> Encoder stores only 24 bit pcm, 16bit pcm is upconverted to 24 bit
> inside encoder, which is far from ideal.

Isn't this what I wrote above?

Carl Eugen
Paul B Mahol Feb. 14, 2020, 11:07 a.m. UTC | #7
On 2/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> Am Fr., 14. Feb. 2020 um 12:02 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>>
>> On 2/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> > The way I read this document is that precision is not stored and that we
>> > need a decoder option to force decoding less than 24 bits.
>>
>> No, your patch is nonsense.
>
> Hendrik already explained this
>
>> Encoder stores only 24 bit pcm, 16bit pcm is upconverted to 24 bit
>> inside encoder, which is far from ideal.
>
> Isn't this what I wrote above?
>

No, I'm of opinion that 16bit should be remove from encoder. Instead
of adding option to decoder.
Carl Eugen Hoyos Feb. 14, 2020, 11:08 a.m. UTC | #8
Am Fr., 14. Feb. 2020 um 12:07 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>
> On 2/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > Am Fr., 14. Feb. 2020 um 12:02 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
> >>
> >> On 2/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >
> >> > The way I read this document is that precision is not stored and that we
> >> > need a decoder option to force decoding less than 24 bits.
> >>
> >> No, your patch is nonsense.
> >
> > Hendrik already explained this
> >
> >> Encoder stores only 24 bit pcm, 16bit pcm is upconverted to 24 bit
> >> inside encoder, which is far from ideal.
> >
> > Isn't this what I wrote above?
> >
>
> No, I'm of opinion that 16bit should be remove from encoder. Instead
> of adding option to decoder.

Understood.

Carl Eugen
diff mbox series

Patch

From 1b98303ab87463037e05e66f3129112fc5c6e484 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Fri, 14 Feb 2020 00:25:52 +0100
Subject: [PATCH] lavc/mlp_parse: Read wordlength from 0xba streams.

Fixes detecting 16 bit streams encoded with FFmpeg.
---
 libavcodec/mlp_parse.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/libavcodec/mlp_parse.c b/libavcodec/mlp_parse.c
index 45715352c2..74173acba3 100644
--- a/libavcodec/mlp_parse.c
+++ b/libavcodec/mlp_parse.c
@@ -123,7 +123,6 @@  int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, GetBitContext *gb)
         mh->channels_mlp       = mlp_channels[channel_arrangement];
         mh->channel_layout_mlp = mlp_layout[channel_arrangement];
     } else if (mh->stream_type == 0xba) {
-        mh->group1_bits = 24; // TODO: Is this information actually conveyed anywhere?
         mh->group2_bits = 0;
 
         ratebits = get_bits(gb, 4);
@@ -159,7 +158,15 @@  int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, GetBitContext *gb)
 
     mh->num_substreams = get_bits(gb, 4);
 
-    skip_bits_long(gb, 4 + (header_size - 17) * 8);
+    if (mh->stream_type == 0xba) {
+        skip_bits(gb, 17);
+        mh->group1_bits = get_bits(gb, 5);
+        if (!mh->group1_bits)
+            mh->group1_bits = 24;
+        skip_bits_long(gb, 4 + (header_size - 17) * 8 - 22);
+    } else {
+        skip_bits_long(gb, 4 + (header_size - 17) * 8);
+    }
 
     return 0;
 }
-- 
2.24.1