Hi all, I've been working for a while on reverse engineering the MLP audio stream format, and writing my own decoder. I've finally got to a point where I have something ready for review. I'm going to try to get the documentation I've produced on the format up on the multimediawiki as well. I've attached the code as two patches - the first adds a parser, the second the actual decoder. Current caveats: - The decoder is limited by a #define to a maximum of six channels. I imagine that 7.1 streams will be more-or-less a case of changing the #defines, but I'd like to see a sample stream first to check. - Surround files are output with the channels in codec order (L, R, C, LFE, Ls, Rs for 5.1). There was some discussion on the list earlier on adding this kind of code to libavcodec instead of individual codecs, so I've left out reordering code for now. - Funky MLP stuff, like having some channels at 96KHz and some at 48KHz isn't supported - Blu-Ray TrueHD streams aren't supported. Apparently these consist of an AC3 base layer, plus a TrueHD layer with residual data or somesuch in. Anyone have any samples? Extracting a two-channel downmix from a 5.1 stream is supported using avctx->request_channels Comments welcome :) Ian
Ian Caulfield wrote:
Hi all,
I've been working for a while on reverse engineering the MLP audio stream format, and writing my own decoder. I've finally got to a point where I have something ready for review. I'm going to try to get the documentation I've produced on the format up on the multimediawiki as well.
I've attached the code as two patches - the first adds a parser, the second the actual decoder.
Current caveats:
- The decoder is limited by a #define to a maximum of six channels. I imagine that 7.1 streams will be more-or-less a case of changing the #defines, but I'd like to see a sample stream first to check. - Surround files are output with the channels in codec order (L, R, C, LFE, Ls, Rs for 5.1). There was some discussion on the list earlier on adding this kind of code to libavcodec instead of individual codecs, so I've left out reordering code for now. - Funky MLP stuff, like having some channels at 96KHz and some at 48KHz isn't supported - Blu-Ray TrueHD streams aren't supported. Apparently these consist of an AC3 base layer, plus a TrueHD layer with residual data or somesuch in. Anyone have any samples?
Atrac lossless uses the same principle. Atrac3(+) plus some residual coded stream.
Extracting a two-channel downmix from a 5.1 stream is supported using avctx->request_channels
Comments welcome :)
Ian
Here's a comment, fucking awesome, great work!! Now there's no reason to code up complete DVD Audio support into mplayer and friends. http://www.sendspace.com/file/atzn56 MvH Benjamin Larsson
On Fri, Oct 12, 2007 at 01:29:46PM +0100, Ian Caulfield wrote:
I've been working for a while on reverse engineering the MLP audio stream format, and writing my own decoder. I've finally got to a point where I have something ready for review. I'm going to try to get the documentation I've produced on the format up on the multimediawiki as well.
New decoders are always very much welcome :)
Comments welcome :)
--- libavcodec/mlp_parser.c (revision 0) +++ libavcodec/mlp_parser.c (revision 0) @@ -0,0 +1,338 @@ +/* + * MLP parser + * Copyright (c) 2007 Ian Caulfield.
nit: This period is useless.
+/** + * @file mlp_parser.c + * MLP parser.
ditto
--- libavcodec/mlp_parser.h (revision 0) +++ libavcodec/mlp_parser.h (revision 0) @@ -0,0 +1,53 @@ +/* + * MLP parser prototypes + * Copyright (c) 2007 Ian Caulfield.
ditto
+/** + * @file mlp_parser.h + * MLP parser prototypes.
ditto, same in the other patch A slightly more useful comment: The header file needs multiple inclusion guards. The build system part of the patch is OK. Diego
On 10/12/07, Ian Caulfield <ian.caulfield at gmail.com> wrote:
Comments welcome :)
It works with some of the samples that I had. Though http://www1.mplayerhq.hu/MPlayer/samples/mlp/God%20Save%20The%20Queen.mlp did not play properly in ffplay. Perhaps because downmixing wasn't happening? Attached is a combined patch that registers the codecs and also adds raw mlp support. -Jeff
On 12/10/2007, Jeff Muizelaar <jrmuizel at gmail.com> wrote:
On 10/12/07, Ian Caulfield <ian.caulfield at gmail.com> wrote:
Comments welcome :)
It works with some of the samples that I had. Though http://www1.mplayerhq.hu/MPlayer/samples/mlp/God%20Save%20The%20Queen.mlp did not play properly in ffplay. Perhaps because downmixing wasn't happening?
Probably - I haven't been able to get ffplay to work with multichannel audio properly, and I don't know if it uses the request_channels field yet.
Attached is a combined patch that registers the codecs and also adds raw mlp support.
Thanks! I'd missed these from my patch... Ian
Hi On Fri, Oct 12, 2007 at 01:29:46PM +0100, Ian Caulfield wrote:
Hi all,
I've been working for a while on reverse engineering the MLP audio stream format, and writing my own decoder. I've finally got to a point where I have something ready for review. I'm going to try to get the documentation I've produced on the format up on the multimediawiki as well.
I've attached the code as two patches - the first adds a parser, the second the actual decoder.
Current caveats:
- The decoder is limited by a #define to a maximum of six channels. I imagine that 7.1 streams will be more-or-less a case of changing the #defines, but I'd like to see a sample stream first to check. - Surround files are output with the channels in codec order (L, R, C, LFE, Ls, Rs for 5.1). There was some discussion on the list earlier on adding this kind of code to libavcodec instead of individual codecs, so I've left out reordering code for now. - Funky MLP stuff, like having some channels at 96KHz and some at 48KHz isn't supported - Blu-Ray TrueHD streams aren't supported. Apparently these consist of an AC3 base layer, plus a TrueHD layer with residual data or somesuch in. Anyone have any samples?
Extracting a two-channel downmix from a 5.1 stream is supported using avctx->request_channels
Comments welcome :) [...] +static inline int mlp_samplerate(int in) +{ + int rate; + + if (in == 0xF) + return 0; + + if (in & 8) + rate = 44100; + else + rate = 48000; + + rate <<= in & 7; + + return rate; +}
this doesnt look speed critical, so why is it inline? [...]
+ if (buf_size < 28) { + av_log(log, AV_LOG_ERROR, "Packet too short, unable to read major sync\n"); + return -1; + } + + if (!crc_init) + { + av_crc_init(crc_2D, 0, 16, 0x002D, sizeof(crc_2D)); + crc_init = 1; + }
the {} are placed inconsistant
+ + crc = av_crc(crc_2D, 0, buf, 24) ^ AV_RL16(buf+24); + if (crc != AV_RL16(buf+26)) + { + av_log(log, AV_LOG_ERROR, "Major sync info header CRC error\n"); + return -1; + }
i would rather write crc = av_crc(crc_2D, 0, buf, 24); if (crc != (AV_RL16(buf+26) ^ AV_RL16(buf+24)))
+ + if (get_bits_long(gbp, 24) != 0xf8726f) /* Sync words */ + return -1; +
+ mh->stream_type = get_bits(gbp, 8); + if (mh->stream_type != 0xba && mh->stream_type != 0xbb) + return -1; + + if (mh->stream_type == 0xbb) { + mh->group1_bits = mlp_quants[get_bits(gbp, 4)]; + mh->group2_bits = mlp_quants[get_bits(gbp, 4)]; + + ratebits = get_bits(gbp, 4); + mh->group1_samplerate = mlp_samplerate(ratebits); + mh->group2_samplerate = mlp_samplerate(get_bits(gbp, 4)); + + skip_bits(gbp, 11); + + mh->channels_mlp = get_bits(gbp, 5); + } else { + mh->group1_bits = 24; // TODO: Is this information actually conveyed anywhere? + mh->group2_bits = 0; + + ratebits = get_bits(gbp, 4); + mh->group1_samplerate = mlp_samplerate(ratebits); + mh->group2_samplerate = 0; + + skip_bits(gbp, 8); + + mh->channels_thd_stream1 = get_bits(gbp, 5); + + skip_bits(gbp, 2); + + mh->channels_thd_stream2 = get_bits(gbp, 13); + }
if bb else if ba else return -1 [...]
+static int mlp_parse(AVCodecParserContext *s, + AVCodecContext *avctx, + const uint8_t **poutbuf, int *poutbuf_size, + const uint8_t *buf, int buf_size) +{ + GetBitContext bits; + MLPParseContext *pc = s->priv_data; + int bytes_read; + int sync_present; + uint8_t parity_bits; + int i, p, offset = 0; + + *poutbuf_size = 0; + +presync: + if (!pc->in_sync) + { + for (i = 0; i < buf_size; i++) { + pc->state = (pc->state << 8) | buf[i]; + if (pc->state == 0xf8726fba || pc->state == 0xf8726fbb) + { + if (i < 7) + { + memmove(pc->buf, pc->buf + pc->buf_size - (7 - i), (7 - i)); + pc->buf_size = 7 - i; + } else { + pc->buf_size = 0; + } + memcpy(pc->buf + pc->buf_size, buf + i - 7, 8 - pc->buf_size);
in the i<7 case buf + i - 7 will point outside of the array also the parser looks messy, maybe using ff_combine_frame() would help? ill review the decoder patch later [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct awnser.
New parser patch attached, I've tried to addess everyone's comments. On 13/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
this doesnt look speed critical, so why is it inline?
Fixed
the {} are placed inconsistant
Fixed
+ + crc = av_crc(crc_2D, 0, buf, 24) ^ AV_RL16(buf+24); + if (crc != AV_RL16(buf+26)) + { + av_log(log, AV_LOG_ERROR, "Major sync info header CRC error\n"); + return -1; + }
i would rather write crc = av_crc(crc_2D, 0, buf, 24); if (crc != (AV_RL16(buf+26) ^ AV_RL16(buf+24)))
I did it like this because of the retarded way that the MLP crc's don't quite match av_crc - the crc variable represents the CRC calculated over the first 26 bytes. I've added a comment to explain this.
if bb else if ba else return -1
Fixed
in the i<7 case buf + i - 7 will point outside of the array
also the parser looks messy, maybe using ff_combine_frame() would help?
OK, I've rewritten the parser to use ff_combine_frame - it's less messy, but still not as clean as most parsers. I guess only having sync words in a few of the packets doesn't help. Ian
On 15/10/2007, Ian Caulfield <ian.caulfield at gmail.com> wrote:
New parser patch attached, I've tried to addess everyone's comments.
Spotted a bug in that version - revised patch attached. Ian
On Mon, Oct 15, 2007 at 12:26:02PM +0100, Ian Caulfield wrote:
New parser patch attached, I've tried to addess everyone's comments. [...] +static int truehd_channels(int chanmap) +{ + int channels = 0; + + if (chanmap & 0x0001) // L,R + channels += 2; + if (chanmap & 0x0002) // C + channels += 1; + if (chanmap & 0x0004) // LFE + channels += 1; + if (chanmap & 0x0008) // Ls,Rs + channels += 2; + if (chanmap & 0x0010) // Lvh,Rvh + channels += 2; + if (chanmap & 0x0020) // Lc,Rc + channels += 2; + if (chanmap & 0x0040) // Lrs,Rrs + channels += 2; + if (chanmap & 0x0080) // Cs + channels += 1; + if (chanmap & 0x0100) // Ts + channels += 1; + if (chanmap & 0x0200) // Lsd,Rsd + channels += 2; + if (chanmap & 0x0400) // Lw,Rw + channels += 2; + if (chanmap & 0x0800) // Cvh + channels += 1; + if (chanmap & 0x1000) // LFE2 + channels += 1; + + return channels; +}
for(i=0; i<13; i++) channels += chan_count[i] * ((chanmap>>i)&1); [...]
+ // The MLP crc is calculated differently to av_crc, hence the need to + // XOR in the last two bytes instead of including them in the CRC
i say it again MLP does NOT calculate CRCs differently it calculates them incorrect this is like saying r*3 is a different way of finding the circumference of the circle, it is not differnt its just wrong CRCs are shortened cyclic codes (see any book about CRCs, maybe even wikipedia) what MLP stores are NOT shortened cyclic codes hence not CRCs! that is unless AV_RL16(buf+24) or AV_RL16(buf+26) is a constant [...]
+ if (i < 0) + pc->bytes_left -= -i;
-= - -> += [...]
+ if (pc->bytes_left == 0) { + // Find length of this packet + + for (i = 0; (pc->state & 0x10000) == 0 && i < buf_size; i++) + { + pc->state = (pc->state << 8) | buf[i]; + } + + if ((pc->state & 0x10000) == 0) + { + ff_combine_frame(&pc->pc, END_NOT_FOUND, &buf, &buf_size); + return buf_size; + }
the {} placement is inconsistant [...]
+ if ((((parity_bits >> 4) ^ parity_bits) & 0xF) != 0xF) { + av_log(NULL, AV_LOG_INFO, "mlpparse: parity check failed\n"); + pc->in_sync = 0; + pc->state = 0; + return -1; + } + } else { + MLPHeaderInfo mh; +
+ init_get_bits(&bits, buf + 4, (buf_size - 4) * 8); + if (ff_mlp_read_major_sync(NULL, &mh, &bits, buf + 4, buf_size - 4) < 0) {
please dont pass 2 redundant things, here its a pointer to the buffer and a GetBitContext
+ pc->in_sync = 0; + pc->state = 0; + return -1;
duplicate, a goto no_sync could be used ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope
New version of mlp parser patch attached. On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Oct 15, 2007 at 12:26:02PM +0100, Ian Caulfield wrote:
New parser patch attached, I've tried to addess everyone's comments. [...] +static int truehd_channels(int chanmap)
for(i=0; i<13; i++) channels += chan_count[i] * ((chanmap>>i)&1);
Fixed
[...]
+ // The MLP crc is calculated differently to av_crc, hence the need to + // XOR in the last two bytes instead of including them in the CRC
i say it again MLP does NOT calculate CRCs differently it calculates them incorrect this is like saying r*3 is a different way of finding the circumference of the circle, it is not differnt its just wrong
CRCs are shortened cyclic codes (see any book about CRCs, maybe even wikipedia) what MLP stores are NOT shortened cyclic codes hence not CRCs! that is unless AV_RL16(buf+24) or AV_RL16(buf+26) is a constant
Renamed to checksum
+ if (pc->bytes_left == 0) { + // Find length of this packet + + for (i = 0; (pc->state & 0x10000) == 0 && i < buf_size; i++) + { + pc->state = (pc->state << 8) | buf[i]; + } + + if ((pc->state & 0x10000) == 0) + { + ff_combine_frame(&pc->pc, END_NOT_FOUND, &buf, &buf_size); + return buf_size; + }
the {} placement is inconsistant
Fixed
+ init_get_bits(&bits, buf + 4, (buf_size - 4) * 8); + if (ff_mlp_read_major_sync(NULL, &mh, &bits, buf + 4, buf_size - 4) < 0) {
please dont pass 2 redundant things, here its a pointer to the buffer and a GetBitContext
Fixed
+ pc->in_sync = 0; + pc->state = 0; + return -1;
duplicate, a goto no_sync could be used ...
Fixed Ian
On Wed, Oct 17, 2007 at 01:31:19PM +0100, Ian Caulfield wrote:
New version of mlp parser patch attached.
--- libavcodec/mlp_parser.h (revision 0) +++ libavcodec/mlp_parser.h (revision 0) @@ -0,0 +1,58 @@ + +#ifndef MLP_PARSER_H +#define MLP_PARSER_H
FFMPEG_MLP_PARSER_H Diego
On Wed, Oct 17, 2007 at 01:31:19PM +0100, Ian Caulfield wrote:
New version of mlp parser patch attached.
On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Oct 15, 2007 at 12:26:02PM +0100, Ian Caulfield wrote:
New parser patch attached, I've tried to addess everyone's comments. [...] +static int truehd_channels(int chanmap)
for(i=0; i<13; i++) channels += chan_count[i] * ((chanmap>>i)&1);
Fixed
[...]
+ // The MLP crc is calculated differently to av_crc, hence the need to + // XOR in the last two bytes instead of including them in the CRC
i say it again MLP does NOT calculate CRCs differently it calculates them incorrect this is like saying r*3 is a different way of finding the circumference of the circle, it is not differnt its just wrong
CRCs are shortened cyclic codes (see any book about CRCs, maybe even wikipedia) what MLP stores are NOT shortened cyclic codes hence not CRCs! that is unless AV_RL16(buf+24) or AV_RL16(buf+26) is a constant
Renamed to checksum
+ if (pc->bytes_left == 0) { + // Find length of this packet + + for (i = 0; (pc->state & 0x10000) == 0 && i < buf_size; i++) + { + pc->state = (pc->state << 8) | buf[i]; + } + + if ((pc->state & 0x10000) == 0) + { + ff_combine_frame(&pc->pc, END_NOT_FOUND, &buf, &buf_size); + return buf_size; + }
the {} placement is inconsistant
Fixed
+ init_get_bits(&bits, buf + 4, (buf_size - 4) * 8); + if (ff_mlp_read_major_sync(NULL, &mh, &bits, buf + 4, buf_size - 4) < 0) {
please dont pass 2 redundant things, here its a pointer to the buffer and a GetBitContext
Fixed
+ pc->in_sync = 0; + pc->state = 0; + return -1;
duplicate, a goto no_sync could be used ...
Fixed
Ian
[...]
+ if (!pc->in_sync) { + // Not in sync - find a major sync header + + for (i = 0; i < buf_size; i++) { + pc->state = (pc->state << 8) | buf[i]; + if ((pc->state & 0xfffffffe) == 0xf8726fba) + { + pc->in_sync = 1; + break; + }
{} placement is inconsistant first its if() { then if() { [...]
+ ff_combine_frame(&pc->pc, END_NOT_FOUND, &buf, &buf_size);
i think using the same name for pc and pc->pc is not good [...]
+ // First nibble of a frame is a parity check of the first few nibbles + // Only check when this isn't a sync frame - syncs have a checksum + + parity_bits = 0; + for (p = 0; p < 4; p++) + parity_bits ^= buf[p]; + for (i = 0; i < pc->num_substreams; i++) { + parity_bits ^= buf[p]; + parity_bits ^= buf[p+1]; + + if (buf[p] & 0x80) { + parity_bits ^= buf[p+2]; + parity_bits ^= buf[p+3]; + p += 2; + } + p += 2; + } + + if ((((parity_bits >> 4) ^ parity_bits) & 0xF) != 0xF) { + av_log(NULL, AV_LOG_INFO, "mlpparse: parity check failed\n"); + goto lost_sync; + }
its not the parsers job to check frames the only reason for a parser to not return part of the input is if it cannot determine the frame boundaries (for example if the frame length from some header is invalid/damaged) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
Hi, On 22/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
its not the parsers job to check frames the only reason for a parser to not return part of the input is if it cannot determine the frame boundaries (for example if the frame length from some header is invalid/damaged)
The reasoning behind this was that MLP streams are a pain to parse - most frames contain no sync word, just a length at the start of the frame. There appears to be no protection against sync word emulation. Without checking the frame parity nibble, there's no way to tell if the stream was corrupted and the parser is now out of sync with the stream - if this happens the parser will just return garbage frame boundaries until the end of the stream. Ian
Hi On Tue, Oct 23, 2007 at 09:06:18AM +0100, Ian Caulfield wrote:
Hi,
On 22/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
its not the parsers job to check frames the only reason for a parser to not return part of the input is if it cannot determine the frame boundaries (for example if the frame length from some header is invalid/damaged)
The reasoning behind this was that MLP streams are a pain to parse - most frames contain no sync word, just a length at the start of the frame. There appears to be no protection against sync word emulation. Without checking the frame parity nibble, there's no way to tell if the stream was corrupted and the parser is now out of sync with the stream - if this happens the parser will just return garbage frame boundaries until the end of the stream.
well then leave that parity check in there [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle
Hi On 10/29/07, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Oct 23, 2007 at 09:06:18AM +0100, Ian Caulfield wrote:
The reasoning behind this was that MLP streams are a pain to parse - most frames contain no sync word, just a length at the start of the frame. There appears to be no protection against sync word emulation. Without checking the frame parity nibble, there's no way to tell if the stream was corrupted and the parser is now out of sync with the stream - if this happens the parser will just return garbage frame boundaries until the end of the stream.
well then leave that parity check in there
OK, updated patch addressing your other comments attached. Ian
Hi On Mon, Oct 29, 2007 at 10:50:39AM +0100, Ian Caulfield wrote:
Hi
On 10/29/07, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Oct 23, 2007 at 09:06:18AM +0100, Ian Caulfield wrote:
The reasoning behind this was that MLP streams are a pain to parse - most frames contain no sync word, just a length at the start of the frame. There appears to be no protection against sync word emulation. Without checking the frame parity nibble, there's no way to tell if the stream was corrupted and the parser is now out of sync with the stream - if this happens the parser will just return garbage frame boundaries until the end of the stream.
well then leave that parity check in there
OK, updated patch addressing your other comments attached.
i really dont like how the parser is implemented, not at all what about: if(!in_sync){ search for sync code, if found set remaining_size to the PREVIOUS packet that is up to this sync code }else if(!remaining_size){ set remaining_size based on header here } if(remaining_size){ add whats available of remaining_size into our buffer and decrease remaining_size accordingly }else add everything we have into the buffer if(!remaining_size) return packet (and yes with needed parity checks where appropriate) [...]
+/** Read a major sync info header - contains high level information about + * the stream - sample rate, channel arrangement etc. Most of this + * information is not actually necessary for decoding, only for playback. + */ + +int ff_mlp_read_major_sync(void *log, MLPHeaderInfo *mh, const uint8_t *buf, + unsigned int buf_size) +{
AVClass** log [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
Hi, On Nov 14, 2007 2:19 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
i really dont like how the parser is implemented, not at all what about:
if(!in_sync){ search for sync code, if found set remaining_size to the PREVIOUS packet that is up to this sync code }else if(!remaining_size){ set remaining_size based on header here } if(remaining_size){ add whats available of remaining_size into our buffer and decrease remaining_size accordingly }else add everything we have into the buffer if(!remaining_size) return packet
(and yes with needed parity checks where appropriate)
How would you suggest handling the case where the sync code is split across two calls to the parser? Ian
On Wed, Nov 14, 2007 at 09:06:20AM +0000, Ian Caulfield wrote:
Hi,
On Nov 14, 2007 2:19 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
i really dont like how the parser is implemented, not at all what about:
if(!in_sync){ search for sync code, if found set remaining_size to the PREVIOUS packet that is up to this sync code }else if(!remaining_size){ set remaining_size based on header here } if(remaining_size){ add whats available of remaining_size into our buffer and decrease remaining_size accordingly }else add everything we have into the buffer if(!remaining_size) return packet
(and yes with needed parity checks where appropriate)
How would you suggest handling the case where the sync code is split across two calls to the parser?
it can be split over many more than 2 and ff_combine_frame() should deal with that or did i misunderstand you and theres another problem with that? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin
Hi, On Nov 14, 2007 10:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
it can be split over many more than 2 and ff_combine_frame() should deal with that or did i misunderstand you and theres another problem with that?
The main problem I could see was that if (for whatever reason) the parser gets fed with garbage and no sync word is found, then it'll eat up memory as it stashes data that won't be used anyway... or am I being dense? Ian
On Wed, Nov 14, 2007 at 10:49:16AM +0000, Ian Caulfield wrote:
Hi,
On Nov 14, 2007 10:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
it can be split over many more than 2 and ff_combine_frame() should deal with that or did i misunderstand you and theres another problem with that?
The main problem I could see was that if (for whatever reason) the parser gets fed with garbage and no sync word is found, then it'll eat up memory as it stashes data that won't be used anyway... or am I being dense?
well add a if(size to big) return packet [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
On Nov 14, 2007 11:10 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
well add a if(size to big) return packet
Fair enough. I'm having trouble figuring out the cleanest way to implement the code to find the header size. ff_combine_frame seems to be designed for cases where packet boundaries can be easily found - not where the size of a packet is determined by data within the packet. What's the nicest way to get hold of the third and fourth bytes of the packet, handling the case where these might not be supplied together? Ian
On Wed, Nov 14, 2007 at 11:31:43AM +0000, Ian Caulfield wrote:
On Nov 14, 2007 11:10 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
well add a if(size to big) return packet
Fair enough. I'm having trouble figuring out the cleanest way to implement the code to find the header size. ff_combine_frame seems to be designed for cases where packet boundaries can be easily found - not where the size of a packet is determined by data within the packet. What's the nicest way to get hold of the third and fourth bytes of the packet, handling the case where these might not be supplied together?
they should be together after ff_combine_frame() combined them in its internal buffer [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin
On Nov 14, 2007 11:52 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Nov 14, 2007 at 11:31:43AM +0000, Ian Caulfield wrote:
On Nov 14, 2007 11:10 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
well add a if(size to big) return packet
Fair enough. I'm having trouble figuring out the cleanest way to implement the code to find the header size. ff_combine_frame seems to be designed for cases where packet boundaries can be easily found - not where the size of a packet is determined by data within the packet. What's the nicest way to get hold of the third and fourth bytes of the packet, handling the case where these might not be supplied together?
they should be together after ff_combine_frame() combined them in its internal buffer
So, should I get ff_combine_frame to read in all the data supplied to the parser, check if at least four bytes are present and read the size from there, and then call ff_combine_frame again with a negative offset if (as is likely) we did overread? Ian
On Wed, Nov 14, 2007 at 12:12:00PM +0000, Ian Caulfield wrote:
On Nov 14, 2007 11:52 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Nov 14, 2007 at 11:31:43AM +0000, Ian Caulfield wrote:
On Nov 14, 2007 11:10 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
well add a if(size to big) return packet
Fair enough. I'm having trouble figuring out the cleanest way to implement the code to find the header size. ff_combine_frame seems to be designed for cases where packet boundaries can be easily found - not where the size of a packet is determined by data within the packet. What's the nicest way to get hold of the third and fourth bytes of the packet, handling the case where these might not be supplied together?
they should be together after ff_combine_frame() combined them in its internal buffer
So, should I get ff_combine_frame to read in all the data supplied to the parser, check if at least four bytes are present and read the size
from there, and then call ff_combine_frame again with a negative offset if (as is likely) we did overread?
id only read enough in as needed for the size in the first pass also we could add a special case for the case where the input containes a whole packet or more so no memcpy is done but thats seperate and we shouldnt waste time with it until the parser is in svn [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Opposition brings concord. Out of discord comes the fairest harmony. -- Heraclitus
On Nov 14, 2007 9:05 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
id only read enough in as needed for the size in the first pass
Here's a first pass at it.
also we could add a special case for the case where the input containes a whole packet or more so no memcpy is done but thats seperate and we shouldnt waste time with it until the parser is in svn
I've left this for now. Ian
Hi On Thu, Nov 15, 2007 at 12:37:47PM +0000, Ian Caulfield wrote:
On Nov 14, 2007 9:05 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
id only read enough in as needed for the size in the first pass
Here's a first pass at it.
also we could add a special case for the case where the input containes a whole packet or more so no memcpy is done but thats seperate and we shouldnt waste time with it until the parser is in svn
I've left this for now. [...] +static int mlp_parse(AVCodecParserContext *s, + AVCodecContext *avctx, + const uint8_t **poutbuf, int *poutbuf_size, + const uint8_t *buf, int buf_size) +{ + MLPParseContext *mp = s->priv_data; + int sync_present; + uint8_t parity_bits; + int next; + int i, p; + + *poutbuf_size = 0; + if (buf_size == 0) + return 0; + + if (!mp->in_sync) { + // Not in sync - find a major sync header + + for (i = 0; i < buf_size; i++) { + mp->pc.state = (mp->pc.state << 8) | buf[i]; + if ((mp->pc.state & 0xfffffffe) == 0xf8726fba) { + mp->in_sync = 1; + mp->bytes_left = 0; + break; + } + } + + if (!mp->in_sync) { + ff_combine_frame(&mp->pc, END_NOT_FOUND, &buf, &buf_size); + return buf_size; + } + + ff_combine_frame(&mp->pc, i - 7, &buf, &buf_size); + + return FFMAX(i - 7, 0);
i think this should be return i-7 [...]
+ if ((((parity_bits >> 4) ^ parity_bits) & 0xF) != 0xF) { + av_log(NULL, AV_LOG_INFO, "mlpparse: parity check failed\n");
please provide some non null context to av_log() otherwise its impossible for an application with several decoders to make sense of the messages [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is not what we do, but why we do it that matters.
Hi On Sun, Nov 18, 2007 at 08:58:05PM +0000, Ian Caulfield wrote:
On Nov 18, 2007 4:43 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
+ + ff_combine_frame(&mp->pc, i - 7, &buf, &buf_size); + + return FFMAX(i - 7, 0);
i think this should be return i-7
Is it valid to return a negative number in a non-error case?
the code which calls it says: /* WARNING: the returned index can be negative */ index = s->parser->parser_parse(s, avctx, (const uint8_t **)poutbuf, poutbuf_size, buf, buf_size); [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes
Hi On Nov 19, 2007 1:26 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sun, Nov 18, 2007 at 08:58:05PM +0000, Ian Caulfield wrote:
On Nov 18, 2007 4:43 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
+ + ff_combine_frame(&mp->pc, i - 7, &buf, &buf_size); + + return FFMAX(i - 7, 0);
i think this should be return i-7
Is it valid to return a negative number in a non-error case?
the code which calls it says:
/* WARNING: the returned index can be negative */ index = s->parser->parser_parse(s, avctx, (const uint8_t **)poutbuf, poutbuf_size, buf, buf_size);
Fair enough, updated patch attached. Ian
On Mon, Nov 19, 2007 at 10:02:19AM +0000, Ian Caulfield wrote:
Hi
On Nov 19, 2007 1:26 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sun, Nov 18, 2007 at 08:58:05PM +0000, Ian Caulfield wrote:
On Nov 18, 2007 4:43 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
+ + ff_combine_frame(&mp->pc, i - 7, &buf, &buf_size); + + return FFMAX(i - 7, 0);
i think this should be return i-7
Is it valid to return a negative number in a non-error case?
the code which calls it says:
/* WARNING: the returned index can be negative */ index = s->parser->parser_parse(s, avctx, (const uint8_t **)poutbuf, poutbuf_size, buf, buf_size);
Fair enough, updated patch attached. [...] + for (p = 0; p < 4; p++) + parity_bits ^= buf[p]; + for (i = 0; i < mp->num_substreams; i++) { + parity_bits ^= buf[p]; + parity_bits ^= buf[p+1]; + + if (buf[p] & 0x80) { + parity_bits ^= buf[p+2]; + parity_bits ^= buf[p+3]; + p += 2; + } + p += 2;
for(i = -1; i < mp->num_substreams; i++) { parity_bits ^= buf[p++]; parity_bits ^= buf[p++]; if (i<0 || buf[p-2] & 0x80) { parity_bits ^= buf[p++]; parity_bits ^= buf[p++]; } } [...]
+#ifdef CONFIG_AUDIO_NONSHORT + avctx->bits_per_sample = mh.group1_bits; + if (avctx->bits_per_sample > 16) +// avctx->sample_fmt = SAMPLE_FMT_S32; +#endif + avctx->sample_rate = mh.group1_samplerate;
this is broken if the ifdef is true except these iam fine with the patch [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire
On Nov 23, 2007 7:27 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
[...]
+ for (p = 0; p < 4; p++) + parity_bits ^= buf[p]; + for (i = 0; i < mp->num_substreams; i++) { + parity_bits ^= buf[p]; + parity_bits ^= buf[p+1]; + + if (buf[p] & 0x80) { + parity_bits ^= buf[p+2]; + parity_bits ^= buf[p+3]; + p += 2; + } + p += 2;
for(i = -1; i < mp->num_substreams; i++) { parity_bits ^= buf[p++]; parity_bits ^= buf[p++]; if (i<0 || buf[p-2] & 0x80) { parity_bits ^= buf[p++]; parity_bits ^= buf[p++]; } }
Changed
[...]
+#ifdef CONFIG_AUDIO_NONSHORT + avctx->bits_per_sample = mh.group1_bits; + if (avctx->bits_per_sample > 16) +// avctx->sample_fmt = SAMPLE_FMT_S32; +#endif + avctx->sample_rate = mh.group1_samplerate;
this is broken if the ifdef is true
Not sure how that line ended up commented. Fixed. MLP parser patch for committal attached. Ian
On Sat, Nov 24, 2007 at 04:27:29PM +0000, Ian Caulfield wrote:
On Nov 23, 2007 7:27 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
[...]
+ for (p = 0; p < 4; p++) + parity_bits ^= buf[p]; + for (i = 0; i < mp->num_substreams; i++) { + parity_bits ^= buf[p]; + parity_bits ^= buf[p+1]; + + if (buf[p] & 0x80) { + parity_bits ^= buf[p+2]; + parity_bits ^= buf[p+3]; + p += 2; + } + p += 2;
for(i = -1; i < mp->num_substreams; i++) { parity_bits ^= buf[p++]; parity_bits ^= buf[p++]; if (i<0 || buf[p-2] & 0x80) { parity_bits ^= buf[p++]; parity_bits ^= buf[p++]; } }
Changed
[...]
+#ifdef CONFIG_AUDIO_NONSHORT + avctx->bits_per_sample = mh.group1_bits; + if (avctx->bits_per_sample > 16) +// avctx->sample_fmt = SAMPLE_FMT_S32; +#endif + avctx->sample_rate = mh.group1_samplerate;
this is broken if the ifdef is true
Not sure how that line ended up commented. Fixed.
MLP parser patch for committal attached.
looks ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
Hi! Ian Caulfield <ian.caulfield <at> gmail.com> writes:
MLP parser patch for committal attached.
This leads to mlp_parser.c: In function ?mlp_parse?: mlp_parser.c:183: warning: ?p? may be used uninitialized in this function I suggest to fix this before the patch gets committed. Carl Eugen
On Nov 25, 2007 7:29 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
Hi!
Ian Caulfield <ian.caulfield <at> gmail.com> writes:
MLP parser patch for committal attached.
This leads to mlp_parser.c: In function 'mlp_parse': mlp_parser.c:183: warning: 'p' may be used uninitialized in this function
I suggest to fix this before the patch gets committed.
Fixed. Ian
Hi! Ian Caulfield <ian.caulfield <at> gmail.com> writes:
MLP parser patch for committal attached.
This leads to mlp_parser.c: In function 'mlp_parse': mlp_parser.c:183: warning: 'p' may be used uninitialized in this function
I suggest to fix this before the patch gets committed.
Fixed.
Applied mlp parser to svn. Carl Eugen
On Nov 28, 2007 2:28 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
Applied mlp parser to svn.
Thanks! Ian
On Fri, 12 Oct 2007 13:29:46 +0100 "Ian Caulfield" <ian.caulfield at gmail.com> wrote:
Hi all,
I've been working for a while on reverse engineering the MLP audio stream format, and writing my own decoder. I've finally got to a point where I have something ready for review. I'm going to try to get the documentation I've produced on the format up on the multimediawiki as well.
Great.
I've attached the code as two patches - the first adds a parser, the second the actual decoder.
+static inline int mlp_samplerate(int in) +{ + int rate; + + if (in == 0xF) + return 0; + + if (in & 8) + rate = 44100; + else + rate = 48000; + + rate <<= in & 7; + + return rate; +}
I would simplify this: if (in == 0xF) return 0; return (in & 8 ? 44100 : 48000) << (in & 7);
+static int mlp_decode_end(AVCodecContext *avctx) +{ + return 0; +}
It does nothing, so remove it. Aurel
On Fri, Oct 12, 2007 at 01:29:46PM +0100, Ian Caulfield wrote:
Hi all,
I've been working for a while on reverse engineering the MLP audio stream format, and writing my own decoder. I've finally got to a point where I have something ready for review. I'm going to try to get the documentation I've produced on the format up on the multimediawiki as well.
I've attached the code as two patches - the first adds a parser, the second the actual decoder.
Current caveats:
- The decoder is limited by a #define to a maximum of six channels. I imagine that 7.1 streams will be more-or-less a case of changing the #defines, but I'd like to see a sample stream first to check. - Surround files are output with the channels in codec order (L, R, C, LFE, Ls, Rs for 5.1). There was some discussion on the list earlier on adding this kind of code to libavcodec instead of individual codecs, so I've left out reordering code for now. - Funky MLP stuff, like having some channels at 96KHz and some at 48KHz isn't supported - Blu-Ray TrueHD streams aren't supported. Apparently these consist of an AC3 base layer, plus a TrueHD layer with residual data or somesuch in. Anyone have any samples?
Extracting a two-channel downmix from a 5.1 stream is supported using avctx->request_channels
Comments welcome :)
Ian
[...]
+/** Get a number of bits from a bitstream, updating a running CRC in the process */ + +static av_always_inline unsigned int get_bits_crc(GetBitContext *gbp, int bit_count, uint8_t *crctab, uint8_t *crc) +{ + uint8_t crcval = *crc; + unsigned int data; + data = get_bits_long(gbp, bit_count); + while (bit_count > 8) { + crcval = crctab[crcval] ^ ((data >> (bit_count - 8)) & 0xff); + bit_count -= 8; + } + *crc = (crcval << bit_count) + ^ crctab[crcval >> (8 - bit_count)] + ^ (data & ((1 << bit_count) - 1)); + return data; +}
please use the normal get_bits*() and av_crc() once over the whole data
+
+/** Calculate an 8-bit CRC. + * The way MLP calculates CRCs is different to the standard implementation */
no the way MLP calculates CRCs is wrong, it destroys many of the good properties of crcs, for example there always exists a 2 bit error which wont be detected with the MLP design no matter how good the underlaying crc is in detecting n bit errors they could as well just have summed all bytes for checksumming
+ +static uint8_t mlp_crc8(AVCRC *crctab, uint8_t crc, uint8_t *buf, int buf_size) +{ + crc = av_crc(crctab, 0, &crc, 1); + crc = av_crc(crctab, crc, buf, buf_size - 1); + crc ^= buf[buf_size-1]; + return crc; +}
the first av_crc() call can be avoided as the initial crc value is a constant [...]
+ for (ch = m->min_channel[substr]; ch <= m->max_channel[substr]; ch++) { + m->filter_order[ch][0] = 0; + m->filter_order[ch][1] = 0; + m->filter_coeff_q[ch][0] = 0; + m->filter_coeff_q[ch][1] = 0; + + memset(m->filter_coeff[ch], 0, sizeof(m->filter_coeff[ch])); + memset(m->filter_state[ch], 0, sizeof(m->filter_state[ch])); + + /* Default audio coding is 24-bit raw PCM */ + m->huff_offset[ch] = 0; + m->codebook[ch] = 0; + m->huff_lsbs[ch] = 24; + }
there seem to be no checks for the validity of the index, if so this is BAD [...]
+/** Generate a PCM sample using the prediction filters and a residual value + * read from the data stream, and update the filter state. + */ + +static int filter_sample(MLPDecodeContext *m, int substr, + int channel, int32_t residual) +{ + int i; + int64_t accum = 0; + int32_t result; + + /* TODO: Move this code to DSPContext? */ + + for (i = 0; i < m->filter_order[channel][0]; i++) + accum += (int64_t)m->filter_state[channel][0][i] * + m->filter_coeff[channel][0][i]; + + for (i = 0; i < m->filter_order[channel][1]; i++) + accum += (int64_t) m->filter_state[channel][1][i] * + m->filter_coeff[channel][1][i];
do these really need 64bit? [...]
+ int32_t sample, filtered; + sample = read_huff(m, gbp, substr, ch); + filtered = filter_sample(m, substr, ch, sample);
declaration and initalization can be merged [...]
+ m->sample_buffer[i][maxchan+1] = ((int8_t)((seed >> 15) & 0xff)) << m->noise_shift[substr]; + m->sample_buffer[i][maxchan+2] = ((int8_t)((seed >> 7) & 0xff)) << m->noise_shift[substr];
the &0xff shouldnt be neede if you cast to int8_t [...]
+ m->noise_buffer[i] = noise_table[(seed >> 15) & 0xff]; + + tmp = seed >> 15; + seed = (seed << 8) ^ tmp ^ (tmp << 5); + seed &= 0x7fffff;
tmp = seed >> 15; m->noise_buffer[i] = noise_table[tmp]; seed = (seed << 8) ^ tmp ^ (tmp << 5); seed &= 0x7fffff; [...]
+/** + * Read an access unit from the stream. + * Returns -1 on error, 0 if not enough data is present in the input stream + * otherwise returns the number of bytes consumed. + */ + +static int read_access_unit(MLPDecodeContext *m, uint8_t *buf, int buf_size) +{ + GetBitContext gb; + int length, substr; + int substream_start; + + if (buf_size < 2) + return 0; + + if (m->out_buf_remaining < 6 * 40 * 4) + return 0;
isnt that an error? also are you sure that no more than 6 * 40 * 4 can be written? [...]
+ for (substr = 0; substr <= m->max_decoded_substream; substr++) { + init_get_bits(&gb, buf, m->substream_data_len[substr] * 16); + + m->blockpos[substr] = 0; + do { + if (get_bits1(&gb)) { + if (get_bits1(&gb)) { + /* A restart header should be present */ + if (read_restart_header(m, &gb, substr) < 0) + goto error; + if (!m->restart_header_present[substr]) + av_log(m->avctx, AV_LOG_INFO, "Restart header present but not indicated in block header.\n"); + m->restart_seen[substr] = 1; + m->restart_header_present[substr] = 0; + } else if (m->restart_header_present[substr]) { + av_log(m->avctx, AV_LOG_INFO, "Restart header indicated in block header but not present.\n"); + if (!m->restart_seen[substr]) + goto error; + } + + if (read_decoding_params(m, &gb, substr) < 0) + goto error; + } + + if (read_block_data(m, &gb, substr) < 0) + goto error; + + m->blockpos[substr] += m->blocksize[substr];
is there anything which limits the size of blockpos, it seems used in many loops and would lead to exploitable bugs if it can get larger than the arrays also if its not checked anywhere please recheck everything for similar bugs! [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The greatest way to live with honor in this world is to be what we pretend to be. -- Socrates
On 14/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
+/** Get a number of bits from a bitstream, updating a running CRC in the process */ + +static av_always_inline unsigned int get_bits_crc(GetBitContext *gbp, int bit_count, uint8_t *crctab, uint8_t *crc) +{ + uint8_t crcval = *crc; + unsigned int data; + data = get_bits_long(gbp, bit_count); + while (bit_count > 8) { + crcval = crctab[crcval] ^ ((data >> (bit_count - 8)) & 0xff); + bit_count -= 8; + } + *crc = (crcval << bit_count) + ^ crctab[crcval >> (8 - bit_count)] + ^ (data & ((1 << bit_count) - 1)); + return data; +}
please use the normal get_bits*() and av_crc() once over the whole data
Is it possible to use av_crc on fractional bytes? Ian
Hi On Mon, Oct 15, 2007 at 12:42:25PM +0100, Ian Caulfield wrote:
On 14/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
+/** Get a number of bits from a bitstream, updating a running CRC in the process */ + +static av_always_inline unsigned int get_bits_crc(GetBitContext *gbp, int bit_count, uint8_t *crctab, uint8_t *crc) +{ + uint8_t crcval = *crc; + unsigned int data; + data = get_bits_long(gbp, bit_count); + while (bit_count > 8) { + crcval = crctab[crcval] ^ ((data >> (bit_count - 8)) & 0xff); + bit_count -= 8; + } + *crc = (crcval << bit_count) + ^ crctab[crcval >> (8 - bit_count)] + ^ (data & ((1 << bit_count) - 1)); + return data; +}
please use the normal get_bits*() and av_crc() once over the whole data
Is it possible to use av_crc on fractional bytes?
is the area over which the crc is calculated not a multiple of 8bits ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates
On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
Hi
On Mon, Oct 15, 2007 at 12:42:25PM +0100, Ian Caulfield wrote:
On 14/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
+/** Get a number of bits from a bitstream, updating a running CRC in the process */ + +static av_always_inline unsigned int get_bits_crc(GetBitContext *gbp, int bit_count, uint8_t *crctab, uint8_t *crc) +{ + uint8_t crcval = *crc; + unsigned int data; + data = get_bits_long(gbp, bit_count); + while (bit_count > 8) { + crcval = crctab[crcval] ^ ((data >> (bit_count - 8)) & 0xff); + bit_count -= 8; + } + *crc = (crcval << bit_count) + ^ crctab[crcval >> (8 - bit_count)] + ^ (data & ((1 << bit_count) - 1)); + return data; +}
please use the normal get_bits*() and av_crc() once over the whole data
Is it possible to use av_crc on fractional bytes?
is the area over which the crc is calculated not a multiple of 8bits ?
Not in this case - this header is usually 125 or 149 bits long, and doesn't start on a byte boundary. What do you suggest I call the not-CRC that MLP uses (the difference being that the order of the table lookup and XOR are swapped)? Ian
On Mon, Oct 15, 2007 at 05:11:23PM +0100, Ian Caulfield wrote:
On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
Hi
On Mon, Oct 15, 2007 at 12:42:25PM +0100, Ian Caulfield wrote:
On 14/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
+/** Get a number of bits from a bitstream, updating a running CRC in the process */ + +static av_always_inline unsigned int get_bits_crc(GetBitContext *gbp, int bit_count, uint8_t *crctab, uint8_t *crc) +{ + uint8_t crcval = *crc; + unsigned int data; + data = get_bits_long(gbp, bit_count); + while (bit_count > 8) { + crcval = crctab[crcval] ^ ((data >> (bit_count - 8)) & 0xff); + bit_count -= 8; + } + *crc = (crcval << bit_count) + ^ crctab[crcval >> (8 - bit_count)] + ^ (data & ((1 << bit_count) - 1)); + return data; +}
please use the normal get_bits*() and av_crc() once over the whole data
Is it possible to use av_crc on fractional bytes?
is the area over which the crc is calculated not a multiple of 8bits ?
Not in this case - this header is usually 125 or 149 bits long, and doesn't start on a byte boundary.
just "filling" the first bits which arent part of the header with 0 should work as long as the end is on a byte boundary
What do you suggest I call the not-CRC that MLP uses (the difference being that the order of the table lookup and XOR are swapped)?
MLP-checksum maybe also its all fine to call it crc as long as it is one, that is before that funky xor also my complaint was only about that comment claiming that the used checksum would be a a crc just different from what libavutil implements [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Oct 15, 2007 at 05:11:23PM +0100, Ian Caulfield wrote:
On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
Hi
On Mon, Oct 15, 2007 at 12:42:25PM +0100, Ian Caulfield wrote:
Is it possible to use av_crc on fractional bytes?
is the area over which the crc is calculated not a multiple of 8bits ?
Not in this case - this header is usually 125 or 149 bits long, and doesn't start on a byte boundary.
just "filling" the first bits which arent part of the header with 0 should work as long as the end is on a byte boundary
I don't think the end is on a byte boundary either...
What do you suggest I call the not-CRC that MLP uses (the difference being that the order of the table lookup and XOR are swapped)?
MLP-checksum maybe also its all fine to call it crc as long as it is one, that is before that funky xor also my complaint was only about that comment claiming that the used checksum would be a a crc just different from what libavutil implements
I'm not so hot at Galois arithmetic and stuff - I assumed it was just a CRC variant. I'll work on the comments. Ian
Hi On Mon, Oct 15, 2007 at 06:58:42PM +0100, Ian Caulfield wrote:
On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Oct 15, 2007 at 05:11:23PM +0100, Ian Caulfield wrote:
On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
Hi
On Mon, Oct 15, 2007 at 12:42:25PM +0100, Ian Caulfield wrote:
Is it possible to use av_crc on fractional bytes?
is the area over which the crc is calculated not a multiple of 8bits ?
Not in this case - this header is usually 125 or 149 bits long, and doesn't start on a byte boundary.
just "filling" the first bits which arent part of the header with 0 should work as long as the end is on a byte boundary
I don't think the end is on a byte boundary either...
:( well if its just fields of the header then maybe the code could be simplified (without a table) does that affect speed of the whole codec meassureably? i think the table wouldnt be in the cache anyway and the cache misses for each header would kill the gain from the table... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I wish the Xiph folks would stop pretending they've got something they do not. Somehow I fear this will remain a wish. -- M?ns Rullg?rd
Updated decoder patch attached. On 14/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
On Fri, Oct 12, 2007 at 01:29:46PM +0100, Ian Caulfield wrote:
please use the normal get_bits*() and av_crc() once over the whole data
Normal get_bits functions used - a checksum function has been added to do an av_crc() over the full bytes of data, and then process the last few bits bit-by-bit.
+
+/** Calculate an 8-bit CRC. + * The way MLP calculates CRCs is different to the standard implementation */
no the way MLP calculates CRCs is wrong, it destroys many of the good properties of crcs, for example there always exists a 2 bit error which wont be detected with the MLP design no matter how good the underlaying crc is in detecting n bit errors they could as well just have summed all bytes for checksumming
Renamed to checksum
+ +static uint8_t mlp_crc8(AVCRC *crctab, uint8_t crc, uint8_t *buf, int buf_size) +{ + crc = av_crc(crctab, 0, &crc, 1); + crc = av_crc(crctab, crc, buf, buf_size - 1); + crc ^= buf[buf_size-1]; + return crc; +}
the first av_crc() call can be avoided as the initial crc value is a constant
Fixed
[...]
+ for (ch = m->min_channel[substr]; ch <= m->max_channel[substr]; ch++) { + m->filter_order[ch][0] = 0; + m->filter_order[ch][1] = 0; + m->filter_coeff_q[ch][0] = 0; + m->filter_coeff_q[ch][1] = 0; + + memset(m->filter_coeff[ch], 0, sizeof(m->filter_coeff[ch])); + memset(m->filter_state[ch], 0, sizeof(m->filter_state[ch])); + + /* Default audio coding is 24-bit raw PCM */ + m->huff_offset[ch] = 0; + m->codebook[ch] = 0; + m->huff_lsbs[ch] = 24; + }
there seem to be no checks for the validity of the index, if so this is BAD
Checks added for the various min/max channel parameters.
[...]
+/** Generate a PCM sample using the prediction filters and a residual value + * read from the data stream, and update the filter state. + */ + +static int filter_sample(MLPDecodeContext *m, int substr, + int channel, int32_t residual) +{ + int i; + int64_t accum = 0; + int32_t result; + + /* TODO: Move this code to DSPContext? */ + + for (i = 0; i < m->filter_order[channel][0]; i++) + accum += (int64_t)m->filter_state[channel][0][i] * + m->filter_coeff[channel][0][i]; + + for (i = 0; i < m->filter_order[channel][1]; i++) + accum += (int64_t) m->filter_state[channel][1][i] * + m->filter_coeff[channel][1][i];
do these really need 64bit?
filter_state is up to 24-bit, filter_coeff is 16-bit, so output is theoretically 40-bits. In practice it should be possible to use a 32-bit version for some blocks - I haven't addressed this yet, but it could be a future optimisation.
[...]
+ int32_t sample, filtered; + sample = read_huff(m, gbp, substr, ch); + filtered = filter_sample(m, substr, ch, sample);
declaration and initalization can be merged
Fixed
[...]
+ m->sample_buffer[i][maxchan+1] = ((int8_t)((seed >> 15) & 0xff)) << m->noise_shift[substr]; + m->sample_buffer[i][maxchan+2] = ((int8_t)((seed >> 7) & 0xff)) << m->noise_shift[substr];
the &0xff shouldnt be neede if you cast to int8_t
Fixed
[...]
+ m->noise_buffer[i] = noise_table[(seed >> 15) & 0xff]; + + tmp = seed >> 15; + seed = (seed << 8) ^ tmp ^ (tmp << 5); + seed &= 0x7fffff;
tmp = seed >> 15; m->noise_buffer[i] = noise_table[tmp]; seed = (seed << 8) ^ tmp ^ (tmp << 5); seed &= 0x7fffff;
Changed
[...]
+/** + * Read an access unit from the stream. + * Returns -1 on error, 0 if not enough data is present in the input stream + * otherwise returns the number of bytes consumed. + */ + +static int read_access_unit(MLPDecodeContext *m, uint8_t *buf, int buf_size) +{ + GetBitContext gb; + int length, substr; + int substream_start; + + if (buf_size < 2) + return 0; + + if (m->out_buf_remaining < 6 * 40 * 4) + return 0;
isnt that an error? also are you sure that no more than 6 * 40 * 4 can be written?
I've changed this to check against the actual amount that will be written, and to return an error.
is there anything which limits the size of blockpos, it seems used in many loops and would lead to exploitable bugs if it can get larger than the arrays also if its not checked anywhere please recheck everything for similar bugs!
I've added a check before any data is written to make sure that the pos doesn't go beyond the amount of audio data that should be generated by that block. Ian
Minor bumpage... Aside from Diego's comment about the header inclusion defines, are these patches OK? Ian
On Wed, Oct 17, 2007 at 04:37:16PM +0100, Ian Caulfield wrote:
Updated decoder patch attached. [...]
+ int out_buf_remaining;
this variable is useless, its used only at the top of read_access_unit() and only set before the call to read_access_unit(), check the buffer before calling a functon filling it, not storing the buffer size in some context and doing the check in the function! [...]
+ uint8_t restart_header_present[MAX_SUBSTREAMS];
this one is used in various places but it looks like the code using this does nothing besides printing useless debug messages [...]
+/** Calculate an 8-bit checksum over a restart header -- a non-multiple-of-8 + * number of bits, starting two bits into the first byte of buf. + */ + +static uint8_t mlp_restart_checksum(const uint8_t *buf, unsigned int bit_size) +{ + int i; + int num_bytes = (bit_size + 2) / 8; + + uint8_t crc = crc_1D[buf[0] & 0x3f]; + crc = av_crc(crc_1D, crc, buf + 1, num_bytes - 2); + crc ^= buf[num_bytes - 1]; + + for (i = 0; i < (bit_size + 2) % 8; i++) { + if (crc & 0x80) + crc = (crc << 1) ^ 0x1D; + else + crc = crc << 1;
crc<<=1; if(crc&0x100) crc ^= 0x11D; and sizeof(crc) > 1 [...]
+/** Read a sample, consisting of either, both or neither of entropy-coded MSBs + * and plain LSBs + */ + +static inline int read_huff(MLPDecodeContext *m, GetBitContext *gbp, + unsigned int substr, unsigned int channel) +{ + int codebook = m->codebook[channel]; + int quant_step_size = m->quant_step_size[substr][channel]; + int lsb_bits = m->huff_lsbs[channel] - quant_step_size; + int sign_shift; + int msbs = 0, lsbs = 0; + int result; + + if (codebook > 0) + msbs = get_vlc2(gbp, huff_vlc[codebook-1].table, + VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7; + + if (lsb_bits > 0) + lsbs = get_bits(gbp, lsb_bits); + + result = (msbs << lsb_bits) + lsbs; + sign_shift = lsb_bits + (codebook ? 2 - codebook : -1); + + if (sign_shift >= 0) + result += (-1) << sign_shift; + + result += m->huff_offset[channel]; + + return result << quant_step_size;
int result=0; if (codebook > 0) result = get_vlc2(gbp, huff_vlc[codebook-1].table, VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7; if(lsb_bits > 0) result = (result<<lsb_bits) + get_bits(gbp, lsb_bits); return (result + m->huff_offset[channel]) << quant_step_size; note, yes you can merge the sign_shift stuff into huff_offset [...]
+ m->stream_type = mh.stream_type; + + if (m->stream_type == 0xbb) {
as this is the only use of the variable, this doesnt need to be in the context [...]
+ if (mh.group2_samplerate != 0 && mh.group2_samplerate != mh.group1_samplerate) {
the != 0 is redundant [...]
+ if (m->num_substreams > MAX_SUBSTREAMS) + av_log(m->avctx, AV_LOG_INFO, + "Number of substreams %d is more than maximum supported by " + "decoder. Only the first %d will be decoded. Please provide a " + "sample of this file to the FFmpeg development list.\n", + m->num_substreams, MAX_SUBSTREAMS);
this should do a return -1 after the message, its much more effective at convincing users to send us a sample ... this would also avoid the FFMIN below and possibly the max_decoded_substream variable as well?
+ + m->max_decoded_substream = FFMIN(m->num_substreams, MAX_SUBSTREAMS) - 1;
just asking (id have to check the other patch to know for sure) ... num_substreams cannot be 0 right? because if it can then this is not good at all [...]
+ for (substr = 0; substr < m->num_substreams; substr++) + m->restart_seen[substr] = 0;
memset() [...]
+ if (m->max_channel[substr] >= MAX_CHANNELS + || m->max_matrix_channel[substr] >= MAX_CHANNELS) + { + if (substr > 0) { + av_log(m->avctx, AV_LOG_INFO, + "Substream %d contains more channels than the maximum " + "supported by this decoder (%d). Only substreams up to %d " + "will be decoded. Please provide a sample of this file " + "to the FFmpeg development list.\n", + substr, MAX_CHANNELS, substr - 1); + m->max_decoded_substream = substr - 1; + m->avctx->channels = m->max_channel[substr - 1] + 1; + } else { + av_log(m->avctx, AV_LOG_INFO, + "This stream contains more channels than the maximum " + "supported by this decoder (%d). Please provide a sample " + "of this file to the FFmpeg development list.\n", + MAX_CHANNELS); + return -1; + } + }
so after this max_channel and max_matrix_channel are invalid, is there anything that stops them from being used? if no then this check is as good as no check at all you could as well print "please reformat your harddisk and reinstall everything your system has been hacked into" also if i really didnt miss something and this can lead to writing file data over the end of arrays then please recheck your code, yes all of it for similar issues [...]
+ m->max_lsbs[substr] = get_bits(gbp, 5); + m->max_bits1[substr] = get_bits(gbp, 5); + m->max_bits2[substr] = get_bits(gbp, 5); + av_log(m->avctx, AV_LOG_DEBUG, "max_lsbs %d max_bits1 %d max_bits2 %d\n", + m->max_lsbs[substr], m->max_bits1[substr], m->max_bits2[substr]);
these 3 variables are not used anywhere else, they dont belong into the context! [...]
+ for (ch = 0; ch <= m->max_matrix_channel[substr]; ch++) { + m->ch_assign[substr][ch] = get_bits(gbp, 6); + av_log(m->avctx, AV_LOG_DEBUG, "ch_assign[%d][%d] = %d\n", + substr, ch, m->ch_assign[substr][ch]); + }
this is not used anywhere [...]
+ for (i = 0; i < order; i++) { + m->filter_coeff[channel][filter][i] = + get_sbits(gbp, coeff_bits) << coeff_shift; + av_log(m->avctx, AV_LOG_DEBUG, " coefficient %f\n", + ((float)m->filter_coeff[channel][filter][i]) + / (1 << m->filter_coeff_q[channel][filter]));
i think dprintf() would be better for these as it would not waste cpu time or memory if debuging is not wanted the same applies to many other av_log() [...]
+ uint32_t tmp; + uint32_t seed = m->noisegen_seed[substr]; + + for (i = 0; i < m->access_unit_size_pow2; i++) { + tmp = seed >> 15;
uint32_t tmp= seed >> 15; [...]
+/** Write the audio data into the output buffer. + */ + +static int output_data(MLPDecodeContext *m, unsigned int substr) +{ + unsigned int i, ch; + int32_t sample; + int16_t *sample_buf16 = (int16_t*)(m->out_buf + m->bytes_output); + +#ifdef CONFIG_AUDIO_NONSHORT + int32_t *sample_buf32 = (int32_t*)(m->out_buf + m->bytes_output); + + if (m->avctx->sample_fmt == SAMPLE_FMT_S32) { + for (i = 0; i < m->blockpos[substr]; i++) { + for (ch = 0; ch <= m->max_channel[substr]; ch++) { + sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; + m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; + *sample_buf32++ = sample << 8; + m->bytes_output += 4; + } + } + } else +#endif + { + for (i = 0; i < m->blockpos[substr]; i++) { + for (ch = 0; ch <= m->max_channel[substr]; ch++) { + sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; + m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; + *sample_buf16++ = (sample) >> 8; + m->bytes_output += 2; + } + } + } + + return 0; +}
static inline void output_data_internal(MLPDecodeContext *m, unsigned int substr, int is32) { unsigned int i, ch; for (i = 0; i < m->blockpos[substr]; i++) { for (ch = 0; ch <= m->max_channel[substr]; ch++) { int sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; if(is32) *((int32_t*)m->out_ptr)++ = sample << 8; else *((int16_t*)m->out_ptr)++ = sample >> 8; } } } static void output_data_internal(MLPDecodeContext *m, unsigned int substr) { if(m->avctx->sample_fmt == SAMPLE_FMT_S32) output_data_internal(m, substr, 1); else output_data_internal(m, substr, 0); } [...]
+ while (buf_size >= 4) { + scratch ^= *((const uint32_t*)buf); + buf += 4; + buf_size -= 4; + }
for(; buf_size >= 4; buf_size -= 4){ } or for(i=0; i < buf_size/4; i++) scratch ^= ((const uint32_t*)buf)[i]; [...]
+ if ((show_bits_long(&gb, 31) << 1) == 0xf8726fba) {
hmmmm the <<1 is useless, shift your constant! [...]
+ if (!m->in_sync) + return length * 2;
please explain this, didnt your parser already drop all "not in sync" parts? [...]
+ extraword_present = get_bits1(&gb); + restart_absent = get_bits1(&gb); + checkdata_present = get_bits1(&gb);
the get_bits1 can be vertically aligned [...]
+ } while ((get_bits_count(&gb) < m->substream_data_len[substr] * 16) + && get_bits1(&gb) == 0);
substream_data_len seems to be only used in this function so it doesnt belong into the context [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data
On Sat, 3 Nov 2007 00:49:21 +0100 Michael Niedermayer <michaelni at gmx.at> wrote:
+/** Write the audio data into the output buffer. + */ + +static int output_data(MLPDecodeContext *m, unsigned int substr) +{ + unsigned int i, ch; + int32_t sample; + int16_t *sample_buf16 = (int16_t*)(m->out_buf + m->bytes_output); + +#ifdef CONFIG_AUDIO_NONSHORT + int32_t *sample_buf32 = (int32_t*)(m->out_buf + m->bytes_output); + + if (m->avctx->sample_fmt == SAMPLE_FMT_S32) { + for (i = 0; i < m->blockpos[substr]; i++) { + for (ch = 0; ch <= m->max_channel[substr]; ch++) { + sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; + m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; + *sample_buf32++ = sample << 8; + m->bytes_output += 4; + } + } + } else +#endif + { + for (i = 0; i < m->blockpos[substr]; i++) { + for (ch = 0; ch <= m->max_channel[substr]; ch++) { + sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; + m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; + *sample_buf16++ = (sample) >> 8; + m->bytes_output += 2; + } + } + } + + return 0; +}
static inline void output_data_internal(MLPDecodeContext *m, unsigned int substr, int is32) { unsigned int i, ch; for (i = 0; i < m->blockpos[substr]; i++) { for (ch = 0; ch <= m->max_channel[substr]; ch++) { int sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; if(is32) *((int32_t*)m->out_ptr)++ = sample << 8; else *((int16_t*)m->out_ptr)++ = sample >> 8; } } }
static void output_data_internal(MLPDecodeContext *m, unsigned int substr) { if(m->avctx->sample_fmt == SAMPLE_FMT_S32) output_data_internal(m, substr, 1); else output_data_internal(m, substr, 0); }
I'm astonished you didn't proposed directly this simpler form: static void output_data(MLPDecodeContext *m, unsigned int substr) { output_data_internal(m, substr, m->avctx->sample_fmt == SAMPLE_FMT_S32); } Aurel
Hi On Sat, Nov 03, 2007 at 01:02:51AM +0100, Aurelien Jacobs wrote:
On Sat, 3 Nov 2007 00:49:21 +0100 Michael Niedermayer <michaelni at gmx.at> wrote:
+/** Write the audio data into the output buffer. + */ + +static int output_data(MLPDecodeContext *m, unsigned int substr) +{ + unsigned int i, ch; + int32_t sample; + int16_t *sample_buf16 = (int16_t*)(m->out_buf + m->bytes_output); + +#ifdef CONFIG_AUDIO_NONSHORT + int32_t *sample_buf32 = (int32_t*)(m->out_buf + m->bytes_output); + + if (m->avctx->sample_fmt == SAMPLE_FMT_S32) { + for (i = 0; i < m->blockpos[substr]; i++) { + for (ch = 0; ch <= m->max_channel[substr]; ch++) { + sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; + m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; + *sample_buf32++ = sample << 8; + m->bytes_output += 4; + } + } + } else +#endif + { + for (i = 0; i < m->blockpos[substr]; i++) { + for (ch = 0; ch <= m->max_channel[substr]; ch++) { + sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; + m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; + *sample_buf16++ = (sample) >> 8; + m->bytes_output += 2; + } + } + } + + return 0; +}
static inline void output_data_internal(MLPDecodeContext *m, unsigned int substr, int is32) { unsigned int i, ch; for (i = 0; i < m->blockpos[substr]; i++) { for (ch = 0; ch <= m->max_channel[substr]; ch++) { int sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch]; m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch; if(is32) *((int32_t*)m->out_ptr)++ = sample << 8; else *((int16_t*)m->out_ptr)++ = sample >> 8; } } }
static void output_data_internal(MLPDecodeContext *m, unsigned int substr) { if(m->avctx->sample_fmt == SAMPLE_FMT_S32) output_data_internal(m, substr, 1); else output_data_internal(m, substr, 0); }
I'm astonished you didn't proposed directly this simpler form:
static void output_data(MLPDecodeContext *m, unsigned int substr) { output_data_internal(m, substr, m->avctx->sample_fmt == SAMPLE_FMT_S32); }
thats because we live in a world without a well written free compiler gcc can optimize the check out of the inner loop with my suggestion but not with yours, at least not last time i looked at what gcc generats for such code [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato
[...]
+ if (m->max_channel[substr] >= MAX_CHANNELS + || m->max_matrix_channel[substr] >= MAX_CHANNELS) + { + if (substr > 0) {
+ av_log(m->avctx, AV_LOG_INFO, + "Substream %d contains more channels than the maximum " + "supported by this decoder (%d). Only substreams up to %d " + "will be decoded. Please provide a sample of this file " + "to the FFmpeg development list.\n", + substr, MAX_CHANNELS, substr - 1); + m->max_decoded_substream = substr - 1; + m->avctx->channels = m->max_channel[substr - 1] + 1; + } else {
+ av_log(m->avctx, AV_LOG_INFO, + "This stream contains more channels than the maximum " + "supported by this decoder (%d). Please provide a sample " + "of this file to the FFmpeg development list.\n", + MAX_CHANNELS); + return -1; + } + }
so after this max_channel and max_matrix_channel are invalid, is there anything that stops them from being used? if no then this check is as good as no check at all
The first case sets max_decoded_substream so that these parameters won't be used, while the second case returns an error - however I seem to have missed resetting the in_sync variable. Will fix.
also if i really didnt miss something and this can lead to writing file data over the end of arrays then please recheck your code, yes all of it for similar issues
Will do.
[...]
+ for (ch = 0; ch <= m->max_matrix_channel[substr]; ch++) { + m->ch_assign[substr][ch] = get_bits(gbp, 6); + av_log(m->avctx, AV_LOG_DEBUG, "ch_assign[%d][%d] = %d\n", + substr, ch, m->ch_assign[substr][ch]); + }
this is not used anywhere
Not yet - I've yet to find a stream that uses these fields in a non-1:1 fashion, so I'm not 100% sure on their usage. I'll add some checking in to fault if ch_assign[ch] != ch.
[...]
+ if ((show_bits_long(&gb, 31) << 1) == 0xf8726fba) {
hmmmm the <<1 is useless, shift your constant!
The original reasoning was that we're looking at only the top 31 bits of a 32-bit value, and shifting the constant would confuse what that value was. Is (0xf8726fba >> 1) acceptable, or should I use a comment?
[...]
+ if (!m->in_sync) + return length * 2;
please explain this, didnt your parser already drop all "not in sync" parts?
in_sync is probably a bad name - the parser does drop frames when it's not in sync. However, the sync frames contain decoding parameters needed for all packets until the next sync frame, so if there's an error decoding the sync frame, the decoder can't decode any further packets until another one comes along, regardless of whether the parser is synced up. Also, is the parser patch OK to commit? Ian
On Nov 8, 2007 1:33 PM, Ian Caulfield <ian.caulfield at gmail.com> wrote:
[...]
+ if (m->max_channel[substr] >= MAX_CHANNELS + || m->max_matrix_channel[substr] >= MAX_CHANNELS) + { + if (substr > 0) {
+ av_log(m->avctx, AV_LOG_INFO, + "Substream %d contains more channels than the maximum " + "supported by this decoder (%d). Only substreams up to %d " + "will be decoded. Please provide a sample of this file " + "to the FFmpeg development list.\n", + substr, MAX_CHANNELS, substr - 1); + m->max_decoded_substream = substr - 1; + m->avctx->channels = m->max_channel[substr - 1] + 1; + } else {
+ av_log(m->avctx, AV_LOG_INFO, + "This stream contains more channels than the maximum " + "supported by this decoder (%d). Please provide a sample " + "of this file to the FFmpeg development list.\n", + MAX_CHANNELS); + return -1; + } + }
so after this max_channel and max_matrix_channel are invalid, is there anything that stops them from being used? if no then this check is as good as no check at all
The first case sets max_decoded_substream so that these parameters won't be used, while the second case returns an error - however I seem to have missed resetting the in_sync variable. Will fix.
Actually, any error raised by that function causes in_sync to be reset by the calling function, so it should be OK... Ian
Hi On Thu, Nov 08, 2007 at 01:33:46PM +0000, Ian Caulfield wrote:
[...]
+ if (m->max_channel[substr] >= MAX_CHANNELS + || m->max_matrix_channel[substr] >= MAX_CHANNELS) + { + if (substr > 0) {
+ av_log(m->avctx, AV_LOG_INFO, + "Substream %d contains more channels than the maximum " + "supported by this decoder (%d). Only substreams up to %d " + "will be decoded. Please provide a sample of this file " + "to the FFmpeg development list.\n", + substr, MAX_CHANNELS, substr - 1); + m->max_decoded_substream = substr - 1; + m->avctx->channels = m->max_channel[substr - 1] + 1; + } else {
+ av_log(m->avctx, AV_LOG_INFO, + "This stream contains more channels than the maximum " + "supported by this decoder (%d). Please provide a sample " + "of this file to the FFmpeg development list.\n", + MAX_CHANNELS); + return -1; + } + }
so after this max_channel and max_matrix_channel are invalid, is there anything that stops them from being used? if no then this check is as good as no check at all
The first case sets max_decoded_substream so that these parameters won't be used, while the second case returns an error - however I seem
i dont see how returing an error will prevent the use of the invalid value in future frames leaving variables in the context which are used to prevent array overflows at invalid values is fragile, what if the code is changed slightly in the future and they do become used? (that is assuming they wouldnt be used currently already in some cases) [...]
[...]
+ if ((show_bits_long(&gb, 31) << 1) == 0xf8726fba) {
hmmmm the <<1 is useless, shift your constant!
The original reasoning was that we're looking at only the top 31 bits of a 32-bit value, and shifting the constant would confuse what that value was. Is (0xf8726fba >> 1) acceptable, or should I use a comment?
(0xf8726fba >> 1) is ok
[...]
+ if (!m->in_sync) + return length * 2;
please explain this, didnt your parser already drop all "not in sync" parts?
in_sync is probably a bad name - the parser does drop frames when it's not in sync. However, the sync frames contain decoding parameters needed for all packets until the next sync frame, so if there's an error decoding the sync frame, the decoder can't decode any further packets until another one comes along, regardless of whether the parser is synced up.
do the parameters change between syncs in practice? or would using the parameters of the last undamaged sync work as well? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
Hi
On Thu, Nov 08, 2007 at 01:33:46PM +0000, Ian Caulfield wrote:
so after this max_channel and max_matrix_channel are invalid, is there anything that stops them from being used? if no then this check is as good as no check at all
The first case sets max_decoded_substream so that these parameters won't be used, while the second case returns an error - however I seem
i dont see how returing an error will prevent the use of the invalid value in future frames leaving variables in the context which are used to prevent array overflows at invalid values is fragile, what if the code is changed slightly in the future and they do become used? (that is assuming they wouldnt be used currently already in some cases)
As well as an error being returned, the "have parameters" flag in the context is cleared, causing all packets to be rejected until we get a (valid) sync packet.
[...]
+ if (!m->in_sync) + return length * 2;
please explain this, didnt your parser already drop all "not in sync" parts?
in_sync is probably a bad name - the parser does drop frames when it's not in sync. However, the sync frames contain decoding parameters needed for all packets until the next sync frame, so if there's an error decoding the sync frame, the decoder can't decode any further packets until another one comes along, regardless of whether the parser is synced up.
do the parameters change between syncs in practice? or would using the parameters of the last undamaged sync work as well?
Unfortunately so - the sync packets contain not just things like "6 channels, 48KHz", but also encoding parameters which change throughout the stream to improve compression - an earlier version of the decoder had a bug where one of the parameters wasn't reset properly and it sounded awful. I think trying to fudge by on the last set of parameters is likely to produce nasty output... New version of the patch attached. Ian
On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
New version of the patch attached.
build system part OK
--- libavcodec/mlpdec.c (revision 0) +++ libavcodec/mlpdec.c (revision 0) @@ -0,0 +1,1116 @@ + +/** Maximum number of channels that can be decoded. It should be safe to + * increase this value, but I haven't got any test streams with more than + * 5.1 channels */
Please end complete sentences in a period, same many times below. Diego
On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
Hi
On Thu, Nov 08, 2007 at 01:33:46PM +0000, Ian Caulfield wrote:
so after this max_channel and max_matrix_channel are invalid, is there anything that stops them from being used? if no then this check is as good as no check at all
The first case sets max_decoded_substream so that these parameters won't be used, while the second case returns an error - however I seem
i dont see how returing an error will prevent the use of the invalid value in future frames leaving variables in the context which are used to prevent array overflows at invalid values is fragile, what if the code is changed slightly in the future and they do become used? (that is assuming they wouldnt be used currently already in some cases)
As well as an error being returned, the "have parameters" flag in the context is cleared, causing all packets to be rejected until we get a (valid) sync packet.
there is no variable, constant or otherwise called "have parameters" or have*
[...]
+ if (!m->in_sync) + return length * 2;
please explain this, didnt your parser already drop all "not in sync" parts?
in_sync is probably a bad name - the parser does drop frames when it's not in sync. However, the sync frames contain decoding parameters needed for all packets until the next sync frame, so if there's an error decoding the sync frame, the decoder can't decode any further packets until another one comes along, regardless of whether the parser is synced up.
do the parameters change between syncs in practice? or would using the parameters of the last undamaged sync work as well?
Unfortunately so - the sync packets contain not just things like "6 channels, 48KHz", but also encoding parameters which change throughout the stream to improve compression - an earlier version of the decoder had a bug where one of the parameters wasn't reset properly and it sounded awful. I think trying to fudge by on the last set of parameters is likely to produce nasty output...
well, but some sound will have to be placed where decoding fails ... from the outside of the decoder we are limited to things like putting silence there or repeating the last valid output does this sound better? [...]
+ int out_buf_remaining;
see previous review [...]
+ uint8_t restart_header_present[MAX_SUBSTREAMS];
see previous review [...]
+static uint8_t mlp_restart_checksum(const uint8_t *buf, unsigned int bit_size) +{ + int i; + int num_bytes = (bit_size + 2) / 8; + + uint8_t crc = crc_1D[buf[0] & 0x3f]; + crc = av_crc(crc_1D, crc, buf + 1, num_bytes - 2); + crc ^= buf[num_bytes - 1]; + + for (i = 0; i < (bit_size + 2) % 8; i++) { + if (crc & 0x80) + crc = (crc << 1) ^ 0x1D; + else + crc = crc << 1;
see previous review remainder not reviewed [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
It appears I attached an older version of the patch in my last email, sorry. I've attached an up-to-date patch (hopefully) addressing Diego's comments. On Dec 1, 2007 10:08 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
As well as an error being returned, the "have parameters" flag in the context is cleared, causing all packets to be rejected until we get a (valid) sync packet.
there is no variable, constant or otherwise called "have parameters" or have*
The variable "has_params" is used (previously called in_sync).
do the parameters change between syncs in practice? or would using the parameters of the last undamaged sync work as well?
Unfortunately so - the sync packets contain not just things like "6 channels, 48KHz", but also encoding parameters which change throughout the stream to improve compression - an earlier version of the decoder had a bug where one of the parameters wasn't reset properly and it sounded awful. I think trying to fudge by on the last set of parameters is likely to produce nasty output...
well, but some sound will have to be placed where decoding fails ... from the outside of the decoder we are limited to things like putting silence there or repeating the last valid output does this sound better?
I don't know offhand, but data like the state for the prediction filters is conveyed each sync frame - if this goes badly wrong, I suspect the output will be little better than random data. The MLP scheme is designed for lossless media, so the error resilience isn't great. I'm not an expert on audio error recovery/concealment though. Ian
On Sat, Dec 01, 2007 at 11:49:17PM +0000, Ian Caulfield wrote:
It appears I attached an older version of the patch in my last email, sorry. I've attached an up-to-date patch (hopefully) addressing Diego's comments.
On Dec 1, 2007 10:08 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
As well as an error being returned, the "have parameters" flag in the context is cleared, causing all packets to be rejected until we get a (valid) sync packet.
there is no variable, constant or otherwise called "have parameters" or have*
The variable "has_params" is used (previously called in_sync).
rather call it params_valid please because if its 0 they are not, also this design is fragile, every future change to the codec would have to be checked that it doesnt use anything from the context if has_params != 1 a memset(0) of the whole context is minimum if you want to keep this design and want to see it applied to svn, but it would be much better not to set critical variables to invalid values to begin with or overwrite them individually with harmless values if they have been filled with something out of range [...]
+ if (m->max_channel[substr] >= MAX_CHANNELS + || m->max_matrix_channel[substr] >= MAX_CHANNELS) { + if (substr > 0) { + av_log(m->avctx, AV_LOG_INFO, + "Substream %d contains more channels than the maximum " + "supported by this decoder (%d). Only substreams up to %d " + "will be decoded. Please provide a sample of this file " + "to the FFmpeg development list.\n", + substr, MAX_CHANNELS, substr - 1); + m->max_decoded_substream = substr - 1; + m->avctx->channels = m->max_channel[substr - 1] + 1;
could you please, remove this fraile hack and just return -1 if the file contains more channels than the decoder supports [...]
+ av_log(m->avctx, AV_LOG_ERROR, + "Non 1:1 channel assignments are used in this stream. " + "Please send a sample of this file to the FFmpeg " + "development list.\n");
no! upload to the ftp and send a mail pointing to the file to the list [...]
+ for (i = 0; i < m->access_unit_size_pow2; i++) { + uint32_t tmp = seed >> 15; + m->noise_buffer[i] = noise_table[tmp]; + seed = (seed << 8) ^ tmp ^ (tmp << 5); + seed &= 0x7fffff; + }
buffer overflow, access_unit_size_pow2 is not checked for validity and the buffer is smaller than the max this variable can be please check the source of the WHOLE decoder for such buffer overflows! and add proper checks [...]
+ if (show_bits_long(&gb, 31) == (0xf8726fba >> 1)) { + dprintf(m->avctx, "Found major sync\n"); + if (read_major_sync(m, buf + 4, buf_size - 4) < 0) + goto error; + skip_bits_long(&gb, 28 * 8); + } + + if (!m->has_params) + {
{ placement is inconsistant [...]
+static int mlp_decode_frame(AVCodecContext *avctx, + void *data, int *data_size, + uint8_t *buf, int buf_size) +{ + MLPDecodeContext *m = avctx->priv_data; + return read_access_unit(m, data, data_size, buf, buf_size); +}
read_access_unit is senseless, you can just put its code in here [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle
Hi, On Dec 2, 2007 2:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sat, Dec 01, 2007 at 11:49:17PM +0000, Ian Caulfield wrote:
It appears I attached an older version of the patch in my last email, sorry. I've attached an up-to-date patch (hopefully) addressing Diego's comments.
On Dec 1, 2007 10:08 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
As well as an error being returned, the "have parameters" flag in the context is cleared, causing all packets to be rejected until we get a (valid) sync packet.
there is no variable, constant or otherwise called "have parameters" or have*
The variable "has_params" is used (previously called in_sync).
rather call it params_valid please because if its 0 they are not, also this design is fragile, every future change to the codec would have to be checked that it doesnt use anything from the context if has_params != 1
Fair enough, although currently is has_params is zero, frames will be skipped until read_major_sync sets it again.
a memset(0) of the whole context is minimum if you want to keep this design and want to see it applied to svn, but it would be much better not to set critical variables to invalid values to begin with or overwrite them individually with harmless values if they have been filled with something out of range
I'll look at doing this if you'd like.
[...]
+ if (m->max_channel[substr] >= MAX_CHANNELS + || m->max_matrix_channel[substr] >= MAX_CHANNELS) { + if (substr > 0) { + av_log(m->avctx, AV_LOG_INFO, + "Substream %d contains more channels than the maximum " + "supported by this decoder (%d). Only substreams up to %d " + "will be decoded. Please provide a sample of this file " + "to the FFmpeg development list.\n", + substr, MAX_CHANNELS, substr - 1); + m->max_decoded_substream = substr - 1; + m->avctx->channels = m->max_channel[substr - 1] + 1;
could you please, remove this fraile hack and just return -1 if the file contains more channels than the decoder supports
Fair enough, although I would have thought that decoding a downmix with fewer channels would be preferable to just borking in this case...
[...]
+ for (i = 0; i < m->access_unit_size_pow2; i++) { + uint32_t tmp = seed >> 15; + m->noise_buffer[i] = noise_table[tmp]; + seed = (seed << 8) ^ tmp ^ (tmp << 5); + seed &= 0x7fffff; + }
buffer overflow, access_unit_size_pow2 is not checked for validity and the buffer is smaller than the max this variable can be
access_unit_size_pow2 is set directly depending on the sample rate, and the sample rate _is_ checked for validity _before_ the value is set. I don't see any way to put an invalid value into the variable without corrupting memory...
please check the source of the WHOLE decoder for such buffer overflows! and add proper checks
I'll have a look - would you prefer redundant checks in cases like the above? Ian
On Mon, Dec 03, 2007 at 04:05:28PM +0000, Ian Caulfield wrote:
Hi,
On Dec 2, 2007 2:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sat, Dec 01, 2007 at 11:49:17PM +0000, Ian Caulfield wrote:
It appears I attached an older version of the patch in my last email, sorry. I've attached an up-to-date patch (hopefully) addressing Diego's comments.
On Dec 1, 2007 10:08 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
As well as an error being returned, the "have parameters" flag in the context is cleared, causing all packets to be rejected until we get a (valid) sync packet.
there is no variable, constant or otherwise called "have parameters" or have*
The variable "has_params" is used (previously called in_sync).
rather call it params_valid please because if its 0 they are not, also this design is fragile, every future change to the codec would have to be checked that it doesnt use anything from the context if has_params != 1
Fair enough, although currently is has_params is zero, frames will be skipped until read_major_sync sets it again.
a memset(0) of the whole context is minimum if you want to keep this design and want to see it applied to svn, but it would be much better not to set critical variables to invalid values to begin with or overwrite them individually with harmless values if they have been filled with something out of range
I'll look at doing this if you'd like.
cleaning up the validity checks is absolutely needed if you want to see this in svn, how you implement it is your decission but it must be idiot proof that is, code which looks unrelated to a check should be unrelated to the check ...
[...]
+ if (m->max_channel[substr] >= MAX_CHANNELS + || m->max_matrix_channel[substr] >= MAX_CHANNELS) { + if (substr > 0) { + av_log(m->avctx, AV_LOG_INFO, + "Substream %d contains more channels than the maximum " + "supported by this decoder (%d). Only substreams up to %d " + "will be decoded. Please provide a sample of this file " + "to the FFmpeg development list.\n", + substr, MAX_CHANNELS, substr - 1); + m->max_decoded_substream = substr - 1; + m->avctx->channels = m->max_channel[substr - 1] + 1;
could you please, remove this fraile hack and just return -1 if the file contains more channels than the decoder supports
Fair enough, although I would have thought that decoding a downmix with fewer channels would be preferable to just borking in this case...
you can change your code to support more channels ... i dont see why you would want to support some max and then do forced downmixing to that ...
[...]
+ for (i = 0; i < m->access_unit_size_pow2; i++) { + uint32_t tmp = seed >> 15; + m->noise_buffer[i] = noise_table[tmp]; + seed = (seed << 8) ^ tmp ^ (tmp << 5); + seed &= 0x7fffff; + }
buffer overflow, access_unit_size_pow2 is not checked for validity and the buffer is smaller than the max this variable can be
access_unit_size_pow2 is set directly depending on the sample rate, and the sample rate _is_ checked for validity _before_ the value is set. I don't see any way to put an invalid value into the variable without corrupting memory...
in mlp_parser.c: ff_mlp_read_major_sync() you read 4 bits ratebits = get_bits(&gb, 4); initalize group1_samplerate of MLPHeaderInfo based on it mh->group1_samplerate = mlp_samplerate(ratebits); static int mlp_samplerate(int in) { if (in == 0xF) return 0; return (in & 8 ? 44100 : 48000) << (in & 7) ; } both the above are done on either side of a branch with various other things next the access_unit_size* from MLPHeaderInfo get initalized based on it mh->access_unit_size = 40 << (ratebits & 7); mh->access_unit_size_pow2 = 64 << (ratebits & 7); after that and many unrelated things ff_mlp_read_major_sync() returns 0 (success) in mlpdec.c: read_major_sync() you check mh.group1_samplerate against 0 and MAX_SAMPLERATE before copying access_unit_size* from MLPHeaderInfo to MLPDecodeContext so yes you are right, it seems like this case cant overflow but its fantastically complicated, such code is simply not fit for svn it would take litteraly days to check any single line change anyone (who is not the author and doesnt know about these tricks) want to do also you will forget about these things a year after you wrote it ... and a harmless looking change to any point of the sample_rate path could cause access_unit_size_pow2 to pass with a too large value ...
please check the source of the WHOLE decoder for such buffer overflows! and add proper checks
I'll have a look - would you prefer redundant checks in cases like the above?
id prefer clean code without redundant checks if that isnt possibly i prefer redundant checks if its security relevant and no redundant checks if its speed critical code also please always check values for validity soon after you read them for example like: ratebits = get_bits(&gb, 4); if((ratebits&7) > 2) return -1; and support all values which pass these checks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato
Hi Another revision of the patch attached. Hopefully creeping towards acceptability :) On Dec 3, 2007 5:34 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Dec 03, 2007 at 04:05:28PM +0000, Ian Caulfield wrote:
On Dec 2, 2007 2:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
a memset(0) of the whole context is minimum if you want to keep this design and want to see it applied to svn, but it would be much better not to set critical variables to invalid values to begin with or overwrite them individually with harmless values if they have been filled with something out of range
I'll look at doing this if you'd like.
cleaning up the validity checks is absolutely needed if you want to see this in svn, how you implement it is your decission but it must be idiot proof that is, code which looks unrelated to a check should be unrelated to the check ...
OK, I've gone through the validity checks and made sure they either check the values before writing to the context or that they zero the values if they're invalid (except one case checking the filter denominators are the same - a bug letting that slip through would sound crap rather than present a security hole).
Fair enough, although I would have thought that decoding a downmix with fewer channels would be preferable to just borking in this case...
you can change your code to support more channels ... i dont see why you would want to support some max and then do forced downmixing to that ...
I've increased the MAX_CHANNELS define to 16, to cover the maximum value of a 4-bit field.
so yes you are right, it seems like this case cant overflow but its fantastically complicated, such code is simply not fit for svn it would take litteraly days to check any single line change anyone (who is not the author and doesnt know about these tricks) want to do also you will forget about these things a year after you wrote it ... and a harmless looking change to any point of the sample_rate path could cause access_unit_size_pow2 to pass with a too large value ...
please check the source of the WHOLE decoder for such buffer overflows! and add proper checks
I'll have a look - would you prefer redundant checks in cases like the above?
id prefer clean code without redundant checks if that isnt possibly i prefer redundant checks if its security relevant and no redundant checks if its speed critical code
I've checked all the loops in the code, and made sure that the variables used as limits are checked for validity. No doubt you'll spot something I've missed though :)
also please always check values for validity soon after you read them for example like:
ratebits = get_bits(&gb, 4); if((ratebits&7) > 2) return -1;
and support all values which pass these checks
In this case I've left the checks where they were, but checked all the derived values - the above line is in the parser, which probably shouldn't fall over just because the decoder won't understand that part of the metadata - stream copy for example should still work. Ian
On Dec 4, 2007 3:03 PM, Ian Caulfield <ian.caulfield at gmail.com> wrote:
Hi
Another revision of the patch attached. Hopefully creeping towards acceptability :)
Updated version attached - fixes a typo and some warnings that crept in. Ian
On Tue, Dec 04, 2007 at 03:50:50PM +0000, Ian Caulfield wrote:
On Dec 4, 2007 3:03 PM, Ian Caulfield <ian.caulfield at gmail.com> wrote:
Hi
Another revision of the patch attached. Hopefully creeping towards acceptability :)
Updated version attached - fixes a typo and some warnings that crept in.
[...]
+#define SAMPLE_MESSAGE \ + "Please file a bug report following the instructions at " \ + "http://ffmpeg.mplayerhq.hu/bugreports.html and include " \ + "a sample of this file."
very ugly use of preprocessor and wasting memory by duplication of slight variations of this please use: static const char *sample_message= ... [...]
+ uint8_t data_check_present[MAX_SUBSTREAMS]; +// uint8_t ch_assign[MAX_SUBSTREAMS][MAX_CHANNELS];
why is this commented out? if its unused it should be removed [...]
+static const uint8_t huffman_tables[3][18][2] = { + { /* Huffman table 0, -7 - +10 */ + {0x01, 9}, {0x01, 8}, {0x01, 7}, {0x01, 6}, {0x01, 5}, {0x01, 4}, {0x01, 3}, + {0x04, 3}, {0x05, 3}, {0x06, 3}, {0x07, 3}, + {0x03, 3}, {0x05, 4}, {0x09, 5}, {0x11, 6}, {0x21, 7}, {0x41, 8}, {0x81, 9}, + }, { /* Huffman table 1, -7 - +8 */ + {0x01, 9}, {0x01, 8}, {0x01, 7}, {0x01, 6}, {0x01, 5}, {0x01, 4}, {0x01, 3}, + {0x02, 2}, {0x03, 2}, + {0x03, 3}, {0x05, 4}, {0x09, 5}, {0x11, 6}, {0x21, 7}, {0x41, 8}, {0x81, 9}, + }, { /* Huffman table 1, -7 - +7 */
Huffman table 2 i would guess ... [...]
+/** Initialize static data, constant between all invocations of the codec. */ + +static void init_static() +{ + static int done = 0; + + if (!done) { + init_vlc(&huff_vlc[0], VLC_BITS, 18, + &huffman_tables[0][0][1], 2, 1, + &huffman_tables[0][0][0], 2, 1, 1); + init_vlc(&huff_vlc[1], VLC_BITS, 16, + &huffman_tables[1][0][1], 2, 1, + &huffman_tables[1][0][0], 2, 1, 1); + init_vlc(&huff_vlc[2], VLC_BITS, 15, + &huffman_tables[2][0][1], 2, 1, + &huffman_tables[2][0][0], 2, 1, 1); + + av_crc_init(crc_63, 0, 8, 0x63, sizeof(crc_63)); + av_crc_init(crc_1D, 0, 8, 0x1D, sizeof(crc_1D)); + + done = 1; + }
static int done is unneeded you can check some entry from one of the tables [...]
+ for (i = 0; i < (bit_size + 2) % 8; i++) {
&7 is faster than %8 depending on how stupid the compiler is [...]
+ if (sign_shift >= 0) + m->sign_huff_offset[ch] += (-1) << sign_shift;
-= 1 << sign_shift [...]
+/** Initialize the decoder. */ + +static int mlp_decode_init(AVCodecContext *avctx) +{ + MLPDecodeContext *m = avctx->priv_data; + + init_static(); + m->avctx = avctx; + memset(m->lossless_check_data, 0xff, sizeof(m->lossless_check_data)); + return 0; +}
is there a special reason why init_static() is a seperate function?
+ +/** Read a major sync info header - contains high level information about + * the stream - sample rate, channel arrangement etc. Most of this + * information is not actually necessary for decoding, only for playback. + */ + +static int read_major_sync(MLPDecodeContext *m, const uint8_t *buf, + unsigned int buf_size) +{ + MLPHeaderInfo mh; +
+ if (ff_mlp_read_major_sync(m->avctx, &mh, buf, buf_size) != 0) + return -1; + + if (mh.stream_type == 0xbb) { + if (mh.group1_bits == 0) { + av_log(m->avctx, AV_LOG_ERROR, "Invalid/unknown bits per sample\n"); + return -1; + } + if (mh.group2_bits > mh.group1_bits) { + av_log(m->avctx, AV_LOG_ERROR, + "Channel group 2 cannot have more bits per sample than group 1\n"); + return -1; + } + + if (mh.group2_samplerate && mh.group2_samplerate != mh.group1_samplerate) { + av_log(m->avctx, AV_LOG_ERROR, + "Channel groups with differing sample rates not currently supported\n"); + return -1; + } + }
there is no reason to skip these checks if stream_type differs from 0xbb, maybe they are guranteed to be set to safe values if stream_type is not 0xBB and or not used if its 0xbb currently but what if somone discovers a new stream type which uses them and changes the parser to set them? it cant be expected that every change to the code is accompanied by a 10hour review of the whole code i repeat the checks should be where the variables are read ideally there are 2 kinds of values the ones which are valid, your decoder should support them, and the ones which are not valid, they should be rejected during reading the header also you can easily just pass a check_stuff flag as argument to ff_mlp_read_major_sync() if you dont want it to check stuff in the parser but that makes no sense [...]
+ dprintf(m->avctx, "Filter %d, order %d\n", filter, order);
please merge these hundreads of dprintf() in few proper if(debug & FF_DEBUG_*) av_log() or dprintf() instead of av_log() for the more obscure/less usefull ones having this debug stuff interleaved with normal code makes reading it harder ... [...]
+ "%s filter coeff_bits must be between 1 and 16\n", + filter ? "IIR" : "FIR");
%cIR filter coeff_bits must be between 1 and 16\n", filter ? 'I' : 'F' [...]
+ for (i = 0; i < m->filter_order[channel][0]; i++) + accum += (int64_t)m->filter_state[channel][0][i] * + m->filter_coeff[channel][0][i]; + + for (i = 0; i < m->filter_order[channel][1]; i++) + accum += (int64_t) m->filter_state[channel][1][i] * + m->filter_coeff[channel][1][i];
for(j=0; j<2; j++) for (i = 0; i < m->filter_order[channel][j]; i++) accum += (int64_t)m->filter_state[channel][j][i] * m->filter_coeff[channel][j][i]; [...]
+ uint32_t tmp; + uint32_t seed = m->noisegen_seed[substr]; + unsigned int maxchan = m->max_matrix_channel[substr]; + + for (i = 0; i < m->blockpos[substr]; i++) { + m->sample_buffer[i][maxchan+1] = ((int8_t)(seed >> 15)) << m->noise_shift[substr]; + m->sample_buffer[i][maxchan+2] = ((int8_t)(seed >> 7)) << m->noise_shift[substr]; + + tmp = seed >> 7; + seed = (seed << 16) ^ tmp ^ (tmp << 5); + seed &= 0x7fffff; + }
uint32_t seed = m->noisegen_seed[substr]; unsigned int maxchan = m->max_matrix_channel[substr]; for (i = 0; i < m->blockpos[substr]; i++) { uint16_t seed_shr7 = seed >> 7; m->sample_buffer[i][maxchan+1] = ((int8_t)(seed >> 15)) << m->noise_shift[substr]; m->sample_buffer[i][maxchan+2] = ((int8_t) seed_shr7 ) << m->noise_shift[substr]; seed = (seed << 16) ^ seed_shr7 ^ (seed_shr7 << 5); }
+ + m->noisegen_seed[substr] = seed; +} + +/** Generate a block of noise, used when restart_sync == 0x31eb. */ + +static void generate_noise_2(MLPDecodeContext *m, unsigned int substr) +{ + unsigned int i; + uint32_t seed = m->noisegen_seed[substr]; +
+ for (i = 0; i < m->access_unit_size_pow2; i++) { + uint32_t tmp = seed >> 15; + m->noise_buffer[i] = noise_table[tmp]; + seed = (seed << 8) ^ tmp ^ (tmp << 5); + seed &= 0x7fffff; + }
for (i = 0; i < m->access_unit_size_pow2; i++) { uint8_t tmp = seed >> 15; m->noise_buffer[i] = noise_table[tmp]; seed = (seed << 8) ^ tmp ^ (tmp << 5); } [...]
+static void output_data_internal(MLPDecodeContext *m, unsigned int substr, + uint8_t *data, unsigned int *data_size, int is32) +{ + unsigned int i, ch = 0; + int32_t sample; + int32_t *data_32 = (int32_t*) data; + int16_t *data_16 = (int16_t*) data; + + for (i = 0; i < m->blockpos[substr]; i++) { + for (ch = 0; ch <= m->max_channel[substr]; ch++) {
+ sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch];
int sample = ... theres no need to preserve the value outside the loop ... [...]
+static uint8_t calculate_parity(const uint8_t *buf, unsigned int buf_size) +{ + uint32_t scratch = 0; +
+ for (; buf_size >= 4; buf_size -= 4) { + scratch ^= *((const uint32_t*)buf); + buf += 4; + }
for(;buf < buf_end-3; buf+=4) scratch ^= *((const uint32_t*)buf); 1 add less
+ scratch ^= scratch >> 16; + scratch ^= scratch >> 8; + scratch &= 0xff; +
+ while (buf_size > 0) { + scratch ^= *buf; + buf++; + buf_size--; + }
for(;buf < buf_end; buf++) scratch ^= *buf; [...]
+ if (restart_absent && !m->restart_seen[substr]) { + av_log(m->avctx, AV_LOG_ERROR, + "No restart header indicated for substream %d", substr); + goto error; + }
what is this check and the whole complicated restart_seen logic good for? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker, user questions for the command line tools ffmpeg, ffplay, ... as well as questions about how to use libav* should be sent to the ffmpeg-user mailinglist.
Hi, New version attached, hopefully addressing most of your comments. On Dec 10, 2007 11:20 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Dec 04, 2007 at 03:50:50PM +0000, Ian Caulfield wrote: [...]
+/** Initialize the decoder. */ + +static int mlp_decode_init(AVCodecContext *avctx) +{ + MLPDecodeContext *m = avctx->priv_data; + + init_static(); + m->avctx = avctx; + memset(m->lossless_check_data, 0xff, sizeof(m->lossless_check_data)); + return 0; +}
is there a special reason why init_static() is a seperate function?
Not really - I can fold it in if you'd like.
[...]
+ dprintf(m->avctx, "Filter %d, order %d\n", filter, order);
please merge these hundreads of dprintf() in few proper if(debug & FF_DEBUG_*) av_log()
or dprintf() instead of av_log() for the more obscure/less usefull ones
having this debug stuff interleaved with normal code makes reading it harder ...
I've deleted a lot of the dprintfs that weren't especially useful to me any more.
[...]
+ if (restart_absent && !m->restart_seen[substr]) { + av_log(m->avctx, AV_LOG_ERROR, + "No restart header indicated for substream %d", substr); + goto error; + }
what is this check and the whole complicated restart_seen logic good for?
The main point is to detect if the restart header containing the needed decoding parameters is missing and to error out if so. I've reworked the check to be somewhat saner. Ian
On Tue, Dec 18, 2007 at 11:38:09AM +0000, Ian Caulfield wrote:
Hi,
New version attached, hopefully addressing most of your comments.
On Dec 10, 2007 11:20 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Dec 04, 2007 at 03:50:50PM +0000, Ian Caulfield wrote: [...]
+/** Initialize the decoder. */ + +static int mlp_decode_init(AVCodecContext *avctx) +{ + MLPDecodeContext *m = avctx->priv_data; + + init_static(); + m->avctx = avctx; + memset(m->lossless_check_data, 0xff, sizeof(m->lossless_check_data)); + return 0; +}
is there a special reason why init_static() is a seperate function?
Not really - I can fold it in if you'd like.
[...]
+ dprintf(m->avctx, "Filter %d, order %d\n", filter, order);
please merge these hundreads of dprintf() in few proper if(debug & FF_DEBUG_*) av_log()
or dprintf() instead of av_log() for the more obscure/less usefull ones
having this debug stuff interleaved with normal code makes reading it harder ...
I've deleted a lot of the dprintfs that weren't especially useful to me any more.
[...]
+ if (restart_absent && !m->restart_seen[substr]) { + av_log(m->avctx, AV_LOG_ERROR, + "No restart header indicated for substream %d", substr); + goto error; + }
what is this check and the whole complicated restart_seen logic good for?
The main point is to detect if the restart header containing the needed decoding parameters is missing and to error out if so. I've reworked the check to be somewhat saner. [...] +typedef struct MLPDecodeContext { + AVCodecContext *avctx; + + uint8_t params_valid; + uint8_t max_decoded_substream; + int access_unit_size; + int access_unit_size_pow2; + uint8_t restart_seen[MAX_SUBSTREAMS]; + + uint8_t num_substreams; + + //@{ + /** Restart header data */
+ uint16_t restart_sync[MAX_SUBSTREAMS];
shouldnt this rather be called sync_word ?
+ uint8_t min_channel[MAX_SUBSTREAMS]; + uint8_t max_channel[MAX_SUBSTREAMS]; + uint8_t max_matrix_channel[MAX_SUBSTREAMS]; + uint8_t noise_shift[MAX_SUBSTREAMS]; + uint32_t noisegen_seed[MAX_SUBSTREAMS]; + uint8_t data_check_present[MAX_SUBSTREAMS]; + uint8_t param_presence_flags[MAX_SUBSTREAMS]; + //@} + + //@{ + /** Matrix data */ + uint8_t num_primitive_matrices[MAX_SUBSTREAMS]; + uint8_t matrix_ch[MAX_SUBSTREAMS][MAX_MATRICES]; + uint8_t lsb_bypass[MAX_SUBSTREAMS][MAX_MATRICES]; + int32_t matrix_coeff[MAX_SUBSTREAMS][MAX_MATRICES][MAX_CHANNELS+2]; + uint8_t matrix_noise_shift[MAX_SUBSTREAMS][MAX_MATRICES]; + //@} + + uint16_t blocksize[MAX_SUBSTREAMS]; + uint16_t blockpos[MAX_SUBSTREAMS]; + int8_t output_shift[MAX_SUBSTREAMS][MAX_CHANNELS]; + uint8_t quant_step_size[MAX_SUBSTREAMS][MAX_CHANNELS]; + + //@{ + /* Filter data. Filter 0 is an FIR filter, filter 1 IIR. */ + uint8_t filter_order[MAX_CHANNELS][2]; + uint8_t filter_coeff_q[MAX_CHANNELS][2]; + int32_t filter_coeff[MAX_CHANNELS][2][MAX_FILTER_ORDER]; + int32_t filter_state[MAX_CHANNELS][2][MAX_FILTER_ORDER]; + //@} + + //@{ + /** Sample data coding infomation */ + int16_t huff_offset[MAX_CHANNELS]; + int32_t sign_huff_offset[MAX_CHANNELS]; + uint8_t codebook[MAX_CHANNELS]; + uint8_t huff_lsbs[MAX_CHANNELS]; + //@} + + int32_t lossless_check_data[MAX_SUBSTREAMS]; + + int8_t noise_buffer[MAX_BLOCKSIZE_POW2]; + int8_t bypassed_lsbs[MAX_BLOCKSIZE][MAX_CHANNELS]; + int32_t sample_buffer[MAX_BLOCKSIZE][MAX_CHANNELS+2]; +} MLPDecodeContext;
some comments explaing what each var is, would be nice [...]
+static inline void calculate_sign_huff(MLPDecodeContext *m, unsigned int substr, + unsigned int ch) +{ + int lsb_bits = m->huff_lsbs[ch] - m->quant_step_size[substr][ch]; + int sign_shift = lsb_bits + (m->codebook[ch] ? 2 - m->codebook[ch] : -1); + + m->sign_huff_offset[ch] = m->huff_offset[ch]; + + if (sign_shift >= 0) + m->sign_huff_offset[ch] -= 1 << sign_shift; +} + +/** Read a sample, consisting of either, both or neither of entropy-coded MSBs + * and plain LSBs. + */ + +static inline int read_huff(MLPDecodeContext *m, GetBitContext *gbp, + unsigned int substr, unsigned int channel) +{ + int codebook = m->codebook[channel]; + int quant_step_size = m->quant_step_size[substr][channel]; + int lsb_bits = m->huff_lsbs[channel] - quant_step_size; + int result = 0; + + if (codebook > 0) + result = get_vlc2(gbp, huff_vlc[codebook-1].table, + VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7;
the -7 can be merged into sign_huff_offset [...]
+static void generate_noise_1(MLPDecodeContext *m, unsigned int substr) +{ + unsigned int i; + uint32_t seed = m->noisegen_seed[substr]; + unsigned int maxchan = m->max_matrix_channel[substr]; + + for (i = 0; i < m->blockpos[substr]; i++) { + uint16_t seed_shr7 = seed >> 7; + m->sample_buffer[i][maxchan+1] = ((int8_t)(seed >> 15)) << m->noise_shift[substr]; + m->sample_buffer[i][maxchan+2] = ((int8_t) seed_shr7) << m->noise_shift[substr]; + + seed = (seed << 16) ^ seed_shr7 ^ (seed_shr7 << 5); + } + + m->noisegen_seed[substr] = seed; +} + +/** Generate a block of noise, used when restart_sync == 0x31eb. */ + +static void generate_noise_2(MLPDecodeContext *m, unsigned int substr) +{ + unsigned int i; + uint32_t seed = m->noisegen_seed[substr]; + + for (i = 0; i < m->access_unit_size_pow2; i++) {
+ uint8_t tmp = seed >> 15;
please name this seed_shr15 [...]
+static int read_access_unit(AVCodecContext *avctx, void* data, int *data_size, + uint8_t *buf, int buf_size) +{ + MLPDecodeContext *m = avctx->priv_data; + GetBitContext gb; + unsigned int length, substr; + unsigned int substream_start; + unsigned int header_size; + uint8_t substream_parity_present[MAX_SUBSTREAMS]; + uint16_t substream_data_len[MAX_SUBSTREAMS]; + + if (buf_size < 2) + return 0; + + if (*data_size < m->avctx->channels * m->access_unit_size + * (m->avctx->sample_fmt == SAMPLE_FMT_S32 ? 4 : 2)) + return -1;
its nice that you check that, it would be nicer i you did after setting the variables, like that it is just exploitable [...]
+ skip_bits(&gb, (-get_bits_count(&gb)) & 15); + if (show_bits_long(&gb, 32) == 0xd234d234 || + show_bits_long(&gb, 18) == 0xd234e) { + skip_bits(&gb, 18); + av_log(m->avctx, AV_LOG_INFO, "End of stream indicated\n"); + if (get_bits1(&gb)) + m->blockpos[substr] -= get_bits(&gb, 13); + else + skip_bits(&gb, 13);
exploitable, blockpos overflows and is later used as array limit (for exmple in rematrix_channels) iam starting to be scared by your patches, can you _please_ take this more serious, and check the variables used to decide where to write data to? all of them, double check your code, if i find another such issue i will not review this patch again and permanently reject it! [...9 -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire
Hi, New version attached On Jan 6, 2008 10:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Dec 18, 2007 at 11:38:09AM +0000, Ian Caulfield wrote:
+typedef struct MLPDecodeContext { + AVCodecContext *avctx; + + uint8_t params_valid; + uint8_t max_decoded_substream; + int access_unit_size; + int access_unit_size_pow2; + uint8_t restart_seen[MAX_SUBSTREAMS]; + + uint8_t num_substreams; + + //@{ + /** Restart header data */
+ uint16_t restart_sync[MAX_SUBSTREAMS];
shouldnt this rather be called sync_word ?
I've changed it to restart_sync_word (to distinguish it from the major sync word if that ever needs to be stored in the context)
+} MLPDecodeContext;
some comments explaing what each var is, would be nice
I've put comments for most of the variables (I thought some were sufficiently obvious to not require explanation - will fill those in too if you like).
+ + if (codebook > 0) + result = get_vlc2(gbp, huff_vlc[codebook-1].table, + VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7;
the -7 can be merged into sign_huff_offset
Done
+static void generate_noise_2(MLPDecodeContext *m, unsigned int substr) +{ + unsigned int i; + uint32_t seed = m->noisegen_seed[substr]; + + for (i = 0; i < m->access_unit_size_pow2; i++) {
+ uint8_t tmp = seed >> 15;
please name this seed_shr15
Done
+ if (*data_size < m->avctx->channels * m->access_unit_size + * (m->avctx->sample_fmt == SAMPLE_FMT_S32 ? 4 : 2)) + return -1;
its nice that you check that, it would be nicer i you did after setting the variables, like that it is just exploitable
Moved to output_data_internal
[...]
+ skip_bits(&gb, (-get_bits_count(&gb)) & 15); + if (show_bits_long(&gb, 32) == 0xd234d234 || + show_bits_long(&gb, 18) == 0xd234e) { + skip_bits(&gb, 18); + av_log(m->avctx, AV_LOG_INFO, "End of stream indicated\n"); + if (get_bits1(&gb)) + m->blockpos[substr] -= get_bits(&gb, 13); + else + skip_bits(&gb, 13);
exploitable, blockpos overflows and is later used as array limit (for exmple in rematrix_channels) iam starting to be scared by your patches, can you _please_ take this more serious, and check the variables used to decide where to write data to? all of them, double check your code, if i find another such issue i will not review this patch again and permanently reject it!
Sorry about that - I was very careful and checked all loops to make sure the bounds were checked properly. I was then given a new sample that decoded incorrectly and added the above code to handle it without thinking fully. I'll avoid making any non-review changes until reviewing is over, and try to be more paranoid in general. Hopefully there aren't any holes left... Ian
On Mon, Jan 07, 2008 at 04:39:11PM +0000, Ian Caulfield wrote:
Hi,
New version attached
On Jan 6, 2008 10:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Dec 18, 2007 at 11:38:09AM +0000, Ian Caulfield wrote:
+typedef struct MLPDecodeContext { + AVCodecContext *avctx; + + uint8_t params_valid; + uint8_t max_decoded_substream; + int access_unit_size; + int access_unit_size_pow2; + uint8_t restart_seen[MAX_SUBSTREAMS]; + + uint8_t num_substreams; + + //@{ + /** Restart header data */
+ uint16_t restart_sync[MAX_SUBSTREAMS];
shouldnt this rather be called sync_word ?
I've changed it to restart_sync_word (to distinguish it from the major sync word if that ever needs to be stored in the context)
+} MLPDecodeContext;
some comments explaing what each var is, would be nice
I've put comments for most of the variables (I thought some were sufficiently obvious to not require explanation - will fill those in too if you like).
+ + if (codebook > 0) + result = get_vlc2(gbp, huff_vlc[codebook-1].table, + VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7;
the -7 can be merged into sign_huff_offset
Done
+static void generate_noise_2(MLPDecodeContext *m, unsigned int substr) +{ + unsigned int i; + uint32_t seed = m->noisegen_seed[substr]; + + for (i = 0; i < m->access_unit_size_pow2; i++) {
+ uint8_t tmp = seed >> 15;
please name this seed_shr15
Done
+ if (*data_size < m->avctx->channels * m->access_unit_size + * (m->avctx->sample_fmt == SAMPLE_FMT_S32 ? 4 : 2)) + return -1;
its nice that you check that, it would be nicer i you did after setting the variables, like that it is just exploitable
Moved to output_data_internal
[...]
+ skip_bits(&gb, (-get_bits_count(&gb)) & 15); + if (show_bits_long(&gb, 32) == 0xd234d234 || + show_bits_long(&gb, 18) == 0xd234e) { + skip_bits(&gb, 18); + av_log(m->avctx, AV_LOG_INFO, "End of stream indicated\n"); + if (get_bits1(&gb)) + m->blockpos[substr] -= get_bits(&gb, 13); + else + skip_bits(&gb, 13);
exploitable, blockpos overflows and is later used as array limit (for exmple in rematrix_channels) iam starting to be scared by your patches, can you _please_ take this more serious, and check the variables used to decide where to write data to? all of them, double check your code, if i find another such issue i will not review this patch again and permanently reject it!
Sorry about that - I was very careful and checked all loops to make sure the bounds were checked properly. I was then given a new sample that decoded incorrectly and added the above code to handle it without thinking fully. I'll avoid making any non-review changes until reviewing is over, and try to be more paranoid in general. Hopefully there aren't any holes left... [...] +/** Data table used for TrueHD noise generation function */ + +static int8_t noise_table[256] = {
static const [...]
+/** Write the audio data into the output buffer. + */ + +static int output_data_internal(MLPDecodeContext *m, unsigned int substr, + uint8_t *data, unsigned int *data_size, int is32) +{ + unsigned int i, ch = 0; + int32_t *data_32 = (int32_t*) data; + int16_t *data_16 = (int16_t*) data; + + if (*data_size < m->max_channel[substr] * m->blockpos[substr] + * (is32 ? 4 : 2)) + return -1; + + for (i = 0; i < m->blockpos[substr]; i++) { + for (ch = 0; ch <= m->max_channel[substr]; ch++) {
ideg ... lets try this differently please actually TEST ALL security checks in the decoder, that is set variables manually to the max which passes the tests ahh and i want to see the test code, attach it! and go over the whole code and check that there are no other max vs. max+1 errors and run -absf noise -acodec copy over a long (>1hour) stream and decode that afterwards if it crashes or ends in an infinite loop, fix your code and try again [...]
+/** XOR together all the bytes of a buffer. + * Does this belong in dspcontext? + */ + +static uint8_t calculate_parity(const uint8_t *buf, unsigned int buf_size) +{ + uint32_t scratch = 0; + const uint8_t *buf_end = buf + buf_size; + + for (; buf < buf_end - 3; buf += 4) + scratch ^= *((const uint32_t*)buf); + + scratch ^= scratch >> 16; + scratch ^= scratch >> 8;
+ scratch &= 0xff;
unnneeded [...]
+/** + * Read an access unit from the stream. + * Returns -1 on error, 0 if not enough data is present in the input stream + * otherwise returns the number of bytes consumed. + */ + +static int read_access_unit(AVCodecContext *avctx, void* data, int *data_size, + uint8_t *buf, int buf_size) +{ + MLPDecodeContext *m = avctx->priv_data; + GetBitContext gb; + unsigned int length, substr; + unsigned int substream_start; + unsigned int header_size; + uint8_t substream_parity_present[MAX_SUBSTREAMS];
+ uint16_t substream_data_len[MAX_SUBSTREAMS];
every use of substream_data_len either has a *2 or a *16 after it multiplying this by *2 when its set would simplify the code [...]
+ skip_bits(&gb, (-get_bits_count(&gb)) & 15); + if ((substream_data_len[substr] * 16) - get_bits_count(&gb) >= 48 &&
superflouse () [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I wish the Xiph folks would stop pretending they've got something they do not. Somehow I fear this will remain a wish. -- M?ns Rullg?rd
participants (7)
-
aurel＠gnuage.org -
banan＠ludd.ltu.se -
cehoyos＠ag.or.at -
diego＠biurrun.de -
ian.caulfield＠gmail.com -
jrmuizel＠gmail.com -
michaelni＠gmx.at