Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(318443)

Issue 1355: Simplifies the Q15 adaptation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 4 months ago by jm
Modified:
3 years, 4 months ago
Reviewers:
derf
Visibility:
Public.

Description

We move the floor subtract/add to the tmp value. Also, we add an offset such
that the floor can actually reach 1/ft even for the first and last symbol.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M src/generic_code.c View 2 chunks +10 lines, -11 lines 3 comments Download

Messages

Total messages: 2
jm
3 years, 4 months ago #1
derf
3 years, 4 months ago #2
r=me with nits fixed.

http://review.xiph.org/1355/diff/2275/src/generic_code.c
File src/generic_code.c (right):

http://review.xiph.org/1355/diff/2275/src/generic_code.c#newcode54
src/generic_code.c:54: not 32768. */
This isn't grammatically correct ("...cdf[0] be..."), and still doesn't really
explain where the values below come from. There are a lot more than two terms,
so behavior at two points isn't really sufficient to explain them (and I don't
think it's obvious why the behaviors you've listed are required). The mixed use
of Matlab and C notation isn't helping matters any.

How about something like this:

"When (i < val), we want the adjustment ((cdf[i] - tmp) >> rate) to be positive
so long as (cdf[i] > i + 1), and 0 when (cdf[i] == i + 1), to ensure we don't
drive any probabilities to 0. Replacing cdf[i] with (i + 2) and solving ((i + 2
- tmp) >> rate == 1) for tmp produces tmp == i + 2 - (1 << rate). Using this
value of tmp with cdf[i] == i + 1 instead gives an adjustment of 0 as desired.

When (i >= val), we want ((cdf[i] - tmp) >> rate) to be negative so long as
cdf[i] < 32768 - (n - 1 - i), and 0 when cdf[i] == 32768 - (n - 1 - i), again to
ensure we don't drive any probabilities to 0. Since right-shifting any negative
value is still negative, we can solve (32768 - (n - 1 - i) - tmp == 0) for tmp,
producing tmp = 32769 - n + i. Using this value of tmp with smaller values of
cdf[i] instead gives negative adjustments, as desired.

Combining the two cases gives the expression below. These could be stored in a
lookup table indexed by n and rate to avoid the arithmetic."

http://review.xiph.org/1355/diff/2275/src/generic_code.c#newcode55
src/generic_code.c:55: tmp = 2 - (1<<rate) + i + (32767 + (1<<rate) - n)*(i >=
val);
Also, it wasn't obvious to me at first, but it looks like this expression tends
to adjust values near "val" much more than those farther away (until it starts
running into the rails of no adjustment), which is an interesting property.

http://review.xiph.org/1355/diff/2275/src/generic_code.c#newcode68
src/generic_code.c:68: cdf[i] -= ((cdf[i] - tmp)*alpha) >> 15;
Removing the rounding offset was not something you mentioned in the commit
message. I presume you tested and it didn't matter, but it would be good to
actually say so somewhere.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld