[PATCH] Added integer 32 bits support to wavpack
The attached patch completes the support for files with INT32INFO chunk with non zero sent_bits. -- fenrir
On Fri, May 01, 2009 at 01:49:37PM +0200, Laurent Aimar wrote:
The attached patch completes the support for files with INT32INFO chunk with non zero sent_bits.
[...]
@@ -344,6 +348,7 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d int A, B, L, L2, R, R2, bit; int pos = 0; uint32_t crc = 0xFFFFFFFF; + uint32_t crc_extra = 0xFFFFFFFF; int16_t *dst16 = dst; int32_t *dst32 = dst;
@@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d if(s->joint) L += (R -= (L >> 1)); crc = (crc * 3 + L) * 3 + R; + if(s->extra_bits){ + L <<= s->extra_bits; + R <<= s->extra_bits; + + if(s->got_extra){ + L |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff);
the second & can be avoided if the >> is unsigned [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many things microsoft did are stupid, but not doing something just because microsoft did it is even more stupid. If everything ms did were stupid they would be bankrupt already.
Hi, On Fri, May 01, 2009, Michael Niedermayer wrote:
On Fri, May 01, 2009 at 01:49:37PM +0200, Laurent Aimar wrote:
The attached patch completes the support for files with INT32INFO chunk with non zero sent_bits.
[...]
@@ -344,6 +348,7 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d int A, B, L, L2, R, R2, bit; int pos = 0; uint32_t crc = 0xFFFFFFFF; + uint32_t crc_extra = 0xFFFFFFFF; int16_t *dst16 = dst; int32_t *dst32 = dst;
@@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d if(s->joint) L += (R -= (L >> 1)); crc = (crc * 3 + L) * 3 + R; + if(s->extra_bits){ + L <<= s->extra_bits; + R <<= s->extra_bits; + + if(s->got_extra){ + L |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff);
the second & can be avoided if the >> is unsigned Something like (uin32_t)L >> 16 (or (unsigned)L) ?
I think it is more obfuscated and it's the job of the compilo to remove it if unneeded. But if it is needed to have this patch applied, I will redo it. -- fenrir
On Fri, May 01, 2009 at 04:05:54PM +0200, Laurent Aimar wrote:
Hi,
On Fri, May 01, 2009, Michael Niedermayer wrote:
On Fri, May 01, 2009 at 01:49:37PM +0200, Laurent Aimar wrote:
The attached patch completes the support for files with INT32INFO chunk with non zero sent_bits.
[...]
@@ -344,6 +348,7 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d int A, B, L, L2, R, R2, bit; int pos = 0; uint32_t crc = 0xFFFFFFFF; + uint32_t crc_extra = 0xFFFFFFFF; int16_t *dst16 = dst; int32_t *dst32 = dst;
@@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d if(s->joint) L += (R -= (L >> 1)); crc = (crc * 3 + L) * 3 + R; + if(s->extra_bits){ + L <<= s->extra_bits; + R <<= s->extra_bits; + + if(s->got_extra){ + L |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff);
the second & can be avoided if the >> is unsigned Something like (uin32_t)L >> 16 (or (unsigned)L) ?
(unsigned)L if thats not correct then the int L is likely wrong too
I think it is more obfuscated and it's the job of the compilo to remove it if unneeded.
suggestions for better compilers are welcome ... [...] -- 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
On Fri, May 01, 2009, Michael Niedermayer wrote:
@@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d if(s->joint) L += (R -= (L >> 1)); crc = (crc * 3 + L) * 3 + R; + if(s->extra_bits){ + L <<= s->extra_bits; + R <<= s->extra_bits; + + if(s->got_extra){ + L |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff);
the second & can be avoided if the >> is unsigned Something like (uin32_t)L >> 16 (or (unsigned)L) ?
(unsigned)L if thats not correct then the int L is likely wrong too
I think it is more obfuscated and it's the job of the compilo to remove it if unneeded.
suggestions for better compilers are welcome ... For the record, gcc -S (Debian 4.3.3-3) does generate the exact same assembly output files using either the 0xffff mask or the unsigned cast...
-- fenrir
Laurent Aimar <fenrir at videolan.org> writes:
On Fri, May 01, 2009, Michael Niedermayer wrote:
@@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d if(s->joint) L += (R -= (L >> 1)); crc = (crc * 3 + L) * 3 + R; + if(s->extra_bits){ + L <<= s->extra_bits; + R <<= s->extra_bits; + + if(s->got_extra){ + L |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff);
the second & can be avoided if the >> is unsigned Something like (uin32_t)L >> 16 (or (unsigned)L) ?
(unsigned)L if thats not correct then the int L is likely wrong too
I think it is more obfuscated and it's the job of the compilo to remove it if unneeded.
suggestions for better compilers are welcome ... For the record, gcc -S (Debian 4.3.3-3) does generate the exact same assembly output files using either the 0xffff mask or the unsigned cast...
On all architectures? I prefer not to rely on compiler optimisations. -- M?ns Rullg?rd mans at mansr.com
On Fri, May 01, 2009 at 01:49:37PM +0200, Laurent Aimar wrote:
The attached patch completes the support for files with INT32INFO chunk with non zero sent_bits.
-- fenrir
diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c index f31402d..0aded35 100644 --- a/libavcodec/wavpack.c +++ b/libavcodec/wavpack.c @@ -57,7 +57,7 @@ enum WP_ID{ WP_ID_INT32INFO, WP_ID_DATA, WP_ID_CORR, - WP_ID_FLT, + WP_ID_EXTRA,
Well, WavPack documentation says it's "special extended bitstream for floating point data or integers > 24 bit". While old name was not entirely correct, new one is even more ambiguous. Please try to give it a better name than mere "extra".
WP_ID_CHANINFO };
@@ -85,11 +85,15 @@ typedef struct WavpackContext { int joint; uint32_t CRC; GetBitContext gb; + int got_extra; + uint32_t crc_extra;
Again, maybe got_wvx and crc_wvx instead?
+ GetBitContext gb_extra; int data_size; // in bits int samples; int terms; Decorr decorr[MAX_TERMS]; int zero, one, zeroes; + int extra_bits; int and, or, shift; int post_shift; int hybrid, hybrid_bitrate; @@ -344,6 +348,7 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d int A, B, L, L2, R, R2, bit; int pos = 0; uint32_t crc = 0xFFFFFFFF; + uint32_t crc_extra = 0xFFFFFFFF; int16_t *dst16 = dst; int32_t *dst32 = dst;
@@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d if(s->joint) L += (R -= (L >> 1)); crc = (crc * 3 + L) * 3 + R; + if(s->extra_bits){ + L <<= s->extra_bits; + R <<= s->extra_bits; + + if(s->got_extra){ + L |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff); + + R |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (R&0xffff) * 3 + ((R>>16)&0xffff);
+ ((unsigned)R >> 16), I think (and Michael has pointed that out already).
+ } + } bit = (L & s->and) | s->or; if(hires) *dst32++ = (((L + bit) << s->shift) - bit) << s->post_shift; @@ -441,6 +458,10 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d av_log(s->avctx, AV_LOG_ERROR, "CRC error\n"); return -1; } + if(s->got_extra && crc_extra != s->crc_extra){ + av_log(s->avctx, AV_LOG_ERROR, "CRC extra error\n"); + return -1; + }
I read it as not very good way to say "An additional CRC error" and suspect it should mean something else. [monaural decoding case skipped]
@@ -533,7 +568,9 @@ static int wavpack_decode_frame(AVCodecContext *avctx,
memset(s->decorr, 0, MAX_TERMS * sizeof(Decorr)); memset(s->ch, 0, sizeof(s->ch)); + s->extra_bits = 0; s->and = s->or = s->shift = 0; + s->got_extra = 0;
s->samples = AV_RL32(buf); buf += 4; if(!s->samples){ @@ -701,7 +738,7 @@ static int wavpack_decode_frame(AVCodecContext *avctx, continue; } if(buf[0]) - s->post_shift = buf[0]; + s->extra_bits = buf[0]; else if(buf[1]) s->shift = buf[1]; else if(buf[2]){ @@ -719,6 +756,12 @@ static int wavpack_decode_frame(AVCodecContext *avctx, buf += size; got_bs = 1; break; + case WP_ID_EXTRA: + init_get_bits(&s->gb_extra, buf, size * 8); + s->crc_extra = get_bits_long( &s->gb_extra, 32);
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check.
+ buf += size; + s->got_extra = 1; + break; default: buf += size; }
On Fri, May 01, 2009, Kostya wrote:
diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c index f31402d..0aded35 100644 --- a/libavcodec/wavpack.c +++ b/libavcodec/wavpack.c @@ -57,7 +57,7 @@ enum WP_ID{ WP_ID_INT32INFO, WP_ID_DATA, WP_ID_CORR, - WP_ID_FLT, + WP_ID_EXTRA,
Well, WavPack documentation says it's "special extended bitstream for floating point data or integers > 24 bit". While old name was not entirely correct, new one is even more ambiguous. Please try to give it a better name than mere "extra". I have no good idea for a better name, I am open to suggestion. WP_ID_EXTRA_BITS, WP_ID_EXTENDED, WP_ID_EXTENSION, WP_ID_SENT_BITS ...
WP_ID_CHANINFO };
@@ -85,11 +85,15 @@ typedef struct WavpackContext { int joint; uint32_t CRC; GetBitContext gb; + int got_extra; + uint32_t crc_extra;
Again, maybe got_wvx and crc_wvx instead?
Well, wv is useless, we know we are in wavpack module I think. Anyway, it should probably have the same name than for the WP_ID_?
+ GetBitContext gb_extra; int data_size; // in bits int samples; int terms; Decorr decorr[MAX_TERMS]; int zero, one, zeroes; + int extra_bits; int and, or, shift; int post_shift; int hybrid, hybrid_bitrate; @@ -344,6 +348,7 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d int A, B, L, L2, R, R2, bit; int pos = 0; uint32_t crc = 0xFFFFFFFF; + uint32_t crc_extra = 0xFFFFFFFF; int16_t *dst16 = dst; int32_t *dst32 = dst;
@@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d if(s->joint) L += (R -= (L >> 1)); crc = (crc * 3 + L) * 3 + R; + if(s->extra_bits){ + L <<= s->extra_bits; + R <<= s->extra_bits; + + if(s->got_extra){ + L |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff); + + R |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (R&0xffff) * 3 + ((R>>16)&0xffff);
+ ((unsigned)R >> 16), I think (and Michael has pointed that out already). Arf, you seem to really want it, I will do it then.
+ } + } bit = (L & s->and) | s->or; if(hires) *dst32++ = (((L + bit) << s->shift) - bit) << s->post_shift; @@ -441,6 +458,10 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d av_log(s->avctx, AV_LOG_ERROR, "CRC error\n"); return -1; } + if(s->got_extra && crc_extra != s->crc_extra){ + av_log(s->avctx, AV_LOG_ERROR, "CRC extra error\n"); + return -1; + }
I read it as not very good way to say "An additional CRC error" and suspect it should mean something else. Well your proposed message error is better, I will include it.
@@ -533,7 +568,9 @@ static int wavpack_decode_frame(AVCodecContext *avctx,
memset(s->decorr, 0, MAX_TERMS * sizeof(Decorr)); memset(s->ch, 0, sizeof(s->ch)); + s->extra_bits = 0; s->and = s->or = s->shift = 0; + s->got_extra = 0;
s->samples = AV_RL32(buf); buf += 4; if(!s->samples){ @@ -701,7 +738,7 @@ static int wavpack_decode_frame(AVCodecContext *avctx, continue; } if(buf[0]) - s->post_shift = buf[0]; + s->extra_bits = buf[0]; else if(buf[1]) s->shift = buf[1]; else if(buf[2]){ @@ -719,6 +756,12 @@ static int wavpack_decode_frame(AVCodecContext *avctx, buf += size; got_bs = 1; break; + case WP_ID_EXTRA: + init_get_bits(&s->gb_extra, buf, size * 8); + s->crc_extra = get_bits_long( &s->gb_extra, 32);
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
-- fenrir
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
On Fri, May 01, 2009, Kostya wrote:
diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c index f31402d..0aded35 100644 --- a/libavcodec/wavpack.c +++ b/libavcodec/wavpack.c @@ -57,7 +57,7 @@ enum WP_ID{ WP_ID_INT32INFO, WP_ID_DATA, WP_ID_CORR, - WP_ID_FLT, + WP_ID_EXTRA,
Well, WavPack documentation says it's "special extended bitstream for floating point data or integers > 24 bit". While old name was not entirely correct, new one is even more ambiguous. Please try to give it a better name than mere "extra". I have no good idea for a better name, I am open to suggestion. WP_ID_EXTRA_BITS, WP_ID_EXTENDED, WP_ID_EXTENSION, WP_ID_SENT_BITS ...
I'm no good at names either but WP_ID_EXTRA_BITS seems better than others
WP_ID_CHANINFO };
@@ -85,11 +85,15 @@ typedef struct WavpackContext { int joint; uint32_t CRC; GetBitContext gb; + int got_extra; + uint32_t crc_extra;
Again, maybe got_wvx and crc_wvx instead? Well, wv is useless, we know we are in wavpack module I think. Anyway, it should probably have the same name than for the WP_ID_?
yes unless it's too long
+ GetBitContext gb_extra; int data_size; // in bits int samples; int terms; Decorr decorr[MAX_TERMS]; int zero, one, zeroes; + int extra_bits; int and, or, shift; int post_shift; int hybrid, hybrid_bitrate; @@ -344,6 +348,7 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d int A, B, L, L2, R, R2, bit; int pos = 0; uint32_t crc = 0xFFFFFFFF; + uint32_t crc_extra = 0xFFFFFFFF; int16_t *dst16 = dst; int32_t *dst32 = dst;
@@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d if(s->joint) L += (R -= (L >> 1)); crc = (crc * 3 + L) * 3 + R; + if(s->extra_bits){ + L <<= s->extra_bits; + R <<= s->extra_bits; + + if(s->got_extra){ + L |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff); + + R |= get_bits(&s->gb_extra, s->extra_bits); + crc_extra = crc_extra * 9 + (R&0xffff) * 3 + ((R>>16)&0xffff);
+ ((unsigned)R >> 16), I think (and Michael has pointed that out already). Arf, you seem to really want it, I will do it then.
It's not that hard, n'est-ce pas?
+ } + } bit = (L & s->and) | s->or; if(hires) *dst32++ = (((L + bit) << s->shift) - bit) << s->post_shift; @@ -441,6 +458,10 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d av_log(s->avctx, AV_LOG_ERROR, "CRC error\n"); return -1; } + if(s->got_extra && crc_extra != s->crc_extra){ + av_log(s->avctx, AV_LOG_ERROR, "CRC extra error\n"); + return -1; + }
I read it as not very good way to say "An additional CRC error" and suspect it should mean something else. Well your proposed message error is better, I will include it.
"Extra bits CRC error" may be clearer though, pick any ;)
@@ -533,7 +568,9 @@ static int wavpack_decode_frame(AVCodecContext *avctx,
memset(s->decorr, 0, MAX_TERMS * sizeof(Decorr)); memset(s->ch, 0, sizeof(s->ch)); + s->extra_bits = 0; s->and = s->or = s->shift = 0; + s->got_extra = 0;
s->samples = AV_RL32(buf); buf += 4; if(!s->samples){ @@ -701,7 +738,7 @@ static int wavpack_decode_frame(AVCodecContext *avctx, continue; } if(buf[0]) - s->post_shift = buf[0]; + s->extra_bits = buf[0]; else if(buf[1]) s->shift = buf[1]; else if(buf[2]){ @@ -719,6 +756,12 @@ static int wavpack_decode_frame(AVCodecContext *avctx, buf += size; got_bs = 1; break; + case WP_ID_EXTRA: + init_get_bits(&s->gb_extra, buf, size * 8); + s->crc_extra = get_bits_long( &s->gb_extra, 32);
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
-- fenrir
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? Is that why ALAC is so horribly broken in that regard? Where to we need to put the large over-size warning sign so we do not get more of a mess based on wrong assumptions?
On Fri, May 1, 2009 at 11:58 PM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. ?Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? Is that why ALAC is so horribly broken in that regard?
Very likely. All input is done *solely* using get_bits, so adding checks requires some hacks. -- Regards, Jai
On Sat, May 02, 2009 at 12:02:00AM +0530, Jai Menon wrote:
On Fri, May 1, 2009 at 11:58 PM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. ?Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? Is that why ALAC is so horribly broken in that regard?
Very likely. All input is done *solely* using get_bits, so adding checks requires some hacks.
I don't know what you consider hacks, but attached patch e.g. fixes issue 758 (I sent it a long time ago to roundup and just tried again - it still seems to refuse any mail with a patch). The uncompressed case still needs to be fixed/optimized (and even the compressed case could be improved) but I can't see what would be so horrible about it that it would be better to have crashes. Actually I don't consider it bad enough to explain why it wasn't done in the first place.
On Sat, May 02, 2009 at 02:56:02PM +0200, Reimar D?ffinger wrote:
On Sat, May 02, 2009 at 12:02:00AM +0530, Jai Menon wrote:
On Fri, May 1, 2009 at 11:58 PM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. ?Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? Is that why ALAC is so horribly broken in that regard?
Very likely. All input is done *solely* using get_bits, so adding checks requires some hacks.
I don't know what you consider hacks, but attached patch e.g. fixes issue 758 (I sent it a long time ago to roundup and just tried again - it still seems to refuse any mail with a patch). The uncompressed case still needs to be fixed/optimized (and even the compressed case could be improved) but I can't see what would be so horrible about it that it would be better to have crashes. Actually I don't consider it bad enough to explain why it wasn't done in the first place.
Index: libavcodec/alac.c =================================================================== --- libavcodec/alac.c (revision 18377) +++ libavcodec/alac.c (working copy) @@ -183,6 +183,9 @@ /* standard rice encoding */ int k; /* size of extra bits */
+ if (get_bits_count(&alac->gb) >= alac->gb.size_in_bits) + break; + /* read k, that is bits as is */ k = av_log2((history >> 9) + 3); x= decode_scalar(&alac->gb, k, rice_kmodifier, readsamplesize); @@ -507,6 +510,8 @@ av_log(avctx, AV_LOG_ERROR, "FIXME: unimplemented, unhandling of wasted_bytes\n");
for (chan = 0; chan < channels; chan++) { + if (get_bits_count(&alac->gb) >= alac->gb.size_in_bits) + break; bastardized_rice_decompress(alac, alac->predicterror_buffer[chan], outputsamples, @@ -539,6 +544,8 @@ /* not compressed, easy case */ int i, chan; for (i = 0; i < outputsamples; i++) + if (get_bits_count(&alac->gb) >= alac->gb.size_in_bits) + break; for (chan = 0; chan < channels; chan++) { int32_t audiobits;
ALAC decoder is still without a single range check, is someone actually maintaining it? I not I intend to apply this soon, updated to use get_bits_left though.
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? There is a CRC anyway, but I added the check to ensure detecting error as soon as possible (and to avoid the possibility of a false positive CRC in this
On Fri, May 01, 2009, Reimar D?ffinger wrote: particuliar case). -- fenrir
On Sat, May 02, 2009 at 12:07:42AM +0200, Laurent Aimar wrote:
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? There is a CRC anyway, but I added the check to ensure detecting error as soon as possible (and to avoid the possibility of a false positive CRC in this
On Fri, May 01, 2009, Reimar D?ffinger wrote: particuliar case).
I can't see how the CRC would avoid crashes. And actually I am not 100% sure the committed code avoids crashes if the extrabits buffer is too small (though it is possible it can't be too small because the compressed data comes after it).
On Sat, May 02, 2009 at 02:47:13PM +0200, Reimar D?ffinger wrote:
On Sat, May 02, 2009 at 12:07:42AM +0200, Laurent Aimar wrote:
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? There is a CRC anyway, but I added the check to ensure detecting error as soon as possible (and to avoid the possibility of a false positive CRC in this
On Fri, May 01, 2009, Reimar D?ffinger wrote: particuliar case).
I can't see how the CRC would avoid crashes. And actually I am not 100% sure the committed code avoids crashes if the extrabits buffer is too small (though it is possible it can't be too small because the compressed data comes after it).
It should not crash in this case at all but I prefer early error detection system *g*.
On Sat, May 02, 2009 at 12:07:42AM +0200, Laurent Aimar wrote:
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this looks fine too. Oh, and please add block size check. Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? There is a CRC anyway, but I added the check to ensure detecting error as soon as possible (and to avoid the possibility of a false positive CRC in this
On Fri, May 01, 2009, Reimar D?ffinger wrote: particuliar case).
I can't see how the CRC would avoid crashes. And actually I am not 100% sure the committed code avoids crashes if the extrabits buffer is too small (though it is possible it can't be too small because the compressed data comes after it). I thought that the bitstream layer ensured that no read could go beyond the
On Sat, May 02, 2009, Reimar D?ffinger wrote: provided buffer. Looking at get_bits.h it does not seems to, so I have attached a patch that check that the provided buffer is wide enought. Regards, -- fenrir
On Sat, May 02, 2009 at 07:10:28PM +0200, Laurent Aimar wrote:
On Sat, May 02, 2009 at 12:07:42AM +0200, Laurent Aimar wrote:
On Fri, May 01, 2009 at 07:39:37PM +0300, Kostya wrote:
On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
> I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this > looks fine too. Oh, and please add block size check. Using the bitstream reader here seems simpler to me, and additionnaly avoid the need of buffer size checks ;)
It's not - if size <=4 (as WavPack source code says), it should be treated as error, so check is needed anyway (not for bits reading though).
Huh? Where does that idea that using the bitstream reader means you can skip the size checks come from? There is a CRC anyway, but I added the check to ensure detecting error as soon as possible (and to avoid the possibility of a false positive CRC in this
On Fri, May 01, 2009, Reimar D?ffinger wrote: particuliar case).
I can't see how the CRC would avoid crashes. And actually I am not 100% sure the committed code avoids crashes if the extrabits buffer is too small (though it is possible it can't be too small because the compressed data comes after it). I thought that the bitstream layer ensured that no read could go beyond the
On Sat, May 02, 2009, Reimar D?ffinger wrote: provided buffer. Looking at get_bits.h it does not seems to, so I have attached a patch that check that the provided buffer is wide enought.
Why not check it once during initialization? Decoder always read extra_bits*channels*samples+32, doesn't it?
Regards,
-- fenrir
On Sat, May 02, 2009, Kostya wrote:
I thought that the bitstream layer ensured that no read could go beyond the provided buffer. Looking at get_bits.h it does not seems to, so I have attached a patch that check that the provided buffer is wide enought.
Why not check it once during initialization? Decoder always read extra_bits*channels*samples+32, doesn't it? From wv docs, it does not seems that the order of the chunks is imposed, so I cannot suppose that INT32INFO (needed to know the number of bits) is received before EXTRA_BITS.
-- fenrir
On Sat, May 02, 2009 at 08:03:17PM +0200, Laurent Aimar wrote:
On Sat, May 02, 2009, Kostya wrote:
I thought that the bitstream layer ensured that no read could go beyond the provided buffer. Looking at get_bits.h it does not seems to, so I have attached a patch that check that the provided buffer is wide enought.
Why not check it once during initialization? Decoder always read extra_bits*channels*samples+32, doesn't it? From wv docs, it does not seems that the order of the chunks is imposed, so I cannot suppose that INT32INFO (needed to know the number of bits) is received before EXTRA_BITS.
On the second thought this check is not really needed - we may be more or less sure that we got right CRC so wrong bits will be catched by it. Checking it for each sample is an overkill IMO. You can always check that after all chunks are parsed but before real decode has started but I'd like to see those efforts spent on finishing float WavPack support instead.
-- fenrir
On Sat, May 02, 2009, Kostya wrote:
On Sat, May 02, 2009 at 08:03:17PM +0200, Laurent Aimar wrote:
On Sat, May 02, 2009, Kostya wrote:
I thought that the bitstream layer ensured that no read could go beyond the provided buffer. Looking at get_bits.h it does not seems to, so I have attached a patch that check that the provided buffer is wide enought.
Why not check it once during initialization? Decoder always read extra_bits*channels*samples+32, doesn't it? From wv docs, it does not seems that the order of the chunks is imposed, so I cannot suppose that INT32INFO (needed to know the number of bits) is received before EXTRA_BITS.
On the second thought this check is not really needed - we may be more or less sure that we got right CRC so wrong bits will be catched by it. That's not the issue, get_bits cannot be called if the buffer is not wide enough (at least it is my understanding). Checking it for each sample is an overkill IMO. It is not what I do, the check is done only once per input packet.
You can always check that after all chunks are parsed but before real decode has started but It is done like that, sorry if I wasn't clear, I just meant that I had to wait for all chunks to be parsed as I cannot check it when I received the EXTRA_BITS one.
-- fenrir
On Sat, May 02, 2009 at 08:42:41PM +0200, Laurent Aimar wrote:
On Sat, May 02, 2009, Kostya wrote:
On Sat, May 02, 2009 at 08:03:17PM +0200, Laurent Aimar wrote:
On Sat, May 02, 2009, Kostya wrote:
I thought that the bitstream layer ensured that no read could go beyond the provided buffer. Looking at get_bits.h it does not seems to, so I have attached a patch that check that the provided buffer is wide enought.
Why not check it once during initialization? Decoder always read extra_bits*channels*samples+32, doesn't it? From wv docs, it does not seems that the order of the chunks is imposed, so I cannot suppose that INT32INFO (needed to know the number of bits) is received before EXTRA_BITS.
On the second thought this check is not really needed - we may be more or less sure that we got right CRC so wrong bits will be catched by it. That's not the issue, get_bits cannot be called if the buffer is not wide enough (at least it is my understanding). Checking it for each sample is an overkill IMO. It is not what I do, the check is done only once per input packet.
You can always check that after all chunks are parsed but before real decode has started but It is done like that, sorry if I wasn't clear, I just meant that I had to wait for all chunks to be parsed as I cannot check it when I received the EXTRA_BITS one.
It's get_bits_count() call that got me confused (so I thought it is called regularly during samples decoding). Applied.
-- fenrir
Hi, Attached is a new version of the patch that should address all the issues. -- fenrir
participants (6)
-
fenrir＠videolan.org -
jmenon86＠gmail.com -
kostya.shishkov＠gmail.com -
mans＠mansr.com -
michaelni＠gmx.at -
Reimar.Doeffinger＠gmx.de