[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
Signed-off-by: Paul B Mahol --- libavcodec/get_bits.h | 218 +- libavcodec/golomb.h | 151 + 2 files changed, 367 insertions(+), 2 deletions(-) diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h index 56ef5f0cbe..58ebb64656 100644 --- a/libavcodec/get_bits.h +++ b/libavcodec/get_bits.h @@ -1,5 +1,6 @@ /* - * copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2016 Alexandra Hájková * * This file is part of FFmpeg. * @@ -55,11 +56,19 @@ typedef struct GetBitContext { const uint8_t *buffer, *buffer_end; +#ifdef CACHED_BITSTREAM_READER +uint64_t cache; +unsigned bits_left; +#endif int index; int size_in_bits; int size_in_bits_plus8; } GetBitContext; +static inline unsigned int get_bits(GetBitContext *s, int n); +static inline void skip_bits(GetBitContext *s, int n); +static inline unsigned int show_bits(GetBitContext *s, int n); + /* Bitstream reader API docs: * name * arbitrary name which is used as prefix for the internal variables @@ -107,12 +116,16 @@ typedef struct GetBitContext { * For examples see get_bits, show_bits, skip_bits, get_vlc. */ -#ifdef LONG_BITSTREAM_READER +#ifdef CACHED_BITSTREAM_READER +# define MIN_CACHE_BITS 64 +#elif defined LONG_BITSTREAM_READER # define MIN_CACHE_BITS 32 #else # define MIN_CACHE_BITS 25 #endif +#ifndef CACHED_BITSTREAM_READER + #define OPEN_READER_NOSIZE(name, gb)\ unsigned int name ## _index = (gb)->index; \ unsigned int av_unused name ## _cache @@ -197,11 +210,75 @@ typedef struct GetBitContext { #define GET_CACHE(name, gb) ((uint32_t) name ## _cache) +#endif + static inline int get_bits_count(const GetBitContext *s) { +#ifdef CACHED_BITSTREAM_READER +return s->index - s->bits_left; +#else return s->index; +#endif +} + +#ifdef CACHED_BITSTREAM_READER +static inline void refill_32(GetBitContext *s) +{ +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << s->bits_left | s->cache; +#else +s->cache = s->cache | (uint64_t)AV_RB32(s->buffer + (s->index >> 3)) << (32 - s->bits_left); +#endif +s->index += 32; +s->bits_left += 32; } +static inline void refill_64(GetBitContext *s) +{ +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = AV_RL64(s->buffer + (s->index >> 3)); +#else +s->cache = AV_RB64(s->buffer + (s->index >> 3)); +#endif +s->index += 64; +s->bits_left = 64; +} + +static inline uint64_t get_val(GetBitContext *s, unsigned n, int is_le) +{ +uint64_t ret; +av_assert2(n>0 && n<=63); +if (is_le) { +ret = s->cache & ((UINT64_C(1) << n) - 1); +s->cache >>= n; +} else { +ret = s->cache >> (64 - n); +s->cache <<= n; +} +s->bits_left -= n; +return ret; +} + +static inline unsigned show_val(const GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +return s->cache & ((UINT64_C(1) << n) - 1); +#else +return s->cache >> (64 - n); +#endif +} +#endif + /** * Skips the specified number of bits. * @param n the number of bits to skip, @@ -211,13 +288,29 @@ static inline int get_bits_count(const GetBitContext *s) */ static inline void skip_bits_long(GetBitContext *s, int n) { +#ifdef CACHED_BITSTREAM_READER +skip_bits(s, n); +#else #if UNCHECKED_BITSTREAM_READER s->index += n; #else s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); #endif +#endif } +#ifdef CACHED_BITSTREAM_READER +static inline void skip_remaining(GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +s->cache >>= n; +#else +s->cache <<= n; +#endif +s->bits_left -= n; +} +#endif + /** * Read MPEG-1 dc-style VLC (sign bit + mantissa with no MSB). * if MSB not set it is negative @@ -225,6 +318,13 @@ static inline void skip_bits_long(GetBitContext *s, int n) */ static inline int get_xbits(GetBitContext *s, int n) { +#ifdef CACHED_BITSTREAM_READER +int32_t cache = show_bits(s, 32); +int sign = ~cache >> 31; +skip_remaining(s, n); + +return uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign; +#else register int sign; register int32_t cache; OPEN_READER(re, s); @@ -235,8 +335,10 @@ static inline int get_xbits(GetBitContext *s, int n) LAST_SKIP_BITS(re, s, n); CLOSE_READER(re, s); return (NEG_USR32(sign ^ cache, n) ^ sign) - sign; +#endif } +#ifndef CACHED_BITSTREAM_READER static inline int get_xbits_le(GetBitContext *s, int n) { register int sign; @@ -250,16 +352,22 @@ static inline int get_xbits_le(GetBitContext *s, int n) CLOSE_READER(re, s); return (zero_extend(si
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Tue, Apr 03, 2018 at 01:38:12PM +0200, Paul B Mahol wrote: [...] > > -static inline void skip_bits_long(GetBitContext *s, int n) > +static inline void refill_32(GetBitContext *s) > { > -#if UNCHECKED_BITSTREAM_READER > -s->index += n; > +#ifdef CACHED_BITSTREAM_READER > +#if !UNCHECKED_BITSTREAM_READER > +if (s->index >> 3 >= s->buffer_end - s->buffer) > +return; > +#endif > + > +#ifdef BITSTREAM_READER_LE > +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << > s->bits_left | s->cache; > #else > -s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); > +s->cache = s->cache | (uint64_t)AV_RB32(s->buffer + (s->index >> > 3)) << (32 - s->bits_left); > +#endif > +s->index += 32; > +s->bits_left += 32; > +#endif > +} please split the movement of code into a seperate patch, this is unreadable > + > +static inline void refill_64(GetBitContext *s) > +{ > +#ifdef CACHED_BITSTREAM_READER > +#if !UNCHECKED_BITSTREAM_READER > +if (s->index >> 3 >= s->buffer_end - s->buffer) > +return; > +#endif > + > +#ifdef BITSTREAM_READER_LE > +s->cache = AV_RL64(s->buffer + (s->index >> 3)); > +#else > +s->cache = AV_RB64(s->buffer + (s->index >> 3)); > +#endif > +s->index += 64; > +s->bits_left = 64; > +#endif > +} all uses of refill_64 are under CACHED_BITSTREAM_READER, so there should not be a need for an empty function and the ifdef can be then merged with the next, simplifying the code > + > +#ifdef CACHED_BITSTREAM_READER > +static inline uint64_t get_val(GetBitContext *s, unsigned n, int is_le) > +{ > +uint64_t ret; > +av_assert2(n>0 && n<=63); > +if (is_le) { > +ret = s->cache & ((UINT64_C(1) << n) - 1); > +s->cache >>= n; > +} else { > +ret = s->cache >> (64 - n); > +s->cache <<= n; > +} > +s->bits_left -= n; > +return ret; > +} > +#endif > + > +#ifdef CACHED_BITSTREAM_READER these 2 ifdef CACHED_BITSTREAM_READER can be merged > +static inline unsigned show_val(const GetBitContext *s, unsigned n) > +{ > +#ifdef BITSTREAM_READER_LE > +return s->cache & ((UINT64_C(1) << n) - 1); > +#else > +return s->cache >> (64 - n); > +#endif > +} > +#endif > + > +/** > + * Show 1-25 bits. > + */ > +static inline unsigned int show_bits(GetBitContext *s, int n) > +{ > +register int tmp; > +#ifdef CACHED_BITSTREAM_READER > +if (n > s->bits_left) > +refill_32(s); > + > +tmp = show_val(s, n); > +#else > +OPEN_READER_NOSIZE(re, s); > +av_assert2(n>0 && n<=25); > +UPDATE_CACHE(re, s); > +tmp = SHOW_UBITS(re, s, n); > #endif > +return tmp; > } This is more moved code in which it is impossible to see what is changed [...] > > -static inline int get_sbits(GetBitContext *s, int n) > +/** > + * Read 1-25 bits. > + */ > +static inline unsigned int get_bits(GetBitContext *s, int n) also unreadable and confusing, the code moves make these matchup incorrectly > { > register int tmp; > +#ifdef CACHED_BITSTREAM_READER > + > +av_assert2(n>0 && n<=32); > +if (n > s->bits_left) { > +refill_32(s); > +if (s->bits_left < 32) > +s->bits_left = n; > +} > + > +#ifdef BITSTREAM_READER_LE > +tmp = get_val(s, n, 1); > +#else > +tmp = get_val(s, n, 0); > +#endif > +#else > OPEN_READER(re, s); > av_assert2(n>0 && n<=25); > UPDATE_CACHE(re, s); > -tmp = SHOW_SBITS(re, s, n); > +tmp = SHOW_UBITS(re, s, n); > LAST_SKIP_BITS(re, s, n); > CLOSE_READER(re, s); > +#endif > return tmp; > } > > -/** > - * Read 1-25 bits. > - */ > -static inline unsigned int get_bits(GetBitContext *s, int n) > +static inline int get_sbits(GetBitContext *s, int n) > { > register int tmp; > +#ifdef CACHED_BITSTREAM_READER > +av_assert2(n>0 && n<=25); > +tmp = sign_extend(get_bits(s, n), n); > +#else > OPEN_READER(re, s); > av_assert2(n>0 && n<=25); > UPDATE_CACHE(re, s); > -tmp = SHOW_UBITS(re, s, n); > +tmp = SHOW_SBITS(re, s, n); > LAST_SKIP_BITS(re, s, n); > CLOSE_READER(re, s); > +#endif > return tmp; > } more mismatched functions that are diffed against each other [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Never trust a computer, one day, it may think you are the virus. -- Compn signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
Signed-off-by: Paul B Mahol --- libavcodec/get_bits.h | 268 +- libavcodec/golomb.h | 151 2 files changed, 393 insertions(+), 26 deletions(-) diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h index d7cf286378..64e1643e34 100644 --- a/libavcodec/get_bits.h +++ b/libavcodec/get_bits.h @@ -1,5 +1,6 @@ /* - * copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2016 Alexandra Hájková * * This file is part of FFmpeg. * @@ -55,6 +56,10 @@ typedef struct GetBitContext { const uint8_t *buffer, *buffer_end; +#ifdef CACHED_BITSTREAM_READER +uint64_t cache; +unsigned bits_left; +#endif int index; int size_in_bits; int size_in_bits_plus8; @@ -107,12 +112,16 @@ typedef struct GetBitContext { * For examples see get_bits, show_bits, skip_bits, get_vlc. */ -#ifdef LONG_BITSTREAM_READER +#ifdef CACHED_BITSTREAM_READER +# define MIN_CACHE_BITS 64 +#elif defined LONG_BITSTREAM_READER # define MIN_CACHE_BITS 32 #else # define MIN_CACHE_BITS 25 #endif +#ifndef CACHED_BITSTREAM_READER + #define OPEN_READER_NOSIZE(name, gb)\ unsigned int name ## _index = (gb)->index; \ unsigned int av_unused name ## _cache @@ -197,20 +206,113 @@ typedef struct GetBitContext { #define GET_CACHE(name, gb) ((uint32_t) name ## _cache) +#endif + static inline int get_bits_count(const GetBitContext *s) { +#ifdef CACHED_BITSTREAM_READER +return s->index - s->bits_left; +#else return s->index; +#endif } -static inline void skip_bits_long(GetBitContext *s, int n) +static inline void refill_32(GetBitContext *s) { -#if UNCHECKED_BITSTREAM_READER -s->index += n; +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << s->bits_left | s->cache; #else -s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); +s->cache = s->cache | (uint64_t)AV_RB32(s->buffer + (s->index >> 3)) << (32 - s->bits_left); +#endif +s->index += 32; +s->bits_left += 32; +#endif +} + +static inline void refill_64(GetBitContext *s) +{ +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = AV_RL64(s->buffer + (s->index >> 3)); +#else +s->cache = AV_RB64(s->buffer + (s->index >> 3)); +#endif +s->index += 64; +s->bits_left = 64; +#endif +} + +#ifdef CACHED_BITSTREAM_READER +static inline uint64_t get_val(GetBitContext *s, unsigned n, int is_le) +{ +uint64_t ret; +av_assert2(n>0 && n<=63); +if (is_le) { +ret = s->cache & ((UINT64_C(1) << n) - 1); +s->cache >>= n; +} else { +ret = s->cache >> (64 - n); +s->cache <<= n; +} +s->bits_left -= n; +return ret; +} +#endif + +#ifdef CACHED_BITSTREAM_READER +static inline unsigned show_val(const GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +return s->cache & ((UINT64_C(1) << n) - 1); +#else +return s->cache >> (64 - n); +#endif +} +#endif + +/** + * Show 1-25 bits. + */ +static inline unsigned int show_bits(GetBitContext *s, int n) +{ +register int tmp; +#ifdef CACHED_BITSTREAM_READER +if (n > s->bits_left) +refill_32(s); + +tmp = show_val(s, n); +#else +OPEN_READER_NOSIZE(re, s); +av_assert2(n>0 && n<=25); +UPDATE_CACHE(re, s); +tmp = SHOW_UBITS(re, s, n); #endif +return tmp; } +#ifdef CACHED_BITSTREAM_READER +static inline void skip_remaining(GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +s->cache >>= n; +#else +s->cache <<= n; +#endif +s->bits_left -= n; +} +#endif + /** * Read MPEG-1 dc-style VLC (sign bit + mantissa with no MSB). * if MSB not set it is negative @@ -218,6 +320,13 @@ static inline void skip_bits_long(GetBitContext *s, int n) */ static inline int get_xbits(GetBitContext *s, int n) { +#ifdef CACHED_BITSTREAM_READER +int32_t cache = show_bits(s, 32); +int sign = ~cache >> 31; +skip_remaining(s, n); + +return uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign; +#else register int sign; register int32_t cache; OPEN_READER(re, s); @@ -228,8 +337,10 @@ static inline int get_xbits(GetBitContext *s, int n) LAST_SKIP_BITS(re, s, n); CLOSE_READER(re, s); return (NEG_USR32(sign ^ cache, n) ^ sign) - sign; +#endif } +#ifndef CACHED_BITSTREAM_READER static inline int get_xbits_le(GetBitContext *s, int n) { register int sign; @@ -243,31 +354,53 @@ static inline int get_xbits_le(GetBitContext *s, int n) CLOSE_READER(re, s); return (zero_extend(sign ^ cache, n) ^ sign) - sign; } +#endif -static inline int
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Fri, Jul 14, 2017 at 05:12:25PM +0200, Hendrik Leppkes wrote: > On Fri, Jul 14, 2017 at 4:08 PM, foo86 wrote: > > On Thu, Jul 13, 2017 at 12:27:03PM +0200, Paul B Mahol wrote: > >> +static inline unsigned int get_bits(GetBitContext *s, int n) > >> { > >> +#ifdef CACHED_BITSTREAM_READER > >> +register int tmp = 0; > >> +#ifdef BITSTREAM_READER_LE > >> +uint64_t left = 0; > >> +#endif > >> + > >> +av_assert2(n>0 && n<=32); > >> +if (n > s->bits_left) { > >> +n -= s->bits_left; > >> +#ifdef BITSTREAM_READER_LE > >> +left = s->bits_left; > >> +#endif > >> +tmp = get_val(s, s->bits_left); > > This triggers an assert in get_val() if s->bits_left == 0. > > > >> +refill_32(s); > >> +} > >> + > >> +#ifdef BITSTREAM_READER_LE > >> +tmp = get_val(s, n) << left | tmp; > >> +#else > >> +tmp = get_val(s, n) | tmp << n; > > This causes undefined behavior if n > 30. > > get_bits is only valid until n = 25 in the "non-cached" case, so its > not a problem to impose the same limitation on the cached reader. > In fact, if they are to share the exact same API, it should probably > follow that they also share the same constraints, so that we can do > proper performance comparisons between the two, instead of having to > re-write the using code. Cached bitstream reader currently uses get_bits() to implement get_bits_long(), which means cached get_bits() must support reading values up to 32 bits. I agree however that cached/uncached bistream readers should have the same API contraints. That means cached get_bits_long() should probably have a separate implementation. > > - Hendrik > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Fri, Jul 14, 2017 at 4:08 PM, foo86 wrote: > On Thu, Jul 13, 2017 at 12:27:03PM +0200, Paul B Mahol wrote: >> +static inline unsigned int get_bits(GetBitContext *s, int n) >> { >> +#ifdef CACHED_BITSTREAM_READER >> +register int tmp = 0; >> +#ifdef BITSTREAM_READER_LE >> +uint64_t left = 0; >> +#endif >> + >> +av_assert2(n>0 && n<=32); >> +if (n > s->bits_left) { >> +n -= s->bits_left; >> +#ifdef BITSTREAM_READER_LE >> +left = s->bits_left; >> +#endif >> +tmp = get_val(s, s->bits_left); > This triggers an assert in get_val() if s->bits_left == 0. > >> +refill_32(s); >> +} >> + >> +#ifdef BITSTREAM_READER_LE >> +tmp = get_val(s, n) << left | tmp; >> +#else >> +tmp = get_val(s, n) | tmp << n; > This causes undefined behavior if n > 30. get_bits is only valid until n = 25 in the "non-cached" case, so its not a problem to impose the same limitation on the cached reader. In fact, if they are to share the exact same API, it should probably follow that they also share the same constraints, so that we can do proper performance comparisons between the two, instead of having to re-write the using code. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Thu, Jul 13, 2017 at 12:27:03PM +0200, Paul B Mahol wrote: > +static inline unsigned int get_bits(GetBitContext *s, int n) > { > +#ifdef CACHED_BITSTREAM_READER > +register int tmp = 0; > +#ifdef BITSTREAM_READER_LE > +uint64_t left = 0; > +#endif > + > +av_assert2(n>0 && n<=32); > +if (n > s->bits_left) { > +n -= s->bits_left; > +#ifdef BITSTREAM_READER_LE > +left = s->bits_left; > +#endif > +tmp = get_val(s, s->bits_left); This triggers an assert in get_val() if s->bits_left == 0. > +refill_32(s); > +} > + > +#ifdef BITSTREAM_READER_LE > +tmp = get_val(s, n) << left | tmp; > +#else > +tmp = get_val(s, n) | tmp << n; This causes undefined behavior if n > 30. > +#endif > + > +#else > register int tmp; > OPEN_READER(re, s); > av_assert2(n>0 && n<=25); > UPDATE_CACHE(re, s); > -tmp = SHOW_SBITS(re, s, n); > +tmp = SHOW_UBITS(re, s, n); > LAST_SKIP_BITS(re, s, n); > CLOSE_READER(re, s); > +#endif > return tmp; > } The code under #ifdef CACHED_BITSTREAM_READER can probably be simplified like this (analogous to show_bits()): if (n > s->bits_left) refill_32(s); tmp = get_val(s, n); This avoids UB and is simpler/faster. Or am I missing something here? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
Signed-off-by: Paul B Mahol --- libavcodec/get_bits.h | 263 +- libavcodec/golomb.h | 151 + 2 files changed, 388 insertions(+), 26 deletions(-) diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h index c530015..dbacdda 100644 --- a/libavcodec/get_bits.h +++ b/libavcodec/get_bits.h @@ -1,5 +1,6 @@ /* - * copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2016 Alexandra Hájková * * This file is part of FFmpeg. * @@ -54,6 +55,10 @@ typedef struct GetBitContext { const uint8_t *buffer, *buffer_end; +#ifdef CACHED_BITSTREAM_READER +uint64_t cache; +unsigned bits_left; +#endif int index; int size_in_bits; int size_in_bits_plus8; @@ -106,12 +111,16 @@ typedef struct GetBitContext { * For examples see get_bits, show_bits, skip_bits, get_vlc. */ -#ifdef LONG_BITSTREAM_READER +#ifdef CACHED_BITSTREAM_READER +# define MIN_CACHE_BITS 64 +#elif defined LONG_BITSTREAM_READER # define MIN_CACHE_BITS 32 #else # define MIN_CACHE_BITS 25 #endif +#ifndef CACHED_BITSTREAM_READER + #define OPEN_READER_NOSIZE(name, gb)\ unsigned int name ## _index = (gb)->index; \ unsigned int av_unused name ## _cache @@ -196,20 +205,113 @@ typedef struct GetBitContext { #define GET_CACHE(name, gb) ((uint32_t) name ## _cache) +#endif + static inline int get_bits_count(const GetBitContext *s) { +#ifdef CACHED_BITSTREAM_READER +return s->index - s->bits_left; +#else return s->index; +#endif } -static inline void skip_bits_long(GetBitContext *s, int n) +static inline void refill_32(GetBitContext *s) { -#if UNCHECKED_BITSTREAM_READER -s->index += n; +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << s->bits_left | s->cache; #else -s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); +s->cache = s->cache | (uint64_t)AV_RB32(s->buffer + (s->index >> 3)) << (32 - s->bits_left); +#endif +s->index += 32; +s->bits_left += 32; +#endif +} + +static inline void refill_64(GetBitContext *s) +{ +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = AV_RL64(s->buffer + (s->index >> 3)); +#else +s->cache = AV_RB64(s->buffer + (s->index >> 3)); +#endif +s->index += 64; +s->bits_left = 64; +#endif +} + +#ifdef CACHED_BITSTREAM_READER +static inline uint64_t get_val(GetBitContext *s, unsigned n) +{ +uint64_t ret; +av_assert2(n>0 && n<=63); +#ifdef BITSTREAM_READER_LE +ret = s->cache & ((UINT64_C(1) << n) - 1); +s->cache >>= n; +#else +ret = s->cache >> (64 - n); +s->cache <<= n; +#endif +s->bits_left -= n; +return ret; +} +#endif + +#ifdef CACHED_BITSTREAM_READER +static inline unsigned show_val(const GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +return s->cache & ((UINT64_C(1) << n) - 1); +#else +return s->cache >> (64 - n); +#endif +} +#endif + +/** + * Show 1-25 bits. + */ +static inline unsigned int show_bits(GetBitContext *s, int n) +{ +register int tmp; +#ifdef CACHED_BITSTREAM_READER +if (n > s->bits_left) +refill_32(s); + +tmp = show_val(s, n); +#else +OPEN_READER_NOSIZE(re, s); +av_assert2(n>0 && n<=25); +UPDATE_CACHE(re, s); +tmp = SHOW_UBITS(re, s, n); #endif +return tmp; } +#ifdef CACHED_BITSTREAM_READER +static inline void skip_remaining(GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +s->cache >>= n; +#else +s->cache <<= n; +#endif +s->bits_left -= n; +} +#endif + /** * Read MPEG-1 dc-style VLC (sign bit + mantissa with no MSB). * if MSB not set it is negative @@ -217,6 +319,13 @@ static inline void skip_bits_long(GetBitContext *s, int n) */ static inline int get_xbits(GetBitContext *s, int n) { +#ifdef CACHED_BITSTREAM_READER +int32_t cache = show_bits(s, 32); +int sign = ~cache >> 31; +skip_remaining(s, n); + +return uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign; +#else register int sign; register int32_t cache; OPEN_READER(re, s); @@ -227,8 +336,10 @@ static inline int get_xbits(GetBitContext *s, int n) LAST_SKIP_BITS(re, s, n); CLOSE_READER(re, s); return (NEG_USR32(sign ^ cache, n) ^ sign) - sign; +#endif } +#ifndef CACHED_BITSTREAM_READER static inline int get_xbits_le(GetBitContext *s, int n) { register int sign; @@ -242,31 +353,61 @@ static inline int get_xbits_le(GetBitContext *s, int n) CLOSE_READER(re, s); return (zero_extend(sign ^ cache, n) ^ sign) - sign; } +#endif -static inline int get_sbits(GetBitContext *s,
[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
Signed-off-by: Paul B Mahol --- libavcodec/get_bits.h | 263 +- 1 file changed, 237 insertions(+), 26 deletions(-) diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h index c530015..dbacdda 100644 --- a/libavcodec/get_bits.h +++ b/libavcodec/get_bits.h @@ -1,5 +1,6 @@ /* - * copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2016 Alexandra Hájková * * This file is part of FFmpeg. * @@ -54,6 +55,10 @@ typedef struct GetBitContext { const uint8_t *buffer, *buffer_end; +#ifdef CACHED_BITSTREAM_READER +uint64_t cache; +unsigned bits_left; +#endif int index; int size_in_bits; int size_in_bits_plus8; @@ -106,12 +111,16 @@ typedef struct GetBitContext { * For examples see get_bits, show_bits, skip_bits, get_vlc. */ -#ifdef LONG_BITSTREAM_READER +#ifdef CACHED_BITSTREAM_READER +# define MIN_CACHE_BITS 64 +#elif defined LONG_BITSTREAM_READER # define MIN_CACHE_BITS 32 #else # define MIN_CACHE_BITS 25 #endif +#ifndef CACHED_BITSTREAM_READER + #define OPEN_READER_NOSIZE(name, gb)\ unsigned int name ## _index = (gb)->index; \ unsigned int av_unused name ## _cache @@ -196,20 +205,113 @@ typedef struct GetBitContext { #define GET_CACHE(name, gb) ((uint32_t) name ## _cache) +#endif + static inline int get_bits_count(const GetBitContext *s) { +#ifdef CACHED_BITSTREAM_READER +return s->index - s->bits_left; +#else return s->index; +#endif } -static inline void skip_bits_long(GetBitContext *s, int n) +static inline void refill_32(GetBitContext *s) { -#if UNCHECKED_BITSTREAM_READER -s->index += n; +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << s->bits_left | s->cache; #else -s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); +s->cache = s->cache | (uint64_t)AV_RB32(s->buffer + (s->index >> 3)) << (32 - s->bits_left); +#endif +s->index += 32; +s->bits_left += 32; +#endif +} + +static inline void refill_64(GetBitContext *s) +{ +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = AV_RL64(s->buffer + (s->index >> 3)); +#else +s->cache = AV_RB64(s->buffer + (s->index >> 3)); +#endif +s->index += 64; +s->bits_left = 64; +#endif +} + +#ifdef CACHED_BITSTREAM_READER +static inline uint64_t get_val(GetBitContext *s, unsigned n) +{ +uint64_t ret; +av_assert2(n>0 && n<=63); +#ifdef BITSTREAM_READER_LE +ret = s->cache & ((UINT64_C(1) << n) - 1); +s->cache >>= n; +#else +ret = s->cache >> (64 - n); +s->cache <<= n; +#endif +s->bits_left -= n; +return ret; +} +#endif + +#ifdef CACHED_BITSTREAM_READER +static inline unsigned show_val(const GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +return s->cache & ((UINT64_C(1) << n) - 1); +#else +return s->cache >> (64 - n); +#endif +} +#endif + +/** + * Show 1-25 bits. + */ +static inline unsigned int show_bits(GetBitContext *s, int n) +{ +register int tmp; +#ifdef CACHED_BITSTREAM_READER +if (n > s->bits_left) +refill_32(s); + +tmp = show_val(s, n); +#else +OPEN_READER_NOSIZE(re, s); +av_assert2(n>0 && n<=25); +UPDATE_CACHE(re, s); +tmp = SHOW_UBITS(re, s, n); #endif +return tmp; } +#ifdef CACHED_BITSTREAM_READER +static inline void skip_remaining(GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +s->cache >>= n; +#else +s->cache <<= n; +#endif +s->bits_left -= n; +} +#endif + /** * Read MPEG-1 dc-style VLC (sign bit + mantissa with no MSB). * if MSB not set it is negative @@ -217,6 +319,13 @@ static inline void skip_bits_long(GetBitContext *s, int n) */ static inline int get_xbits(GetBitContext *s, int n) { +#ifdef CACHED_BITSTREAM_READER +int32_t cache = show_bits(s, 32); +int sign = ~cache >> 31; +skip_remaining(s, n); + +return uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign; +#else register int sign; register int32_t cache; OPEN_READER(re, s); @@ -227,8 +336,10 @@ static inline int get_xbits(GetBitContext *s, int n) LAST_SKIP_BITS(re, s, n); CLOSE_READER(re, s); return (NEG_USR32(sign ^ cache, n) ^ sign) - sign; +#endif } +#ifndef CACHED_BITSTREAM_READER static inline int get_xbits_le(GetBitContext *s, int n) { register int sign; @@ -242,31 +353,61 @@ static inline int get_xbits_le(GetBitContext *s, int n) CLOSE_READER(re, s); return (zero_extend(sign ^ cache, n) ^ sign) - sign; } +#endif -static inline int get_sbits(GetBitContext *s, int n) +/** + * Read 1-25 bits. + */ +static inline unsigne
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On 7/8/17, foo86 wrote: > On Sat, Jul 08, 2017 at 08:07:49PM +0200, Hendrik Leppkes wrote: >> On Sat, Jul 8, 2017 at 7:09 PM, foo86 wrote: >> >> +static inline void skip_bits_long(GetBitContext *s, int n) >> >> +{ >> >> +#ifdef CACHED_BITSTREAM_READER >> >> +skip_bits(s, n); >> >> +#else >> >> +#if UNCHECKED_BITSTREAM_READER >> >> +s->index += n; >> >> +#else >> >> +s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - >> >> s->index); >> > Uncached bitstream reader allows seeking back by passing negative n >> > here. If cached bitstream reader disallows this, there should be a >> > comment saying so (and possibly an assert). >> >> This seems like an undocumented and possibly insecure/invalid use of >> the API, maybe we should just generally discourage such use. > > If skip_bits_long() is not supposed to seek backward, then it's fine by > me. (I thought it was looking at its implementation which allows > negative n). > >> Why would you need to skip backwards anyway? Usually code uses >> show_bits, or creates a copy of the reader so it can revert to the >> original if needed. > > DCA XLL decoder needs to seek backward to recover from segment overread. > I will probably change it to create a copy of the GetBitContext as you > suggested, seems to be a better solution. I will do same in skip_bits_long(). No need to change anything in codecs. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Sat, Jul 08, 2017 at 08:07:49PM +0200, Hendrik Leppkes wrote: > On Sat, Jul 8, 2017 at 7:09 PM, foo86 wrote: > >> +static inline void skip_bits_long(GetBitContext *s, int n) > >> +{ > >> +#ifdef CACHED_BITSTREAM_READER > >> +skip_bits(s, n); > >> +#else > >> +#if UNCHECKED_BITSTREAM_READER > >> +s->index += n; > >> +#else > >> +s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); > > Uncached bitstream reader allows seeking back by passing negative n > > here. If cached bitstream reader disallows this, there should be a > > comment saying so (and possibly an assert). > > This seems like an undocumented and possibly insecure/invalid use of > the API, maybe we should just generally discourage such use. If skip_bits_long() is not supposed to seek backward, then it's fine by me. (I thought it was looking at its implementation which allows negative n). > Why would you need to skip backwards anyway? Usually code uses > show_bits, or creates a copy of the reader so it can revert to the > original if needed. DCA XLL decoder needs to seek backward to recover from segment overread. I will probably change it to create a copy of the GetBitContext as you suggested, seems to be a better solution. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Sat, Jul 08, 2017 at 07:25:52PM +0200, Paul B Mahol wrote: > On 7/8/17, foo86 wrote: > > On Sat, Jul 08, 2017 at 11:12:06AM +0200, Paul B Mahol wrote: > >> [...] > > > >> static inline void skip_bits(GetBitContext *s, int n) > >> { > >> +#ifdef CACHED_BITSTREAM_READER > >> +if (n <= s->bits_left) > >> +skip_remaining(s, n); > >> +else { > >> +n -= s->bits_left; > >> +skip_remaining(s, s->bits_left); > > This causes undefined behavior if s->bits_left == 64. > > Could you elaborate?, it looks to me Libav have same issue. Calling skip_bits_long() with n > 64 after init_get_bits() causes this error: libavcodec/get_bits.h:309:14: runtime error: shift exponent 64 is too large for 64-bit type 'long unsigned int' ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Sat, Jul 8, 2017 at 7:23 PM, Rostislav Pehlivanov wrote: > On 8 July 2017 at 10:12, Paul B Mahol wrote: > >> Signed-off-by: Paul B Mahol >> --- >> libavcodec/get_bits.h | 261 ++ >> +++- >> 1 file changed, 235 insertions(+), 26 deletions(-) >> >> >> > I still say it should be enabled by default with a flag to choose between > 64 and 32 bit cache size (based on the arch). That'll give a speed increase > throughout most of the code and not cause much or any regression. Do you have any numbers for those claims? - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Sat, Jul 8, 2017 at 7:09 PM, foo86 wrote: > On Sat, Jul 08, 2017 at 11:12:06AM +0200, Paul B Mahol wrote: >> [...] > >> static inline void skip_bits(GetBitContext *s, int n) >> { >> +#ifdef CACHED_BITSTREAM_READER >> +if (n <= s->bits_left) >> +skip_remaining(s, n); >> +else { >> +n -= s->bits_left; >> +skip_remaining(s, s->bits_left); > This causes undefined behavior if s->bits_left == 64. > >> +if (n >= 64) { >> +unsigned skip = n; >> + >> +n -= skip; >> +s->index += skip; >> +} > This block looks strange. > >> +refill_32(s); >> +if (n) >> +skip_remaining(s, n); >> +} >> +#else >> OPEN_READER(re, s); >> LAST_SKIP_BITS(re, s, n); >> CLOSE_READER(re, s); >> +#endif >> +} >> + >> +static inline void skip_bits_long(GetBitContext *s, int n) >> +{ >> +#ifdef CACHED_BITSTREAM_READER >> +skip_bits(s, n); >> +#else >> +#if UNCHECKED_BITSTREAM_READER >> +s->index += n; >> +#else >> +s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); > Uncached bitstream reader allows seeking back by passing negative n > here. If cached bitstream reader disallows this, there should be a > comment saying so (and possibly an assert). This seems like an undocumented and possibly insecure/invalid use of the API, maybe we should just generally discourage such use. Why would you need to skip backwards anyway? Usually code uses show_bits, or creates a copy of the reader so it can revert to the original if needed. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On 8 July 2017 at 10:12, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol > --- > libavcodec/get_bits.h | 261 ++ > +++- > 1 file changed, 235 insertions(+), 26 deletions(-) > > > I still say it should be enabled by default with a flag to choose between 64 and 32 bit cache size (based on the arch). That'll give a speed increase throughout most of the code and not cause much or any regression. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On 7/8/17, foo86 wrote: > On Sat, Jul 08, 2017 at 11:12:06AM +0200, Paul B Mahol wrote: >> [...] > >> static inline void skip_bits(GetBitContext *s, int n) >> { >> +#ifdef CACHED_BITSTREAM_READER >> +if (n <= s->bits_left) >> +skip_remaining(s, n); >> +else { >> +n -= s->bits_left; >> +skip_remaining(s, s->bits_left); > This causes undefined behavior if s->bits_left == 64. Could you elaborate?, it looks to me Libav have same issue. > >> +if (n >= 64) { >> +unsigned skip = n; >> + >> +n -= skip; >> +s->index += skip; >> +} > This block looks strange. > >> +refill_32(s); >> +if (n) >> +skip_remaining(s, n); >> +} >> +#else >> OPEN_READER(re, s); >> LAST_SKIP_BITS(re, s, n); >> CLOSE_READER(re, s); >> +#endif >> +} >> + >> +static inline void skip_bits_long(GetBitContext *s, int n) >> +{ >> +#ifdef CACHED_BITSTREAM_READER >> +skip_bits(s, n); >> +#else >> +#if UNCHECKED_BITSTREAM_READER >> +s->index += n; >> +#else >> +s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); > Uncached bitstream reader allows seeking back by passing negative n > here. If cached bitstream reader disallows this, there should be a > comment saying so (and possibly an assert). I can add seeking one from other API. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Sat, Jul 08, 2017 at 11:12:06AM +0200, Paul B Mahol wrote: > [...] > static inline void skip_bits(GetBitContext *s, int n) > { > +#ifdef CACHED_BITSTREAM_READER > +if (n <= s->bits_left) > +skip_remaining(s, n); > +else { > +n -= s->bits_left; > +skip_remaining(s, s->bits_left); This causes undefined behavior if s->bits_left == 64. > +if (n >= 64) { > +unsigned skip = n; > + > +n -= skip; > +s->index += skip; > +} This block looks strange. > +refill_32(s); > +if (n) > +skip_remaining(s, n); > +} > +#else > OPEN_READER(re, s); > LAST_SKIP_BITS(re, s, n); > CLOSE_READER(re, s); > +#endif > +} > + > +static inline void skip_bits_long(GetBitContext *s, int n) > +{ > +#ifdef CACHED_BITSTREAM_READER > +skip_bits(s, n); > +#else > +#if UNCHECKED_BITSTREAM_READER > +s->index += n; > +#else > +s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); Uncached bitstream reader allows seeking back by passing negative n here. If cached bitstream reader disallows this, there should be a comment saying so (and possibly an assert). ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
Signed-off-by: Paul B Mahol --- libavcodec/get_bits.h | 261 +- 1 file changed, 235 insertions(+), 26 deletions(-) diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h index c530015..f404b80 100644 --- a/libavcodec/get_bits.h +++ b/libavcodec/get_bits.h @@ -1,5 +1,6 @@ /* - * copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2016 Alexandra Hájková * * This file is part of FFmpeg. * @@ -54,6 +55,10 @@ typedef struct GetBitContext { const uint8_t *buffer, *buffer_end; +#ifdef CACHED_BITSTREAM_READER +uint64_t cache; +unsigned bits_left; +#endif int index; int size_in_bits; int size_in_bits_plus8; @@ -106,12 +111,16 @@ typedef struct GetBitContext { * For examples see get_bits, show_bits, skip_bits, get_vlc. */ -#ifdef LONG_BITSTREAM_READER +#ifdef CACHED_BITSTREAM_READER +# define MIN_CACHE_BITS 64 +#elif defined LONG_BITSTREAM_READER # define MIN_CACHE_BITS 32 #else # define MIN_CACHE_BITS 25 #endif +#ifndef CACHED_BITSTREAM_READER + #define OPEN_READER_NOSIZE(name, gb)\ unsigned int name ## _index = (gb)->index; \ unsigned int av_unused name ## _cache @@ -196,20 +205,113 @@ typedef struct GetBitContext { #define GET_CACHE(name, gb) ((uint32_t) name ## _cache) +#endif + static inline int get_bits_count(const GetBitContext *s) { +#ifdef CACHED_BITSTREAM_READER +return s->index - s->bits_left; +#else return s->index; +#endif } -static inline void skip_bits_long(GetBitContext *s, int n) +static inline void refill_32(GetBitContext *s) { -#if UNCHECKED_BITSTREAM_READER -s->index += n; +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << s->bits_left | s->cache; #else -s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index); +s->cache = s->cache | (uint64_t)AV_RB32(s->buffer + (s->index >> 3)) << (32 - s->bits_left); +#endif +s->index += 32; +s->bits_left += 32; +#endif +} + +static inline void refill_64(GetBitContext *s) +{ +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->index >> 3 >= s->buffer_end - s->buffer) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = AV_RL64(s->buffer + (s->index >> 3)); +#else +s->cache = AV_RB64(s->buffer + (s->index >> 3)); +#endif +s->index += 64; +s->bits_left = 64; +#endif +} + +#ifdef CACHED_BITSTREAM_READER +static inline uint64_t get_val(GetBitContext *s, unsigned n) +{ +uint64_t ret; +av_assert2(n>0 && n<=63); +#ifdef BITSTREAM_READER_LE +ret = s->cache & ((UINT64_C(1) << n) - 1); +s->cache >>= n; +#else +ret = s->cache >> (64 - n); +s->cache <<= n; +#endif +s->bits_left -= n; +return ret; +} +#endif + +#ifdef CACHED_BITSTREAM_READER +static inline unsigned show_val(const GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +return s->cache & ((UINT64_C(1) << n) - 1); +#else +return s->cache >> (64 - n); +#endif +} +#endif + +/** + * Show 1-25 bits. + */ +static inline unsigned int show_bits(GetBitContext *s, int n) +{ +register int tmp; +#ifdef CACHED_BITSTREAM_READER +if (n > s->bits_left) +refill_32(s); + +tmp = show_val(s, n); +#else +OPEN_READER_NOSIZE(re, s); +av_assert2(n>0 && n<=25); +UPDATE_CACHE(re, s); +tmp = SHOW_UBITS(re, s, n); #endif +return tmp; } +#ifdef CACHED_BITSTREAM_READER +static inline void skip_remaining(GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +s->cache >>= n; +#else +s->cache <<= n; +#endif +s->bits_left -= n; +} +#endif + /** * Read MPEG-1 dc-style VLC (sign bit + mantissa with no MSB). * if MSB not set it is negative @@ -217,6 +319,13 @@ static inline void skip_bits_long(GetBitContext *s, int n) */ static inline int get_xbits(GetBitContext *s, int n) { +#ifdef CACHED_BITSTREAM_READER +int32_t cache = show_bits(s, 32); +int sign = ~cache >> 31; +skip_remaining(s, n); + +return uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign; +#else register int sign; register int32_t cache; OPEN_READER(re, s); @@ -227,8 +336,10 @@ static inline int get_xbits(GetBitContext *s, int n) LAST_SKIP_BITS(re, s, n); CLOSE_READER(re, s); return (NEG_USR32(sign ^ cache, n) ^ sign) - sign; +#endif } +#ifndef CACHED_BITSTREAM_READER static inline int get_xbits_le(GetBitContext *s, int n) { register int sign; @@ -242,31 +353,61 @@ static inline int get_xbits_le(GetBitContext *s, int n) CLOSE_READER(re, s); return (zero_extend(sign ^ cache, n) ^ sign) - sign; } +#endif -static inline int get_sbits(GetBitContext *s, int n) +/** + * Read 1-25 bits. + */ +static inline unsigne
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On Fri, Jul 07, 2017 at 08:48:46PM +0200, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol > --- > libavcodec/get_bits.h | 205 > +++--- > 1 file changed, 196 insertions(+), 9 deletions(-) > > diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h > index c530015..8a9021a 100644 > --- a/libavcodec/get_bits.h > +++ b/libavcodec/get_bits.h > @@ -1,5 +1,6 @@ > /* > - * copyright (c) 2004 Michael Niedermayer > + * Copyright (c) 2004 Michael Niedermayer > + * Copyright (c) 2016 Alexandra Hájková > * > * This file is part of FFmpeg. > * > @@ -54,6 +55,10 @@ > > typedef struct GetBitContext { > const uint8_t *buffer, *buffer_end; > +#ifdef CACHED_BITSTREAM_READER > +uint64_t cache; > +unsigned bits_left; > +#endif > int index; > int size_in_bits; > int size_in_bits_plus8; > @@ -106,7 +111,9 @@ typedef struct GetBitContext { > * For examples see get_bits, show_bits, skip_bits, get_vlc. > */ > > -#ifdef LONG_BITSTREAM_READER > +#ifdef CACHED_BITSTREAM_READER > +# define MIN_CACHE_BITS 64 > +#elif defined LONG_BITSTREAM_READER > # define MIN_CACHE_BITS 32 > #else > # define MIN_CACHE_BITS 25 > @@ -198,7 +205,11 @@ typedef struct GetBitContext { > > static inline int get_bits_count(const GetBitContext *s) > { > +#ifdef CACHED_BITSTREAM_READER > +return s->index - s->bits_left; > +#else > return s->index; > +#endif > } > > static inline void skip_bits_long(GetBitContext *s, int n) > @@ -210,6 +221,68 @@ static inline void skip_bits_long(GetBitContext *s, int > n) > #endif > } > > +static inline void refill_32(GetBitContext *s) > +{ > +#ifdef CACHED_BITSTREAM_READER > +if (s->buffer + (s->index >> 3) >= s->buffer_end) > +return; should be under !UNCHECKED_BITSTREAM_READER also this looks like it can result in intermediate invalid pointers this should avoid that: s->index >> 3 >= s->buffer_end - s->buffer same issue in refill_64() patch overall is nice thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
Hi, On Fri, Jul 7, 2017 at 4:43 PM, Paul B Mahol wrote: > On 7/7/17, Ronald S. Bultje wrote: > > (I'm assuming the low-level interface no longer works with the cached > > reader, so can we prevent users from accessing these macros unless > > cached=1?) > > They are not supposed to work, Do you mean to not define such macros > if cached is defined? Yes. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
On 7/7/17, Ronald S. Bultje wrote: > Hi, > > On Fri, Jul 7, 2017 at 2:48 PM, Paul B Mahol wrote: > >> typedef struct GetBitContext { >> const uint8_t *buffer, *buffer_end; >> +#ifdef CACHED_BITSTREAM_READER >> +uint64_t cache; >> +unsigned bits_left; >> +#endif >> > > Can you post some stats (from relevant systems, ideally, e.g. 32-bit binary > on x86, or 32-bit arm) on how a 32bit cache performs compared to a 64bit > cache on systems with HAVE_FAST_64BIT=0? I can only post results from 64bit x86 with gcc 6.2.0. > > >> +static inline void refill_32(GetBitContext *s) >> > [..] > >> +#ifdef BITSTREAM_READER_LE >> +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << >> s->cache | s->cache; >> > > As said on IRC: middle s->cache should be s->bits_left. > > Overall very nice improvement, I would in particular not be surprised if > this is generally faster for almost all users, except those using the > lower-level macros (things like SHOW_BITS() etc.) in the old interface. If > that's true, it may be positive to enable this by default and disable only > for those using the low-level interface. > > (I'm assuming the low-level interface no longer works with the cached > reader, so can we prevent users from accessing these macros unless > cached=1?) They are not supposed to work, Do you mean to not define such macros if cached is defined? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
Hi, On Fri, Jul 7, 2017 at 2:48 PM, Paul B Mahol wrote: > typedef struct GetBitContext { > const uint8_t *buffer, *buffer_end; > +#ifdef CACHED_BITSTREAM_READER > +uint64_t cache; > +unsigned bits_left; > +#endif > Can you post some stats (from relevant systems, ideally, e.g. 32-bit binary on x86, or 32-bit arm) on how a 32bit cache performs compared to a 64bit cache on systems with HAVE_FAST_64BIT=0? > +static inline void refill_32(GetBitContext *s) > [..] > +#ifdef BITSTREAM_READER_LE > +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << > s->cache | s->cache; > As said on IRC: middle s->cache should be s->bits_left. Overall very nice improvement, I would in particular not be surprised if this is generally faster for almost all users, except those using the lower-level macros (things like SHOW_BITS() etc.) in the old interface. If that's true, it may be positive to enable this by default and disable only for those using the low-level interface. (I'm assuming the low-level interface no longer works with the cached reader, so can we prevent users from accessing these macros unless cached=1?) Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader
Signed-off-by: Paul B Mahol --- libavcodec/get_bits.h | 205 +++--- 1 file changed, 196 insertions(+), 9 deletions(-) diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h index c530015..8a9021a 100644 --- a/libavcodec/get_bits.h +++ b/libavcodec/get_bits.h @@ -1,5 +1,6 @@ /* - * copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2004 Michael Niedermayer + * Copyright (c) 2016 Alexandra Hájková * * This file is part of FFmpeg. * @@ -54,6 +55,10 @@ typedef struct GetBitContext { const uint8_t *buffer, *buffer_end; +#ifdef CACHED_BITSTREAM_READER +uint64_t cache; +unsigned bits_left; +#endif int index; int size_in_bits; int size_in_bits_plus8; @@ -106,7 +111,9 @@ typedef struct GetBitContext { * For examples see get_bits, show_bits, skip_bits, get_vlc. */ -#ifdef LONG_BITSTREAM_READER +#ifdef CACHED_BITSTREAM_READER +# define MIN_CACHE_BITS 64 +#elif defined LONG_BITSTREAM_READER # define MIN_CACHE_BITS 32 #else # define MIN_CACHE_BITS 25 @@ -198,7 +205,11 @@ typedef struct GetBitContext { static inline int get_bits_count(const GetBitContext *s) { +#ifdef CACHED_BITSTREAM_READER +return s->index - s->bits_left; +#else return s->index; +#endif } static inline void skip_bits_long(GetBitContext *s, int n) @@ -210,6 +221,68 @@ static inline void skip_bits_long(GetBitContext *s, int n) #endif } +static inline void refill_32(GetBitContext *s) +{ +#ifdef CACHED_BITSTREAM_READER +if (s->buffer + (s->index >> 3) >= s->buffer_end) +return; + +#ifdef BITSTREAM_READER_LE +s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << s->cache | s->cache; +#else +s->cache = s->cache | (uint64_t)AV_RB32(s->buffer + (s->index >> 3)) << (32 - s->bits_left); +#endif +s->index += 32; +s->bits_left += 32; +#endif +} + +static inline void refill_64(GetBitContext *s) +{ +#ifdef CACHED_BITSTREAM_READER +#if !UNCHECKED_BITSTREAM_READER +if (s->buffer + (s->index >> 3) >= s->buffer_end) +return; +#endif + +#ifdef BITSTREAM_READER_LE +s->cache = AV_RL64(s->buffer + (s->index >> 3)); +#else +s->cache = AV_RB64(s->buffer + (s->index >> 3)); +#endif +s->index += 64; +s->bits_left = 64; +#endif +} + +#ifdef CACHED_BITSTREAM_READER +static inline uint64_t get_val(GetBitContext *s, unsigned n) +{ +uint64_t ret; +av_assert2(n>0 && n<=63); +#ifdef BITSTREAM_READER_LE +ret = s->cache & ((UINT64_C(1) << n) - 1); +s->cache >>= n; +#else +ret = s->cache >> (64 - n); +s->cache <<= n; +#endif +s->bits_left -= n; +return ret; +} +#endif + +#ifdef CACHED_BITSTREAM_READER +static inline unsigned show_val(const GetBitContext *s, unsigned n) +{ +#ifdef BITSTREAM_READER_LE +return s->cache & ((UINT64_C(1) << n) - 1); +#else +return s->cache >> (64 - n); +#endif +} +#endif + /** * Read MPEG-1 dc-style VLC (sign bit + mantissa with no MSB). * if MSB not set it is negative @@ -243,30 +316,59 @@ static inline int get_xbits_le(GetBitContext *s, int n) return (zero_extend(sign ^ cache, n) ^ sign) - sign; } -static inline int get_sbits(GetBitContext *s, int n) +/** + * Read 1-25 bits. + */ +static inline unsigned int get_bits(GetBitContext *s, int n) { +#ifdef CACHED_BITSTREAM_READER +register int tmp = 0; +#ifdef BITSTREAM_READER_LE +uint64_t left = 0; +#endif + +av_assert2(n>0 && n<=32); +if (n > s->bits_left) { +n -= s->bits_left; +#ifdef BITSTREAM_READER_LE +left = s->bits_left; +#endif +tmp = get_val(s, s->bits_left); +refill_32(s); +} + +#ifdef BITSTREAM_READER_LE +tmp = get_val(s, n) << left | tmp; +#else +tmp = get_val(s, n) | tmp << n; +#endif + +#else register int tmp; OPEN_READER(re, s); av_assert2(n>0 && n<=25); UPDATE_CACHE(re, s); -tmp = SHOW_SBITS(re, s, n); +tmp = SHOW_UBITS(re, s, n); LAST_SKIP_BITS(re, s, n); CLOSE_READER(re, s); +#endif return tmp; } -/** - * Read 1-25 bits. - */ -static inline unsigned int get_bits(GetBitContext *s, int n) +static inline int get_sbits(GetBitContext *s, int n) { register int tmp; +#ifdef CACHED_BITSTREAM_READER +av_assert2(n>0 && n<=25); +tmp = sign_extend(get_bits(s, n), n); +#else OPEN_READER(re, s); av_assert2(n>0 && n<=25); UPDATE_CACHE(re, s); -tmp = SHOW_UBITS(re, s, n); +tmp = SHOW_SBITS(re, s, n); LAST_SKIP_BITS(re, s, n); CLOSE_READER(re, s); +#endif return tmp; } @@ -296,22 +398,65 @@ static inline unsigned int get_bits_le(GetBitContext *s, int n) static inline unsigned int show_bits(GetBitContext *s, int n) { register int tmp; +#ifdef CACHED_BITSTREAM_READER +if (n > s->bits_left) +refill_32(s); + +tmp = show_val(s, n); +#else OPEN_READER_NOSIZE(re, s); av_assert2(n>0 && n<=25); UPDATE_CACHE(re, s); tmp = SHOW_UBITS(re, s, n); +#endif