On Mon, Dec 12, 2011 at 07:35:44AM -0800, Ronald S. Bultje wrote: > Hi guys, > > the idea of this patch is very simple, and has existed in Chrome in a slightly > different form for quite a while: for each call to get_bits() and related > functions, check for overreads before advancing the frame pointer. > > This protects against overreads in most decoders except those that use custom > bitreaders, like VP8 (which already does this), CABAC (which I'll do later) > and > a few more. Speed depends on how many bits are read per frame, but ranges from > a 1-10% loss on low to ultra-high bitrate CAVLC H264 streams, to unnoticeable > losses of <0.1% on e.g. VC1 (because the DSP is less optimized, so the > bitreader overhead is relatively lower). > > Design is that there's a configure option (currently default-off) to enable > the > "safe" bitstream reader, and individual decoders that do bitstream checks > themselves can turn it off on a per-decoder basis.
I think that with your patch, get_bits_count() will never be negative and this will creates issues with some decoders. You may want to read the following thread "[PATCH] Checked get_bits.h functions to prevent overread" (libav-devel/ffmpeg-devel) to see the associated discussions/arguments. I attached the version of a similar patch that I came up with after taking thoses arguments into account. (I never created the configue part). Regards, -- fenrir
>From af1e5c3a989291a1e78b1f0dbeabf76f5e8bdfc6 Mon Sep 17 00:00:00 2001 From: Laurent Aimar <[email protected]> Date: Fri, 9 Sep 2011 00:52:36 +0200 Subject: [PATCH] Prevent by default for overread in get_bits.h functions. It can be disabled (at compilation time so without any performance loss) for decoder that check for overreads by themselves by defining UNCHECKED_BITSTREAM_READER before including get_bits.h Checks for A32_BITSTREAM_READER are not yet implemented. --- libavcodec/get_bits.h | 34 ++++++++++++++++++++++++++++++---- 1 files changed, 30 insertions(+), 4 deletions(-) diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h index d2ae345..687169b 100644 --- a/libavcodec/get_bits.h +++ b/libavcodec/get_bits.h @@ -35,6 +35,8 @@ #include "libavutil/log.h" #include "mathops.h" +/* You can define UNCHECKED_BITSTREAM_READER before including this file to + * avoid the penalty cost of checking for overread */ #if defined(ALT_BITSTREAM_READER_LE) && !defined(ALT_BITSTREAM_READER) # define ALT_BITSTREAM_READER #endif @@ -127,18 +129,37 @@ for examples see get_bits, show_bits, skip_bits, get_vlc # define OPEN_READER(name, gb) \ unsigned int name##_index = (gb)->index; \ + unsigned int av_unused name##_size_in_bits = (gb)->size_in_bits; \ unsigned int av_unused name##_cache = 0 # define CLOSE_READER(name, gb) (gb)->index = name##_index # ifdef ALT_BITSTREAM_READER_LE -# define UPDATE_CACHE(name, gb) \ - name##_cache = AV_RL32(((const uint8_t *)(gb)->buffer)+(name##_index>>3)) >> (name##_index&0x07) +# ifdef UNCHECKED_BITSTREAM_READER +# define UPDATE_CACHE(name, gb) \ + name##_cache = AV_RL32(((const uint8_t *)(gb)->buffer)+(name##_index>>3)) >> (name##_index&0x07) +# else +# define UPDATE_CACHE(name, gb) do { \ + if (name##_index < name##_size_in_bits) \ + name##_cache = AV_RL32(((const uint8_t *)(gb)->buffer)+(name##_index>>3)) >> (name##_index&0x07); \ + else \ + name##_cache = 0; \ + } while (0) +# endif # define SKIP_CACHE(name, gb, num) name##_cache >>= (num) # else -# define UPDATE_CACHE(name, gb) \ - name##_cache = AV_RB32(((const uint8_t *)(gb)->buffer)+(name##_index>>3)) << (name##_index&0x07) +# ifdef UNCHECKED_BITSTREAM_READER +# define UPDATE_CACHE(name, gb) \ + name##_cache = AV_RB32(((const uint8_t *)(gb)->buffer)+(name##_index>>3)) << (name##_index&0x07) +# else +# define UPDATE_CACHE(name, gb) do {\ + if (name##_index < name##_size_in_bits) \ + name##_cache = AV_RB32(((const uint8_t *)(gb)->buffer)+(name##_index>>3)) << (name##_index&0x07); \ + else \ + name##_cache = 0; \ + } while (0) +# endif # define SKIP_CACHE(name, gb, num) name##_cache <<= (num) # endif @@ -303,7 +324,12 @@ static inline void skip_bits(GetBitContext *s, int n){ static inline unsigned int get_bits1(GetBitContext *s){ #ifdef ALT_BITSTREAM_READER unsigned int index = s->index; +#ifdef UNCHECKED_BITSTREAM_READER uint8_t result = s->buffer[index>>3]; +#else + uint8_t result = index < s->size_in_bits ? s->buffer[index>>3] : 0; +#endif + #ifdef ALT_BITSTREAM_READER_LE result >>= index & 7; result &= 1; -- 1.7.2.5
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
