[FFmpeg-devel,27/35] avcodec/proresenc_anatoliy: remove TO_GOLOMB2()

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(-)

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)

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

On date Monday 2024-01-08 00:00:37 +0100, Clément Bœsch wrote:
> 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.

Thanks for the detailed explanation, I think it's safe to push
especially considering that there are no output changes.