[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader

2018-08-23 Thread Paul B Mahol 
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

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader

2018-04-03 Thread Michael Niedermayer 
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

2018-04-03 Thread Paul B Mahol 
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 ^

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader

2017-07-14 Thread foo86 
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

2017-07-14 Thread Hendrik Leppkes 
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

2017-07-14 Thread foo86 
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

2017-07-13 Thread Paul B Mahol 
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;
 }

[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader

2017-07-11 Thread Paul B Mahol 
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

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader

2017-07-08 Thread Paul B Mahol 
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

2017-07-08 Thread foo86 
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

2017-07-08 Thread foo86 
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

2017-07-08 Thread Hendrik Leppkes 
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

2017-07-08 Thread Hendrik Leppkes 
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

2017-07-08 Thread Rostislav Pehlivanov 
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

2017-07-08 Thread Paul B Mahol 
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

2017-07-08 Thread foo86 
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

2017-07-08 Thread Paul B Mahol 
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

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader

2017-07-07 Thread Michael Niedermayer 
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

2017-07-07 Thread Ronald S. Bultje 
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

2017-07-07 Thread Paul B Mahol 
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

2017-07-07 Thread Ronald S. Bultje 
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

2017-07-07 Thread Paul B Mahol 
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);