| Message ID | CABA=pqfOTp7uq6W9_V5Q8F820FuxOPOE-XimK44ieDtcA9rd8Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 25 August 2017 at 16:38, Ivan Kalvachev <ikalvachev@gmail.com> wrote: > Instead of returning all zeroes as result and Syy=1.0, > place all the K pulses in the first element y[0] > and return Syy=K*K. > > This is how the original opus function handles the case. > This is how the existing pvq_search_c handles the case. > > Also, according to Rostislav, the encoded all zeros vector > would be decoded as such y[0]=K vector, before dequantization. > So it is better to do that explicitly and calculate > the proper gain in the encoder. > -- > > I must point out that ppp_pvq_search_c() does generate > y[0]=-K vector, not +K. > This is because FFSIGN(0.0) returns -1. > I do consider this bug, however I'm not quite sure what > is the best way to handle it. > > I don't think its a bug, celt_cwrsi() also has the same behavior (and doesn't use FFSIGN) so FF_SIGN returning -1 in the encoder matches what celt_cwrsi() would see. Could you send a V2 which outputs a -K in y[0] instead?
On 8/25/17, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On 25 August 2017 at 16:38, Ivan Kalvachev <ikalvachev@gmail.com> wrote: > >> Instead of returning all zeroes as result and Syy=1.0, >> place all the K pulses in the first element y[0] >> and return Syy=K*K. >> >> This is how the original opus function handles the case. >> This is how the existing pvq_search_c handles the case. >> >> Also, according to Rostislav, the encoded all zeros vector >> would be decoded as such y[0]=K vector, before dequantization. >> So it is better to do that explicitly and calculate >> the proper gain in the encoder. >> -- >> >> I must point out that ppp_pvq_search_c() does generate >> y[0]=-K vector, not +K. >> This is because FFSIGN(0.0) returns -1. >> I do consider this bug, however I'm not quite sure what >> is the best way to handle it. >> >> > I don't think its a bug, It is most definitely a bug. e.g. Float point has positive and negative zeros, and this FFSIGN() turns both into negatives. >celt_cwrsi() also has the same behavior (and > doesn't use FFSIGN) so FF_SIGN returning -1 in the encoder matches what > celt_cwrsi() would see. I suspect that this might be celt special case value, so K + (-K) would decode to vector that is all zeroes, including y[0]. Could you check that. The bitstream encoding scheme is really hard to figure out, with all these self referencing pre-calculated tables. Is there a description how this stuff works? > Could you send a V2 which outputs a -K in y[0] instead? I would really prefer if you commit this patch as it is. It handles the special case in the same way as celt opus does, so it is more likely to be correct. To get the bug-for-bug behavior of ppp_pvq_search_c() just add "neg Kd" right before the "mov [outYq], Kd" (around line 367). and commit it as your own change. Best Regards
On 8/25/17, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On 25 August 2017 at 16:38, Ivan Kalvachev <ikalvachev@gmail.com> wrote: > >> Instead of returning all zeroes as result and Syy=1.0, >> place all the K pulses in the first element y[0] >> and return Syy=K*K. >> >> This is how the original opus function handles the case. >> This is how the existing pvq_search_c handles the case. >> >> Also, according to Rostislav, the encoded all zeros vector >> would be decoded as such y[0]=K vector, before dequantization. >> So it is better to do that explicitly and calculate >> the proper gain in the encoder. >> -- >> >> I must point out that ppp_pvq_search_c() does generate >> y[0]=-K vector, not +K. >> This is because FFSIGN(0.0) returns -1. >> I do consider this bug, however I'm not quite sure what >> is the best way to handle it. >> >> > I don't think its a bug, It is most definitely a bug. e.g. Float point has positive and negative zeros, and this FFSIGN() turns both into negatives. >celt_cwrsi() also has the same behavior (and > doesn't use FFSIGN) so FF_SIGN returning -1 in the encoder matches what > celt_cwrsi() would see. I suspect that this might be celt special case value, so K + (-K) would decode to vector that is all zeroes, including y[0]. Could you check that. The bitstream encoding scheme is really hard to figure out, with all these self referencing pre-calculated tables. Is there a description how this stuff works? > Could you send a V2 which outputs a -K in y[0] instead? I would really prefer if you commit this patch as it is. It handles the special case in the same way as celt opus does, so it is more likely to be correct. To get the bug-for-bug behavior of ppp_pvq_search_c() just add "neg Kd" right before the "mov [outYq], Kd" (around line 367). and commit it as your own change. Best Regards
From 33d29a33dcf5ad8c4850edf9ed8d83292f10b03f Mon Sep 17 00:00:00 2001 From: Ivan Kalvachev <ikalvachev@gmail.com> Date: Fri, 25 Aug 2017 17:14:28 +0300 Subject: [PATCH] opus_pvq_search.asm: Handle zero vector input differently. Instead of returning all zeroes as result and Syy=1.0, place all the K pulses in the first element y[0] and return Syy=K*K. This is how the original opus function handles the case. This is how the existing pvq_search_c handles the case. Also, according to Rostislav, the encoded all zeros vector would be decoded as such y[0]=K vector, before dequantization. So it is better to do that explicitly and calculate the proper gain in the encoder. Signed-off-by: Ivan Kalvachev <ikalvachev@gmail.com> --- libavcodec/x86/opus_pvq_search.asm | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libavcodec/x86/opus_pvq_search.asm b/libavcodec/x86/opus_pvq_search.asm index 5c1e6d6174..adf3e6f87c 100644 --- a/libavcodec/x86/opus_pvq_search.asm +++ b/libavcodec/x86/opus_pvq_search.asm @@ -252,9 +252,9 @@ align 16 xorps m0, m0 comiss xm0, xm1 ; + cvtsi2ss xm0, dword Kd ; m0 = K jz %%zero_input ; if (Sx==0) goto zero_input - cvtsi2ss xm0, dword Kd ; m0 = K %if USE_APPROXIMATION == 1 rcpss xm1, xm1 ; m1 = approx(1/Sx) mulss xm0, xm1 ; m0 = K*(1/Sx) @@ -355,7 +355,8 @@ align 16 RET align 16 -%%zero_input: +%%zero_input: ; expected m0 = K + mulss xm6, xm0, xm0 ; Syy_norm = K*K lea r4d, [Nd - mmsize] xorps m0, m0 %%zero_loop: @@ -363,7 +364,7 @@ align 16 sub r4d, mmsize jnc %%zero_loop - movaps m6, [const_float_1] + mov [outYq], Kd jmp %%return %endmacro -- 2.14.1
Instead of returning all zeroes as result and Syy=1.0, place all the K pulses in the first element y[0] and return Syy=K*K. This is how the original opus function handles the case. This is how the existing pvq_search_c handles the case. Also, according to Rostislav, the encoded all zeros vector would be decoded as such y[0]=K vector, before dequantization. So it is better to do that explicitly and calculate the proper gain in the encoder. -- I must point out that ppp_pvq_search_c() does generate y[0]=-K vector, not +K. This is because FFSIGN(0.0) returns -1. I do consider this bug, however I'm not quite sure what is the best way to handle it. 1. Fix localy #undef FFSIGN #define FFSIGN(a) ((a) >= 0 ? 1 : -1) 2. Use different name for that macro #define OPUS_SIGN(a) ... 3. Fix by special case in ppp_pvq_search_c(): if( !(res > 0.0) ) { y[0]=K; for(i=1;i<N;i++) y[i]=0; return K*K; } 4. Fix FFSIGN globally. That might have some side effects in addition of changing public API. Best Regards