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

[Ivan Kalvachev]

On 8/25/17, Rostislav Pehlivanov <atomnuker at gmail.com> wrote:
> On 25 August 2017 at 16:38, Ivan Kalvachev <ikalvachev at 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