| Message ID | CAB0OVGon-SR2WMQ98OS4zFYNu5dR_yhY5Ntkq8L-HwvYvRbnVA@mail.gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [FFmpeg-devel] lavc/mlp_parse: Read wordlength from 0xba streams | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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.
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
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
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
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.
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
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.
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
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