[FFmpeg-devel] [PATCH 2/2] avcodec/tests/dct: Add Mean square error test

2017-07-08 Thread Michael Niedermayer 
based on quotes of IEEE 1180 / ISO/IEC 23002-1

Signed-off-by: Michael Niedermayer 
---
 libavcodec/tests/dct.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libavcodec/tests/dct.c b/libavcodec/tests/dct.c
index cf71b96508..b44c66f427 100644
--- a/libavcodec/tests/dct.c
+++ b/libavcodec/tests/dct.c
@@ -182,6 +182,7 @@ static int dct_error(const struct algo *dct, int test, int 
is_idct, int speed, c
 int err_inf, v;
 int64_t err2, ti, ti1, it1, err_sum = 0;
 int64_t sysErr[64], sysErrMax = 0;
+int64_t err2_matrix[64], err2_max = 0;
 int maxout = 0;
 int blockSumErrMax = 0, blockSumErr;
 AVLFG prng;
@@ -194,7 +195,7 @@ static int dct_error(const struct algo *dct, int test, int 
is_idct, int speed, c
 err_inf = 0;
 err2 = 0;
 for (i = 0; i < 64; i++)
-sysErr[i] = 0;
+err2_matrix[i] = sysErr[i] = 0;
 for (it = 0; it < NB_ITS; it++) {
 init_block(block1, test, is_idct, , vals);
 permute(block, block1, dct->perm_type);
@@ -221,6 +222,7 @@ static int dct_error(const struct algo *dct, int test, int 
is_idct, int speed, c
 v = abs(err);
 if (v > err_inf)
 err_inf = v;
+err2_matrix[i] += v * v;
 err2 += v * v;
 sysErr[i] += block[i] - block1[i];
 blockSumErr += v;
@@ -230,8 +232,10 @@ static int dct_error(const struct algo *dct, int test, int 
is_idct, int speed, c
 if (blockSumErrMax < blockSumErr)
 blockSumErrMax = blockSumErr;
 }
-for (i = 0; i < 64; i++)
+for (i = 0; i < 64; i++) {
 sysErrMax = FFMAX(sysErrMax, FFABS(sysErr[i]));
+err2_max  = FFMAX(err2_max , FFABS(err2_matrix[i]));
+}
 
 for (i = 0; i < 64; i++) {
 if (i % 8 == 0)
@@ -245,7 +249,7 @@ static int dct_error(const struct algo *dct, int test, int 
is_idct, int speed, c
 
 spec_err = is_idct && (err_inf > 1 || omse > 0.02 || fabs(ome) > 0.0015);
 if (test < 2)
-spec_err = is_idct && ((double) sysErrMax / NB_ITS > 0.015);
+spec_err = is_idct && ((double) err2_max / NB_ITS > 0.06 || (double) 
sysErrMax / NB_ITS > 0.015);
 
 printf("%s %s: max_err=%d omse=%0.8f ome=%0.8f syserr=%0.8f maxout=%d 
blockSumErr=%d\n",
is_idct ? "IDCT" : "DCT", dct->name, err_inf,
-- 
2.13.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] avcodec/tests/dct: Add peak mean error check

2017-07-08 Thread Michael Niedermayer 
based on quotes of IEEE 1180 / ISO/IEC 23002-1

Signed-off-by: Michael Niedermayer 
---
 libavcodec/tests/dct.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavcodec/tests/dct.c b/libavcodec/tests/dct.c
index 29af3fea8a..cf71b96508 100644
--- a/libavcodec/tests/dct.c
+++ b/libavcodec/tests/dct.c
@@ -244,6 +244,8 @@ static int dct_error(const struct algo *dct, int test, int 
is_idct, int speed, c
 ome  = (double) err_sum / NB_ITS / 64;
 
 spec_err = is_idct && (err_inf > 1 || omse > 0.02 || fabs(ome) > 0.0015);
+if (test < 2)
+spec_err = is_idct && ((double) sysErrMax / NB_ITS > 0.015);
 
 printf("%s %s: max_err=%d omse=%0.8f ome=%0.8f syserr=%0.8f maxout=%d 
blockSumErr=%d\n",
is_idct ? "IDCT" : "DCT", dct->name, err_inf,
-- 
2.13.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [WIP][PATCH]v4 Opus Pyramid Vector Quantization Search in x86 SIMD asm

2017-07-08 Thread Ivan Kalvachev 
This should be the final work-in-progress patch.

What's changed:

1. Removed macros conditional defines. The defaults seems to be
optimal on all machines that I got benchmarks from. HADDPS and PHADDD
are always slower, "BLEND"s are never slower than the emulation.

2. Remove SHORT_SYY_UPDATE. It is always slower.

3. Remove ALL_FLOAT_PRESEARCH, it is always slower. Remove the ugly
hack to use 256bit ymm with avx1 and integer operations.

4. Remove remaining disabled code.

5. Use HADDD macro from "x86util", I don't need the result in all lanes/elements

6. Use v-prefix for all avx code.

7. Small optimization: Move some of the HSUMPS in the K!=0 branch.

8. Small optimization: Instead of pre-calculation 2*Y[i] and then
correcting it on exit, It is possible to use Syy/2 instead in
distortion parameter calculations. It saves few multiplications in
pre-search and sign restore loop. It however gives different
approximation of sqrt(). It's not (consistently) better or worse than
the previous approximation.

9. Using movdqa to load "const_int32_offsets". Wrong type might
explain why directly using mem constants is sometimes faster.

10. Move some code around and do minor tweaks.
---

I do not intend of removing "PRESEARCH_ROUNDING" and
"USE_APPROXIMATION", (while for the latter I think I will remove
method#1, I've left it this time just for consistency").
These defines control the precision and the type of results that the
function generates.
E.g. This function can produce same results as opus functions with
"PRESEARCH_ROUNDING 0".
If you want same results as the ffmpeg improved function, then you
need "approx#0". It uses real division and is much slower on older
cpu's, but reasonably fast on anything recent.

I've left 2 other defines. "CONST_IN_X64_REG_IS_FASTER" and
"STALL_WRITE_FORWARDING".
On Sandy Bridge and laters, "const_in_x64" has always been faster. On
my cpu it is about the same.
On Ryzen the "const_in_x64" was consistently faster in all sse/avx
variants, with about 5%. But not if "stall_write" is enabled too.
Ryzen (allegedly) has no write stalling, but that method alone is just
a few cycles faster (about 0.5% ).

I'd like to see if the changes I've done this time, would affect the
above results.


The code is much cleaner and you are free to nitpick on it.

There is something that I'm not exactly sure if I need it.
The function gets 2 integer parameters, and I am not sure
if I have to sign extend them in 64 bit more, in order to clear
the high 32 bits. These parameters should never be negative, so the
sign is not needed.
All 32bit operands should also clear the high bits.
Still I'm not sure if there is guarantee that
the high bits won't contain garbage.


Best Regards
From 4591ad752d1d111615f320b17ad19d5fad0d91d4 Mon Sep 17 00:00:00 2001
From: Ivan Kalvachev 
Date: Thu, 8 Jun 2017 22:24:33 +0300
Subject: [PATCH 1/5] SIMD opus pvq_search implementation v4

---
 libavcodec/opus_pvq.c  |   9 +
 libavcodec/opus_pvq.h  |   5 +-
 libavcodec/x86/Makefile|   1 +
 libavcodec/x86/opus_dsp_init.c |  47 
 libavcodec/x86/opus_pvq_search.asm | 524 +
 5 files changed, 584 insertions(+), 2 deletions(-)
 create mode 100644 libavcodec/x86/opus_dsp_init.c
 create mode 100644 libavcodec/x86/opus_pvq_search.asm

diff --git a/libavcodec/opus_pvq.c b/libavcodec/opus_pvq.c
index 2ac66a0ede..6c7504296d 100644
--- a/libavcodec/opus_pvq.c
+++ b/libavcodec/opus_pvq.c
@@ -418,7 +418,13 @@ static uint32_t celt_alg_quant(OpusRangeCoder *rc, float *X, uint32_t N, uint32_
 int *y = pvq->qcoeff;
 
 celt_exp_rotation(X, N, blocks, K, spread, 1);
+
+{
+START_TIMER
 gain /= sqrtf(pvq->pvq_search(X, y, K, N));
+STOP_TIMER("pvq_search");
+}
+
 celt_encode_pulses(rc, y,  N, K);
 celt_normalize_residual(y, X, N, gain);
 celt_exp_rotation(X, N, blocks, K, spread, 0);
@@ -947,6 +953,9 @@ int av_cold ff_celt_pvq_init(CeltPVQ **pvq)
 s->encode_band= pvq_encode_band;
 s->band_cost  = pvq_band_cost;
 
+if (ARCH_X86 && CONFIG_OPUS_ENCODER)
+ff_opus_dsp_init_x86(s);
+
 *pvq = s;
 
 return 0;
diff --git a/libavcodec/opus_pvq.h b/libavcodec/opus_pvq.h
index 6691494838..9246337360 100644
--- a/libavcodec/opus_pvq.h
+++ b/libavcodec/opus_pvq.h
@@ -33,8 +33,8 @@
float *lowband_scratch, int fill)
 
 struct CeltPVQ {
-DECLARE_ALIGNED(32, int,   qcoeff  )[176];
-DECLARE_ALIGNED(32, float, hadamard_tmp)[176];
+DECLARE_ALIGNED(32, int,   qcoeff  )[256];
+DECLARE_ALIGNED(32, float, hadamard_tmp)[256];
 
 float (*pvq_search)(float *X, int *y, int K, int N);
 
@@ -45,6 +45,7 @@ struct CeltPVQ {
 };
 
 int  ff_celt_pvq_init  (struct CeltPVQ **pvq);
+void ff_opus_dsp_init_x86(struct CeltPVQ *s);
 void ff_celt_pvq_uninit(struct CeltPVQ **pvq);
 
 #endif /* AVCODEC_OPUS_PVQ_H */
diff --git

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

2017-07-08 Thread Ronald S. Bultje 
Hi,

On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer 
wrote:

> On Mon, Jul 03, 2017 at 01:37:09AM +0200, Michael Niedermayer wrote:
> > On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote:
> > > On 2 July 2017 at 03:28, Michael Niedermayer 
> wrote:
> > >
> > > > Fixes: runtime error: signed integer overflow: 1965219850 + 995792909
> > > > cannot be represented in type 'int'
> > > > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
> > > >
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-
> > > > fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by
> > > >  ffmpeg%0ASigned-off-by>:
> > > > Michael Niedermayer 
> > > > ---
> > > >  libavcodec/aacpsdsp_template.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
> > > > template.c
> > > > index 9e1a95c1a1..2c0afd4512 100644
> > > > --- a/libavcodec/aacpsdsp_template.c
> > > > +++ b/libavcodec/aacpsdsp_template.c
> > > > @@ -26,9 +26,10 @@
> > > >  #include "libavutil/attributes.h"
> > > >  #include "aacpsdsp.h"
> > > >
> > > > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT
> (*src)[2], int
> > > > n)
> > > > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
> > > > (*src)[2], int n)
> > > >  {
> > > >  int i;
> > > > +SUINTFLOAT *dst = dst_param;
> > > >  for (i = 0; i < n; i++)
> > > >  dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1],
> src[i][1]);
> > > >  }
> > > >
> > > >
> > > What's the issue with just _not_ fixing it here? It only occurs on
> fuzzed
> > > inputs, doesn't crash on any known platform ever, makes the code
> uglier and
> > > why? Because some fuzzer is super pedantic.
> > > Why not fix the fuzzer? Why not just mark this as a false positive,
> since
> > > fixing it is pointless from the standpoint of security (you can't
> exploit
> > > overflows in transforms or functions like this), and all developers
> hate it.
> >
> > Iam not sure you understand the problem.
> > signed integer overflows are undefined behavior, undefined behavior
> > is not allowed in C.
> >
> >
> > Theres also no option to mark anything as false positive.
> > If the tool makes a mistake, the issue needs to be reported to its
> > authors and needs to be fixed.
> > I belive the tool is correct, if someone thinks its not correct, the
> > place to report this to is likely where the clang sanitizers are
> > developed.
> >
> > I do understand that this is annoying but this is how C works.
> >
> > About "doesn't crash on any known platform ever",
> > "you can't exploit  overflows in transforms or functions like this"
> >
> > undefined behavior has bitten people with unexpected results in the
> > past. for example "if (a+b < a)" to test for overflow, but because the
> condition
> > can only be true in case of overflow and overflow isnt allowed in C
> > the compiler didnt assemble this the way the human thought.
> >
> > In the case of ps_add_squares_c(), dst[i] has to increase if iam
> > not mistaken. In case of overflow it can decrease but overflow is
> > not allowed so a compiler can prune anything that implies dst[] to be
> > negative.
> > dst[] is used downstream in checks of greater / less and in FFMAX
> > In this code the compiler can assume that the sign bit is clear,
> > what happens when  the compilers optimizer has false assumtations
> > i dont know but i know its not good.
> >
> > That said even if no such chain of conditions exist its still invalid
> > code if theres undefined behavior in it
>
> Does anyone object to this patch ?
> Or does anyone have a better idea on how to fix this ?
> if not id like to apply it


I think Rostislav's point is: why fix it, if it can only happen with
corrupt input? The before and after situation is identical: garbage in,
garbage out. If the compiler does funny things that makes the garbage
slightly differently bad, is that really so devilishly bad? It's still
garbage. Is anything improved by this?

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pthread_frame: save the FF_DEBUG_THREADS option in FrameThreadContext.

2017-07-08 Thread Michael Niedermayer 
On Sat, Jul 08, 2017 at 03:38:11PM -0700, Wan-Teh Chang wrote:
> Hi Ronald,
> 
> On Sat, Jul 8, 2017 at 2:33 PM, Ronald S. Bultje  wrote:
> >
> > I can see the design from the patch.
> >
> > What's missing is a justification for the downside of the design, which is
> > that updates to this variable by the user are no longer propagated to the
> > worker threads.
> 
> My justification is the YAGNI principle.
> 
> Although the current code allows the FF_DEBUG_THREADS option to be
> toggled dynamically, I believe that was not intended, and I believe
> nobody actually does that. In my (admittedly limited) code search, I
> only see the FF_DEBUG_THREADS option set via the -debug thread_ops
> command-line option.

ffmpeg (the command line tool) allows changing the AVCodecContext->debug
value at runtime

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pthread_frame: save the FF_DEBUG_THREADS option in FrameThreadContext.

2017-07-08 Thread Wan-Teh Chang 
Hi Ronald,

On Sat, Jul 8, 2017 at 2:33 PM, Ronald S. Bultje  wrote:
>
> I can see the design from the patch.
>
> What's missing is a justification for the downside of the design, which is
> that updates to this variable by the user are no longer propagated to the
> worker threads.

My justification is the YAGNI principle.

Although the current code allows the FF_DEBUG_THREADS option to be
toggled dynamically, I believe that was not intended, and I believe
nobody actually does that. In my (admittedly limited) code search, I
only see the FF_DEBUG_THREADS option set via the -debug thread_ops
command-line option.

> The syncing is extremely trivial (simply by moving the
> assignment from init to update_from_user) and has no threading implications
> (since it's a boolean, so you can make it atomic) so this really shouldn't
> be a big deal to amend.

Yes, the syncing is trivial to add. When someone actually needs to
toggle the FF_DEBUG_THREADS option dynamically, we can easily add the
syncing at that time.

This is your call. Please let me know your decision. Thanks!

Wan-Teh Chang
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pthread_frame: save the FF_DEBUG_THREADS option in FrameThreadContext.

2017-07-08 Thread Ronald S. Bultje 
Hi,

On Sat, Jul 8, 2017 at 5:28 PM, Wan-Teh Chang 
wrote:

> Hi Ronald,
>
> On Sat, Jul 8, 2017 at 6:19 AM, Ronald S. Bultje 
> wrote:
> > Hi,
> >
> > On Fri, Jul 7, 2017 at 5:02 PM, Wan-Teh Chang <
> wtc-at-google@ffmpeg.org>
> > wrote:
> >
> >> @@ -763,6 +764,7 @@ int ff_frame_thread_init(AVCodecContext *avctx)
> >>
> >>  fctx->async_lock = 1;
> >>  fctx->delaying = 1;
> >> +fctx->debug_threads = (avctx->debug & FF_DEBUG_THREADS) != 0;
> >
> > Shouldn't this be done in update_context_from_user()?
>
> This patch differs from the approach you outlined at the end of
> http://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213259.html
> as follows:
>
> * The debug_threads field is added to FrameThreadContext and applies to
>   all the decoding threads owned by the FrameThreadContext.
> * The debug_threads field is set when avcodec_open2() is called, and
>   never changes thereafter.
>
> In this design, we just need to set fctx->debug_threads in
> ff_frame_thread_init() (which is called by avcodec_open2()).


I can see the design from the patch.

What's missing is a justification for the downside of the design, which is
that updates to this variable by the user are no longer propagated to the
worker threads. The syncing is extremely trivial (simply by moving the
assignment from init to update_from_user) and has no threading implications
(since it's a boolean, so you can make it atomic) so this really shouldn't
be a big deal to amend.

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pthread_frame: save the FF_DEBUG_THREADS option in FrameThreadContext.

2017-07-08 Thread Wan-Teh Chang 
Hi Ronald,

On Sat, Jul 8, 2017 at 6:19 AM, Ronald S. Bultje  wrote:
> Hi,
>
> On Fri, Jul 7, 2017 at 5:02 PM, Wan-Teh Chang 
> wrote:
>
>> @@ -763,6 +764,7 @@ int ff_frame_thread_init(AVCodecContext *avctx)
>>
>>  fctx->async_lock = 1;
>>  fctx->delaying = 1;
>> +fctx->debug_threads = (avctx->debug & FF_DEBUG_THREADS) != 0;
>
> Shouldn't this be done in update_context_from_user()?

This patch differs from the approach you outlined at the end of
http://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213259.html
as follows:

* The debug_threads field is added to FrameThreadContext and applies to
  all the decoding threads owned by the FrameThreadContext.
* The debug_threads field is set when avcodec_open2() is called, and
  never changes thereafter.

In this design, we just need to set fctx->debug_threads in
ff_frame_thread_init() (which is called by avcodec_open2()).

Wan-Teh Chang
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/ylc: Fix vlc of 31 bits

2017-07-08 Thread Michael Niedermayer 
On Sat, Jul 08, 2017 at 10:59:02PM +0200, Paul B Mahol wrote:
> On 7/8/17, Michael Niedermayer  wrote:
> > Fixes: runtime error: left shift of 1 by 31 places cannot be represented in
> > type 'int'
> > Fixes: 2515/clusterfuzz-testcase-minimized-6197200012967936
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavcodec/ylc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> 
> ok

will apply

thx

[...]
-- 
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 2/3] avcodec/sbrdsp_fixed: Fix integer overflow in sbr_hf_apply_noise()

2017-07-08 Thread Michael Niedermayer 
On Sun, Jul 02, 2017 at 04:28:55AM +0200, Michael Niedermayer wrote:
> Fixes: runtime error: signed integer overflow: -2049425300 + -117591631 
> cannot be represented in type 'int'
> Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/sbrdsp_fixed.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Will apply this patch of the patchset

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

2017-07-08 Thread Michael Niedermayer 
On Mon, Jul 03, 2017 at 01:37:09AM +0200, Michael Niedermayer wrote:
> On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote:
> > On 2 July 2017 at 03:28, Michael Niedermayer  wrote:
> > 
> > > Fixes: runtime error: signed integer overflow: 1965219850 + 995792909
> > > cannot be represented in type 'int'
> > > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
> > >
> > > Found-by: continuous fuzzing process https://github.com/google/oss-
> > > fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by
> > > :
> > > Michael Niedermayer 
> > > ---
> > >  libavcodec/aacpsdsp_template.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
> > > template.c
> > > index 9e1a95c1a1..2c0afd4512 100644
> > > --- a/libavcodec/aacpsdsp_template.c
> > > +++ b/libavcodec/aacpsdsp_template.c
> > > @@ -26,9 +26,10 @@
> > >  #include "libavutil/attributes.h"
> > >  #include "aacpsdsp.h"
> > >
> > > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT (*src)[2], int
> > > n)
> > > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
> > > (*src)[2], int n)
> > >  {
> > >  int i;
> > > +SUINTFLOAT *dst = dst_param;
> > >  for (i = 0; i < n; i++)
> > >  dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1], src[i][1]);
> > >  }
> > >
> > >
> > What's the issue with just _not_ fixing it here? It only occurs on fuzzed
> > inputs, doesn't crash on any known platform ever, makes the code uglier and
> > why? Because some fuzzer is super pedantic.
> > Why not fix the fuzzer? Why not just mark this as a false positive, since
> > fixing it is pointless from the standpoint of security (you can't exploit
> > overflows in transforms or functions like this), and all developers hate it.
> 
> Iam not sure you understand the problem.
> signed integer overflows are undefined behavior, undefined behavior
> is not allowed in C.
> 
> 
> Theres also no option to mark anything as false positive.
> If the tool makes a mistake, the issue needs to be reported to its
> authors and needs to be fixed.
> I belive the tool is correct, if someone thinks its not correct, the
> place to report this to is likely where the clang sanitizers are
> developed.
> 
> I do understand that this is annoying but this is how C works.
> 
> About "doesn't crash on any known platform ever",
> "you can't exploit  overflows in transforms or functions like this"
> 
> undefined behavior has bitten people with unexpected results in the
> past. for example "if (a+b < a)" to test for overflow, but because the 
> condition
> can only be true in case of overflow and overflow isnt allowed in C
> the compiler didnt assemble this the way the human thought.
> 
> In the case of ps_add_squares_c(), dst[i] has to increase if iam
> not mistaken. In case of overflow it can decrease but overflow is
> not allowed so a compiler can prune anything that implies dst[] to be
> negative.
> dst[] is used downstream in checks of greater / less and in FFMAX
> In this code the compiler can assume that the sign bit is clear,
> what happens when  the compilers optimizer has false assumtations
> i dont know but i know its not good.
> 
> That said even if no such chain of conditions exist its still invalid
> code if theres undefined behavior in it

Does anyone object to this patch ?
Or does anyone have a better idea on how to fix this ?
if not id like to apply it

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Add FITS Demuxer

2017-07-08 Thread Reimar Döffinger 
On Tue, Jul 04, 2017 at 09:50:31PM +0530, Paras Chadha wrote:
> On Tue, Jul 4, 2017 at 4:12 AM, Reimar Döffinger 
> wrote:
> > > +data_size *= naxisn[i];
> > > +if (data_size < 0)
> > > +return AVERROR_INVALIDDATA;
> >
> > If this is supposed to be overlow check as well: same issue as before: you
> > need to PREVENT the overflow, afterwards it's too late, at least for signed
> > types.
> > Also the golden rule of input sanitization: sanitize/range check FIRST,
> > THEN do the arithmetic.
> > NEVER do it the other way run without at least spending 30 minutes per
> > operation making sure it's really still safe.
> >
> 
> Actually, here also overflow is not possible unless someone intentionally
> put values of naxisn[i] that would cause overflow. Most of the FITS files
> are in MB's or at max in GB's (although i have not seen any in GB). So, if
> someone intentionally put wrong values of size in header then it would
> crash or cause undefined behavior.

crash == usually exploitable
undefined behaviour == possibly exploitable with risk of becoming
exploitable at any point of time with new compiler versions etc.

Both are completely unacceptable.

> Just to prevent that i have added a
> check.

The problem is, your check doesn't prevent it.
It SOMETIMES MIGHT detect it, AFTER it happened.
You can reduce the problem somewhat by using unsigned types,
then at least the overflow would not be undefined behaviour,
but then you have lots of weird corner-case that ALL the
following code must ALWAYS deal correctly with.
For example (not that you would have this specific code in FFmpeg):
ptr = malloc(data_size);
fread(ptr, 1, naxisn[0], somefile);
would be exploitable if you had an overflow.

> So, can suggest a better way to do this ?

As said, always check the inputs BEFORE use.
One way would be
if (naxisn[i] > INT_MAX / data_size) goto err_out;
However you still have to carefully decide on
which limit value (INT_MAX in my example) is the
right compromise between limiting use-cases and
making the code easy to write and maintain.
If you choose it too close, the following code
can't even safely do "data_size + 1" without
another overflow check.
av_image_check_size might be a guideline,
as well as the default value of max_alloc_size.

> Regarding range check, FITS
> standard does not have any limit on the range of values of dimension,
> theoretically it can be anything but practically ofcourse it cannot exceed
> the limits of 64 bit integer. So, how should i go about this ?

As you can see from max_alloc_size, anything
that results in more than 2 GB in a single
packet won't really work by default anyway.
(3D images COULD have their 3rd dimension split
up into separate packages for example, but the
might be something better to leave for later?).

> > > +fits->image = 0;
> > > +pos = avio_tell(s->pb);
> > > +while (!fits->image) {
> > > +if ((ret = avio_seek(s->pb, pos+size, SEEK_SET)) < 0)
> > > +return ret;
> > > +
> > > +if (avio_feof(s->pb))
> > > +return AVERROR_EOF;
> > > +
> > > +pos = avio_tell(s->pb);
> > > +if ((size = find_size(s->pb, fits)) < 0)
> > > +return size;
> > > +}
> > > +
> > > +if ((ret = avio_seek(s->pb, pos, SEEK_SET)) < 0)
> > > +return ret;
> >
> > Does this seek ever do anything?
> >
> 
> Yes, it seeks back the pointer to point to the begining of the header.

Yes, I failed at reading code (missed that the find_size does
the whole header reading).
Maybe you should just be using av_append_packet while reading the
header? So that when you are done with parsing the header, you
already have all the header data in the packet and the just need
to use av_append_packet again to get the image data itself.
(av_append_packet is not perfect/the most efficient way of doing it,
but if the last bit of speed does not matter so much it tends
to be easy way to avoid seeking in demuxer without increasing complexity
much, sometimes even simplifying things).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/htmlsubtitles: Be a bit more picky on syntax

2017-07-08 Thread Michael Niedermayer 
On Mon, Jul 03, 2017 at 05:50:08PM +0200, Michael Niedermayer wrote:
> On Mon, Jul 03, 2017 at 12:40:13PM +0200, wm4 wrote:
> > On Sun, 2 Jul 2017 13:43:57 +0200
> > Michael Niedermayer  wrote:
> > 
> > > On Sun, Jul 02, 2017 at 01:14:00PM +0200, wm4 wrote:
> > > > On Sun,  2 Jul 2017 00:09:42 +0200
> > > > Michael Niedermayer  wrote:
> > > >   
> > > > > This reduces the number of strstr() calls per byte
> > > > > This diasalows empty tags like '< >' as well as '<' in tags like 
> > > > > ' > > > > 
> > > > > Fixes timeout
> > > > > Fixes: 1817/clusterfuzz-testcase-minimized-5104230530547712
> > > > > 
> > > > > Found-by: continuous fuzzing process 
> > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer 
> > > > > ---
> > > > >  libavcodec/htmlsubtitles.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/libavcodec/htmlsubtitles.c b/libavcodec/htmlsubtitles.c
> > > > > index be5c9316ca..67abc94085 100644
> > > > > --- a/libavcodec/htmlsubtitles.c
> > > > > +++ b/libavcodec/htmlsubtitles.c
> > > > > @@ -110,13 +110,13 @@ int ff_htmlmarkup_to_ass(void *log_ctx, 
> > > > > AVBPrint *dst, const char *in)
> > > > >  case '<':
> > > > >  tag_close = in[1] == '/';
> > > > >  len = 0;
> > > > > -if (sscanf(in+tag_close+1, "%127[^>]>%n", buffer, ) 
> > > > > >= 1 && len > 0) {
> > > > > +if (sscanf(in+tag_close+1, "%127[^<>]>%n", buffer, ) 
> > > > > >= 1 && len > 0) {
> > > > >  const char *tagname = buffer;
> > > > >  while (*tagname == ' ')
> > > > >  tagname++;
> > > > >  if ((param = strchr(tagname, ' ')))
> > > > >  *param++ = 0;
> > > > > -if ((!tag_close && sptr < FF_ARRAY_ELEMS(stack)) ||
> > > > > +if ((!tag_close && sptr < FF_ARRAY_ELEMS(stack) && 
> > > > > *tagname) ||
> > > > >  ( tag_close && sptr > 0 && 
> > > > > !strcmp(stack[sptr-1].tag, tagname))) {
> > > > >  int i, j, unknown = 0;
> > > > >  in += len + tag_close;  
> > > > 
> > > > Invalid syntax is not unusual in SRT files. Are you sure this doesn't
> > > > make the output worse in files that do not use the syntax correctly?  
> > > 
> > > I do not know if this makes the output worse for files with invalid syntax
> > > I also do not know if this makes the output better for files with invalid
> > > syntax
> > > 
> > > i dont have a large library of (real world invalid syntax) srt files
> > > whith which to find cases where it makes a difference.
> > > 
> > > You seem to know alot about srt, maybe you can pick some srt files
> > > and add fate tests, so we have better coverage of odd and nasty cases
> > > 
> > > about this patch specifically, what do you suggest ?
> > > should i try to fix this while maintaining existing behavior for
> > > invalid syntax exactly? (i dont know how that code would look)
> > > 
> > > [...]
> > 
> > I don't know either, but due to the messy syntax situation, I'd rather
> > not change the code in unpredictable ways just to get faster fuzzing.
> 
> Its primarly intended to fix the potential denial of service.
> That is moderate sized files with short duration, few streams, low
> resolution, low channel count should not consume huge amounts of
> cpu time.

5 days passed with no comments and no suggestions on how to implement
this better.
does anyone object to the fix as in the patch to be applied ?

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/ylc: Fix vlc of 31 bits

2017-07-08 Thread Paul B Mahol 
On 7/8/17, Michael Niedermayer  wrote:
> Fixes: runtime error: left shift of 1 by 31 places cannot be represented in
> type 'int'
> Fixes: 2515/clusterfuzz-testcase-minimized-6197200012967936
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/ylc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

ok
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec/ylc: Fix vlc of 31 bits

2017-07-08 Thread Michael Niedermayer 
Fixes: runtime error: left shift of 1 by 31 places cannot be represented in 
type 'int'
Fixes: 2515/clusterfuzz-testcase-minimized-6197200012967936

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
 libavcodec/ylc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/ylc.c b/libavcodec/ylc.c
index bf55e37be1..ae46b3b8c2 100644
--- a/libavcodec/ylc.c
+++ b/libavcodec/ylc.c
@@ -69,7 +69,7 @@ static void get_tree_codes(uint32_t *bits, int16_t *lens, 
uint8_t *xlat,
 
 s = nodes[node].sym;
 if (s != -1) {
-bits[*pos] = (~pfx) & ((1 << FFMAX(pl, 1)) - 1);
+bits[*pos] = (~pfx) & ((1U << FFMAX(pl, 1)) - 1);
 lens[*pos] = FFMAX(pl, 1);
 xlat[*pos] = s + (pl == 0);
 (*pos)++;
-- 
2.13.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Add FITS Demuxer

2017-07-08 Thread Reimar Döffinger 
On Sat, Jul 08, 2017 at 12:59:09PM +0200, Nicolas George wrote:
> Le sextidi 16 messidor, an CCXXV, Paras Chadha a écrit :
> > So, now should i do this ?
> 
> Based on what you explained here, FITS seems like just an image format,
> without provisions for animations like GIF or PNG. Therefore, it should
> have been integrated in the img2 framework in the first place and
> writing a dedicated demuxer was a mistake.

I don't think that's a correct description then.
First, the format is made to change and be extended, so what is
true now might not stay true.
But also the images can have arbitrary dimensions, in particular
they can be "3D" images with the third dimension being time,
thus being a video.
This may not be well enough specified for the demuxer to be
able to produce a proper "video stream" at this point,
but implementing it in the img2 framework to me seems to
have a significant risk of turning out a dead-end.
___
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 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

Re: [FFmpeg-devel] [PATCH] avcodec/noise_bsf: add support for dropping packets

2017-07-08 Thread Michael Niedermayer 
On Sat, Jul 08, 2017 at 01:21:32PM +0200, Marton Balint wrote:
> Signed-off-by: Marton Balint 
> ---
>  doc/bitstream_filters.texi | 14 +++---
>  libavcodec/noise_bsf.c |  8 
>  libavcodec/version.h   |  2 +-
>  3 files changed, 20 insertions(+), 4 deletions(-)

should be ok

supporting duplicatig random previous packets might be interresting
too

thx

PS: these should also be tested in fate

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libswresample: check input to swr_convert_frame for NULL

2017-07-08 Thread Michael Niedermayer 
On Sat, Jul 08, 2017 at 03:56:53PM +0200, hexpointer wrote:
> > Sent: Saturday, July 08, 2017 at 2:19 AM
> > From: "Michael Niedermayer" 
> > To: "FFmpeg development discussions and patches" 
> > Subject: Re: [FFmpeg-devel] [PATCH] libswresample: check input to 
> > swr_convert_frame for NULL
> >
> > Is the author value as intended ?
> > "Author: hexpointer "
> > Theres no name
> > 
> > (it cannot be changed after pushing)
> 
> Yes. Pseudonym only please.

ok, applied

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH v2 1/2] pixdesc: Improve scoring for opaque/unknown pixel formats

2017-07-08 Thread Mark Thompson 
Hardware pixel formats do not tell you anything about their actual
contents, but should still score higher than formats with completely
unknown properties, which in turn should score higher than invalid
formats.

Do not return an AVERROR code as a score.

Fixes a hang in libavfilter where format negotiation gets stuck in a
loop because AV_PIX_FMT_NONE scores more highly than all other
possibilities.
---
On 07/07/17 01:14, Michael Niedermayer wrote:
> On Thu, Jul 06, 2017 at 10:59:24PM +0100, Mark Thompson wrote:
>> ...
>> +if ((src_desc->flags & AV_PIX_FMT_FLAG_HWACCEL) ||
>> +(dst_desc->flags & AV_PIX_FMT_FLAG_HWACCEL))
>> +return 0;
> 
> this breaks ffmpegs choose_pixel_fmt()
> src_desc being NULL

Where does it ever get called with invalid src?  I think it would probably be 
better to fix those cases in the caller.  (It would be asking "of the pixel 
formats A and B, which is a better match for conversion from ", which 
is nonsensical.)

In any case, I've changed the invalid format check to src_desc && dst_desc to 
make this more robust.

>>  
>>  /* compute loss */
>>  *lossp = loss = 0;
> 
> shouldnt this be set on a 0 return ?
> 
> i also wonder if src and dst being identical should score different
> than if not if either is AV_PIX_FMT_FLAG_HWACCEL

Changed to:

-1  Matching hardware formats.
-2  Non-matching hardware formats.
-3  One of the input formats doesn't have enough metadata to return anything 
useful.
-4  One of the input formats is completely invalid.


 libavutil/pixdesc.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
index 46a7eff06d..1983ce9ef5 100644
--- a/libavutil/pixdesc.c
+++ b/libavutil/pixdesc.c
@@ -2511,8 +2511,16 @@ static int get_pix_fmt_score(enum AVPixelFormat 
dst_pix_fmt,
 int ret, loss, i, nb_components;
 int score = INT_MAX - 1;
 
-if (dst_pix_fmt >= AV_PIX_FMT_NB || dst_pix_fmt <= AV_PIX_FMT_NONE)
-return ~0;
+if (!src_desc || !dst_desc)
+return -4;
+
+if ((src_desc->flags & AV_PIX_FMT_FLAG_HWACCEL) ||
+(dst_desc->flags & AV_PIX_FMT_FLAG_HWACCEL)) {
+if (dst_pix_fmt == src_pix_fmt)
+return -1;
+else
+return -2;
+}
 
 /* compute loss */
 *lossp = loss = 0;
@@ -2521,9 +2529,9 @@ static int get_pix_fmt_score(enum AVPixelFormat 
dst_pix_fmt,
 return INT_MAX;
 
 if ((ret = get_pix_fmt_depth(_min_depth, _max_depth, src_pix_fmt)) 
< 0)
-return ret;
+return -3;
 if ((ret = get_pix_fmt_depth(_min_depth, _max_depth, dst_pix_fmt)) 
< 0)
-return ret;
+return -3;
 
 src_color = get_color_type(src_desc);
 dst_color = get_color_type(dst_desc);
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH v2 2/2] pixdesc: Add a test for av_find_best_pix_fmt_of_2()

2017-07-08 Thread Mark Thompson 
---
On 07/07/17 01:42, Michael Niedermayer wrote:
> On Thu, Jul 06, 2017 at 11:00:31PM +0100, Mark Thompson wrote:
>> ---
>>  libavutil/Makefile|   1 +
>>  libavutil/tests/pixfmt_best.c | 115 
>> ++
>>  tests/fate/libavutil.mak  |   4 ++
>>  tests/ref/fate/pixfmt_best|   1 +
>>  4 files changed, 121 insertions(+)
>>  create mode 100644 libavutil/tests/pixfmt_best.c
>>  create mode 100644 tests/ref/fate/pixfmt_best
> 
> i think this is missing a test for monochrome, and maybe others

Added monochrome and a few other cases.

Thanks,

- Mark


 libavutil/Makefile|   1 +
 libavutil/tests/pixfmt_best.c | 125 ++
 tests/fate/libavutil.mak  |   4 ++
 tests/ref/fate/pixfmt_best|   1 +
 4 files changed, 131 insertions(+)
 create mode 100644 libavutil/tests/pixfmt_best.c
 create mode 100644 tests/ref/fate/pixfmt_best

diff --git a/libavutil/Makefile b/libavutil/Makefile
index b4464b0d76..b2662c843e 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -218,6 +218,7 @@ TESTPROGS = adler32 
\
 parseutils  \
 pixdesc \
 pixelutils  \
+pixfmt_best \
 random_seed \
 rational\
 ripemd  \
diff --git a/libavutil/tests/pixfmt_best.c b/libavutil/tests/pixfmt_best.c
new file mode 100644
index 00..a617633e9b
--- /dev/null
+++ b/libavutil/tests/pixfmt_best.c
@@ -0,0 +1,125 @@
+/*
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/pixdesc.c"
+
+static const enum AVPixelFormat pixfmt_list[] = {
+AV_PIX_FMT_MONOWHITE,
+AV_PIX_FMT_GRAY8,
+AV_PIX_FMT_GRAY10,
+AV_PIX_FMT_GRAY16,
+AV_PIX_FMT_YUV420P,
+AV_PIX_FMT_YUV420P10,
+AV_PIX_FMT_YUV420P16,
+AV_PIX_FMT_YUV422P,
+AV_PIX_FMT_YUV422P10,
+AV_PIX_FMT_YUV422P16,
+AV_PIX_FMT_YUV444P,
+AV_PIX_FMT_YUV444P10,
+AV_PIX_FMT_YUV444P16,
+AV_PIX_FMT_RGB565,
+AV_PIX_FMT_RGB24,
+AV_PIX_FMT_RGB48,
+AV_PIX_FMT_VDPAU,
+AV_PIX_FMT_VAAPI,
+};
+
+static enum AVPixelFormat find_best(enum AVPixelFormat pixfmt)
+{
+enum AVPixelFormat best = AV_PIX_FMT_NONE;
+int i;
+for (i = 0; i < FF_ARRAY_ELEMS(pixfmt_list); i++)
+best = av_find_best_pix_fmt_of_2(best, pixfmt_list[i],
+ pixfmt, 0, NULL);
+return best;
+}
+
+int main(void)
+{
+enum AVPixelFormat output;
+int i, pass = 0, fail = 0;
+
+#define TEST(input, expected) do {  \
+output = find_best(input);  \
+if (output != expected) {   \
+printf("Matching %s: got %s, expected %s\n",\
+   av_get_pix_fmt_name(input),  \
+   av_get_pix_fmt_name(output), \
+   av_get_pix_fmt_name(expected));  \
+++fail; \
+} else  \
+++pass; \
+} while (0)
+
+// Same formats.
+for (i = 0; i < FF_ARRAY_ELEMS(pixfmt_list); i++)
+TEST(pixfmt_list[i], pixfmt_list[i]);
+
+// Formats containing the same data in different layouts.
+TEST(AV_PIX_FMT_MONOBLACK, AV_PIX_FMT_MONOWHITE);
+TEST(AV_PIX_FMT_NV12,  AV_PIX_FMT_YUV420P);
+TEST(AV_PIX_FMT_P010,  AV_PIX_FMT_YUV420P10);
+TEST(AV_PIX_FMT_P016,  AV_PIX_FMT_YUV420P16);
+TEST(AV_PIX_FMT_NV16,  AV_PIX_FMT_YUV422P);
+TEST(AV_PIX_FMT_YUYV422,   AV_PIX_FMT_YUV422P);
+TEST(AV_PIX_FMT_UYVY422,   AV_PIX_FMT_YUV422P);
+TEST(AV_PIX_FMT_BGR565,AV_PIX_FMT_RGB565);
+TEST(AV_PIX_FMT_BGR24, AV_PIX_FMT_RGB24);
+

Re: [FFmpeg-devel] [PATCH] libswresample: check input to swr_convert_frame for NULL

2017-07-08 Thread hexpointer 
> Sent: Saturday, July 08, 2017 at 2:19 AM
> From: "Michael Niedermayer" 
> To: "FFmpeg development discussions and patches" 
> Subject: Re: [FFmpeg-devel] [PATCH] libswresample: check input to 
> swr_convert_frame for NULL
>
> Is the author value as intended ?
> "Author: hexpointer "
> Theres no name
> 
> (it cannot be changed after pushing)

Yes. Pseudonym only please.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pthread_frame: save the FF_DEBUG_THREADS option in FrameThreadContext.

2017-07-08 Thread Ronald S. Bultje 
Hi,

On Fri, Jul 7, 2017 at 5:02 PM, Wan-Teh Chang 
wrote:

> @@ -763,6 +764,7 @@ int ff_frame_thread_init(AVCodecContext *avctx)
>
>  fctx->async_lock = 1;
>  fctx->delaying = 1;
> +fctx->debug_threads = (avctx->debug & FF_DEBUG_THREADS) != 0;


Shouldn't this be done in update_context_from_user()?

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pthread_frame: save the FF_DEBUG_THREADS option in FrameThreadContext.

2017-07-08 Thread Ronald S. Bultje 
Hi,

On Fri, Jul 7, 2017 at 5:31 PM, Ronald S. Bultje  wrote:

> Hi,
>
> On Fri, Jul 7, 2017 at 5:30 PM, Wan-Teh Chang <
> wtc-at-google@ffmpeg.org> wrote:
>
>> Note: I suspect we can simply delete the following line from
>> update_context_from_user() in libavcodec/pthread_frame.c:
>>
>> dst->debug= src->debug;
>>
>> That also fixes the tsan warning, but it'll take more time to
>> investigate whether it is necessary to update the |debug| field from
>> the user's AVCodecContext (src).
>>
>> That line in update_context_from_user() was added in the initial
>> commit of libavcodec/pthread.c:
>>
>> http://git.videolan.org/?p=ffmpeg.git;a=commit;h=37b00b47cbe
>> ecd66bb34c5c7c534d016d6e8da24
>>
>> Does any user actually modify avctx->debug after the avcodec_open2() call?
>
>
> To sync values of debug between worker threads if the user dynamically
> toggles bits in this flag.
>

Hm, I misread your question yesterday, sorry about that. Yes, users can
dynamically toggle this flag. Whether they do is a good question, but we'd
typically consider it a regression if this breaks. Since it's not hard to
keep it working, I'd prefer to keep it working.

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec/noise_bsf: add support for dropping packets

2017-07-08 Thread Marton Balint 
Signed-off-by: Marton Balint 
---
 doc/bitstream_filters.texi | 14 +++---
 libavcodec/noise_bsf.c |  8 
 libavcodec/version.h   |  2 +-
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
index 926610ca7b..2dffe021f9 100644
--- a/doc/bitstream_filters.texi
+++ b/doc/bitstream_filters.texi
@@ -220,19 +220,27 @@ ffmpeg -i INPUT.avi -codec copy -bsf:v 
mpeg4_unpack_bframes OUTPUT.avi
 
 @section noise
 
-Damages the contents of packets without damaging the container. Can be
-used for fuzzing or testing error resilience/concealment.
+Damages the contents of packets or simply drops them without damaging the
+container. Can be used for fuzzing or testing error resilience/concealment.
 
 Parameters:
+@table @option
+@item amount
 A numeral string, whose value is related to how often output bytes will
 be modified. Therefore, values below or equal to 0 are forbidden, and
 the lower the more frequent bytes will be modified, with 1 meaning
 every byte is modified.
+@item dropamount
+A numeral string, whose value is related to how often packets will be dropped.
+Therefore, values below or equal to 0 are forbidden, and the lower the more
+frequent packets will be dropped, with 1 meaning every packet is dropped.
+@end table
 
+The following example applies the modification to every byte but does not drop
+any packets.
 @example
 ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
 @end example
-applies the modification to every byte.
 
 @section null
 This bitstream filter passes the packets through unchanged.
diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
index 0aebee1ad6..84b94032ad 100644
--- a/libavcodec/noise_bsf.c
+++ b/libavcodec/noise_bsf.c
@@ -31,6 +31,7 @@
 typedef struct NoiseContext {
 const AVClass *class;
 int amount;
+int dropamount;
 unsigned int state;
 } NoiseContext;
 
@@ -48,6 +49,12 @@ static int noise(AVBSFContext *ctx, AVPacket *out)
 if (ret < 0)
 return ret;
 
+if (s->dropamount > 0 && s->state % s->dropamount == 0) {
+s->state++;
+av_packet_free();
+return AVERROR(EAGAIN);
+}
+
 ret = av_new_packet(out, in->size);
 if (ret < 0)
 goto fail;
@@ -73,6 +80,7 @@ fail:
 #define OFFSET(x) offsetof(NoiseContext, x)
 static const AVOption options[] = {
 { "amount", NULL, OFFSET(amount), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 
INT_MAX },
+{ "dropamount", NULL, OFFSET(dropamount), AV_OPT_TYPE_INT, { .i64 = 0 }, 
0, INT_MAX },
 { NULL },
 };
 
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 3c5fea9327..096b062e97 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@
 
 #define LIBAVCODEC_VERSION_MAJOR  57
 #define LIBAVCODEC_VERSION_MINOR 100
-#define LIBAVCODEC_VERSION_MICRO 103
+#define LIBAVCODEC_VERSION_MICRO 104
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
LIBAVCODEC_VERSION_MINOR, \
-- 
2.12.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Add FITS Demuxer

2017-07-08 Thread Nicolas George 
Le sextidi 16 messidor, an CCXXV, Paras Chadha a écrit :
> So, now should i do this ?

Based on what you explained here, FITS seems like just an image format,
without provisions for animations like GIF or PNG. Therefore, it should
have been integrated in the img2 framework in the first place and
writing a dedicated demuxer was a mistake.

>Even if i am able to call read_header() from
> there, i will have to modify read_header() a lot and it will become very
> complex and long.

I do not see why you would need to modify fits_read_header() a lot:
fits_read_header() populates the FITSContext structure, and the parser
uses the fields to compute the size. Done.

You may need to make minimal changes to fits_read_header() to prevent it
from doing extra works, for example building metadata that will not be
used, but that is very minor.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital 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

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