Golomb Rice description fixes #207
Conversation
JeromeMartinez
force-pushed
the
more-AD-review-updates4
branch
from
e7fb17a
to
9825999
Compare
|
About sign_extend, this maybe should mention twos complement somewhere |
|
applied the first patch of the 2 |
JeromeMartinez
force-pushed
the
more-AD-review-updates4
branch
from
9825999
to
b6d5af2
Compare
I added this wording in the section introduction, please review again. |
ffv1.md
Outdated
| sign_extend(value, bits) { | ||
| neg_bias = 1 << (bits - 1); | ||
| bits_mask = neg_bias - 1; | ||
| value &= bits_mask; // Remove neg bit |
There was a problem hiding this comment.
(previously?) fixed.
| @@ -689,6 +689,21 @@ if (run_count == 0 && run_mode == 1) { | | |||
|
|
|||
| The `log2_run` array is also used within [@ISO.14495-1.1999]. | |||
|
|
|||
| #### Sign extension | |||
There was a problem hiding this comment.
Should this go into Section 2.2.5 "Mathematical Functions"?
There was a problem hiding this comment.
IMO "Sign extension" is more an algorithm based on bit shifting (input is twos complement and we "convert" it to a number, mathematically speaking) than a mathematical function (the "Mathematical Functions" has a sign(a) definition, which is applied on numbers, and numbers are not always encoded in twos complement), so it should not be in "Mathematical Functions" which is for functions not having any mandatory encoding.
ffv1.md
Outdated
| @@ -689,6 +689,21 @@ if (run_count == 0 && run_mode == 1) { | | |||
|
|
|||
| The `log2_run` array is also used within [@ISO.14495-1.1999]. | |||
|
|
|||
| #### Sign extension | |||
|
|
|||
| `sign_extend` is the operation of increasing the number of bits of a binary number in twos complement signed number representation while preserving the number's sign (positive/negative) and value, in order to fit in the output bit width. It MAY be computed with: | |||
ffv1.md
Outdated
| negative_bias = 1 << (bits - 1); | ||
| bits_mask = negative_bias - 1; | ||
| value = value & bits_mask; // Remove negative bit | ||
| if (neg) |
There was a problem hiding this comment.
'neg' is a new term for this function. Does it mean 'if the value is negative'?
There was a problem hiding this comment.
I added the missing "neg" definition.
ffv1.md
Outdated
| sign_extend(value, bits) { | ||
| negative_bias = 1 << (bits - 1); | ||
| bits_mask = negative_bias - 1; | ||
| value = value & bits_mask; // Remove negative bit |
There was a problem hiding this comment.
I'm confused by this: Suppose I have value=11010 and bits=8.
The term 'bits' in the function is a bit ambiguous. Does it mean the bits of 'increasing the number of bits' or 'the output bit width'. In my example, I am already unsure if the output would be 8 bits in length or 13 (8 + the 5 bits of the value).
To continue the formula:
negative_bias = 1 << (8 - 1); so negative_bias= 10000000.
bits_mask = negative_bias - 1; so bits_mask = 1111111
value = value & bits_mask; so value = 11010 & 1111111 = 11010
The output is the same length as before. Where am I going wrong?
There was a problem hiding this comment.
You need a negative number, your example is positive so no change (the sign does not need to be extended as it is 0)
Example: value=11010 and bits=5 (this is the input size). output size is 8.
negative_bias = 1 << (5 - 1); so negative_bias = 10000.
bits_mask = negative_bias - 1; so bits_mask = 1111
value = value & bits_mask; so value = 11010 & 1111 = 1010
neg = value & bits_mask; so value = 11010 & 10000 = 10000
value -= negative_bias; so value = 1010 - 10000 = 11111010 (if twos complement)
The formula is neutral about the negative number handling by the CPU (could be twos complement, or not) and the width of the output (could be 8, or something else)
In practice you see that in binary and with twos complement CPUs, highest bits are filled with the negative bit value (1 if negative, else 0).
Note that if input and output size is same, the function does nothing (in reference decoder, if output is 8-bit so with bits == 8 it becomes the corresponding code, just a C type conversion without real CPU instruction called)
I adapted the introduction as well as the example code for being more explicit about input and output, does it make more sense?
JeromeMartinez
force-pushed
the
more-AD-review-updates4
branch
from
b6d5af2
to
33b1373
Compare
|
@dericed please review again. |
|
The first hunk seems unrelated |
JeromeMartinez
force-pushed
the
more-AD-review-updates4
branch
from
33b1373
to
c5ba6ad
Compare
I have split the commit in 2. |
Github: Closes FFmpeg#207
More AD review updates, these ones are focused on Golomb Rice description (1 section inversion & 1 function definition).
Fix #174.