On Sun, Dec 24, 2023 at 12:43:32AM +0100, Stefano Sabatini wrote: > On date Monday 2023-12-11 02:35:28 +0100, Clément Bœsch wrote: > > A few cosmetics aside, this makes the function identical to the one with > > the same name in proresenc_kostya. > > --- > > libavcodec/proresenc_anatoliy.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/libavcodec/proresenc_anatoliy.c > > b/libavcodec/proresenc_anatoliy.c > > index bdf7bface4..aed5c68b1b 100644 > > --- a/libavcodec/proresenc_anatoliy.c > > +++ b/libavcodec/proresenc_anatoliy.c > > @@ -257,7 +257,6 @@ static void encode_vlc_codeword(PutBitContext *pb, > > unsigned codebook, int val) > > > > #define GET_SIGN(x) ((x) >> 31) > > #define MAKE_CODE(x) (((x) * 2) ^ GET_SIGN(x)) > > -#define TO_GOLOMB2(val,sign) ((val)==0 ? 0 : ((val) << 1) + (sign)) > > > > static av_always_inline int get_level(int val) > > { > > @@ -271,7 +270,6 @@ static void encode_dcs(PutBitContext *pb, int16_t > > *blocks, > > { > > int i; > > int codebook = 5, code, dc, prev_dc, delta, sign, new_sign; > > - int diff_sign; > > > > prev_dc = (blocks[0] - 0x4000) / scale; > > encode_vlc_codeword(pb, FIRST_DC_CB, MAKE_CODE(prev_dc)); > > @@ -282,8 +280,8 @@ static void encode_dcs(PutBitContext *pb, int16_t > > *blocks, > > dc = (blocks[0] - 0x4000) / scale; > > delta = dc - prev_dc; > > new_sign = GET_SIGN(delta); > > > - diff_sign = new_sign ^ sign; > > - code = TO_GOLOMB2(get_level(delta), diff_sign); > > + delta = (delta ^ sign) - sign; > > + code = MAKE_CODE(delta); > > These don't look equivalent, > > MAKE_CODE((delta ^ sign) - sign) is equivalent to > TO_GOLOMB2(get_level(delta), sign) > > not to > TO_GOLOMB2(get_level(delta), diff_sign)
OK so this one is a bit tricky. Let's start from the specs, which states that the signed integer to symbol (code) mapping should be: 2|n| if n>=0 2|n|-1 if n<0 We also know that n>>31 is -1 if n < 0, and 0 if n>=0, which means the above condition can be simplified to: 2|n| + (n>>31) With prores_aw we have: s = -1 if different sign, 0 otherwise 2|n| + s Because: - get_level() is an absolute function¹ - the val==0 case doesn't matter because in this case s will also be 0 In prores_ks we have: n'=-n if different sign, n otherwise (2n')^sign(n') <=> 2|n'|-(n'>>31) So basically, aw does use the comparison with the previous delta and encodes it accordingly, while ks decides to swap the sign of n according to that previous delta, then encode it using its new sign. I wouldn't mind a third pair of eyes on the matter, but these look equivalent to me. Note that in practice I also tried to encode a bunch of frames from testsrc2 with and without the patch, and they are bit identical. ¹ I mis-replied earlier directly to Stefano in the get_level() patch, so the explanation for why get_leve() is equivalent to the absolute value: sign can only be -1 (if val < 0) or 0 (if val >= 0). Which means val^sign will either swap all bits or do nothing. And then it's pretty much what can be found here: https://graphics.stanford.edu/~seander/bithacks.html#IntegerAbs -- Clément B. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".