[FFmpeg-devel] opus_pvq_search.asm: Handle zero vector input differently.

Submitted by Ivan Kalvachev on Aug. 25, 2017, 3:38 p.m.

Details

Message ID CABA=pqfOTp7uq6W9_V5Q8F820FuxOPOE-XimK44ieDtcA9rd8Q@mail.gmail.com
State New
Headers show

Commit Message

Ivan Kalvachev Aug. 25, 2017, 3:38 p.m.
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

Comments

Rostislav Pehlivanov Aug. 25, 2017, 3:48 p.m.
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?
Ivan Kalvachev Aug. 25, 2017, 7:32 p.m.
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
Ivan Kalvachev Aug. 25, 2017, 7:32 p.m.
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

Patch hide | download patch | download mbox

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