[FFmpeg-devel] avutil: Rename RSHIFT macro to ROUNDED_RSHIFT

Submitted by FeRD (Frank Dana) on Jan. 21, 2019, 7:09 p.m.

Details

Message ID 20190121190936.24902-1-ferdnyc@gmail.com
State New
Headers show

Commit Message

FeRD (Frank Dana) Jan. 21, 2019, 7:09 p.m.
The RSHIFT macro in libavutil/common.h does not actually perform
a bitwise right-shift, but rather a rounded version of the same
operation, as is noted by a comment above the macro. The rounded
divsion macro on the very next line is named ROUNDED_DIV, which
seems far more clear.

This patch renames RSHIFT to ROUNDED_RSHIFT, then updates all
uses of the macro to use the new name. (These are all located
in three codec files under libavcodec/.)

Signed-off-by: FeRD (Frank Dana) <ferdnyc@gmail.com>
---
 libavcodec/mpeg4videodec.c |  4 ++--
 libavcodec/vp3.c           | 16 ++++++++--------
 libavcodec/vp56.c          |  4 ++--
 libavutil/common.h         |  2 +-
 4 files changed, 13 insertions(+), 13 deletions(-)

Comments

James Almer Jan. 21, 2019, 8:47 p.m.
On 1/21/2019 4:09 PM, FeRD (Frank Dana) wrote:
> The RSHIFT macro in libavutil/common.h does not actually perform
> a bitwise right-shift, but rather a rounded version of the same
> operation, as is noted by a comment above the macro. The rounded
> divsion macro on the very next line is named ROUNDED_DIV, which
> seems far more clear.
> 
> This patch renames RSHIFT to ROUNDED_RSHIFT, then updates all
> uses of the macro to use the new name. (These are all located
> in three codec files under libavcodec/.)
> 
> Signed-off-by: FeRD (Frank Dana) <ferdnyc@gmail.com>
> ---
>  libavcodec/mpeg4videodec.c |  4 ++--
>  libavcodec/vp3.c           | 16 ++++++++--------
>  libavcodec/vp56.c          |  4 ++--
>  libavutil/common.h         |  2 +-
>  4 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> index f44ee76bd4..5d63ba12ba 100644
> --- a/libavcodec/mpeg4videodec.c
> +++ b/libavcodec/mpeg4videodec.c
> @@ -601,7 +601,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int n)
>          if (ctx->divx_version == 500 && ctx->divx_build == 413 && a >= s->quarter_sample)
>              sum = s->sprite_offset[0][n] / (1 << (a - s->quarter_sample));
>          else
> -            sum = RSHIFT(s->sprite_offset[0][n] * (1 << s->quarter_sample), a);
> +            sum = ROUNDED_RSHIFT(s->sprite_offset[0][n] * (1 << s->quarter_sample), a);
>      } else {
>          dx    = s->sprite_delta[n][0];
>          dy    = s->sprite_delta[n][1];
> @@ -623,7 +623,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int n)
>                  v   += dx;
>              }
>          }
> -        sum = RSHIFT(sum, a + 8 - s->quarter_sample);
> +        sum = ROUNDED_RSHIFT(sum, a + 8 - s->quarter_sample);
>      }
>  
>      if (sum < -len)
> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
> index a5d8c2ed0b..13b3d6e22a 100644
> --- a/libavcodec/vp3.c
> +++ b/libavcodec/vp3.c
> @@ -861,10 +861,10 @@ static int unpack_vectors(Vp3DecodeContext *s, GetBitContext *gb)
>  
>                  if (s->chroma_y_shift) {
>                      if (s->macroblock_coding[current_macroblock] == MODE_INTER_FOURMV) {
> -                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1] +
> -                                             motion_x[2] + motion_x[3], 2);
> -                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1] +
> -                                             motion_y[2] + motion_y[3], 2);
> +                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] + motion_x[1] +
> +                                                     motion_x[2] + motion_x[3], 2);
> +                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] + motion_y[1] +
> +                                                     motion_y[2] + motion_y[3], 2);
>                      }
>                      motion_x[0] = (motion_x[0] >> 1) | (motion_x[0] & 1);
>                      motion_y[0] = (motion_y[0] >> 1) | (motion_y[0] & 1);
> @@ -873,10 +873,10 @@ static int unpack_vectors(Vp3DecodeContext *s, GetBitContext *gb)
>                      s->motion_val[1][frag][1] = motion_y[0];
>                  } else if (s->chroma_x_shift) {
>                      if (s->macroblock_coding[current_macroblock] == MODE_INTER_FOURMV) {
> -                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1], 1);
> -                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1], 1);
> -                        motion_x[1] = RSHIFT(motion_x[2] + motion_x[3], 1);
> -                        motion_y[1] = RSHIFT(motion_y[2] + motion_y[3], 1);
> +                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] + motion_x[1], 1);
> +                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] + motion_y[1], 1);
> +                        motion_x[1] = ROUNDED_RSHIFT(motion_x[2] + motion_x[3], 1);
> +                        motion_y[1] = ROUNDED_RSHIFT(motion_y[2] + motion_y[3], 1);
>                      } else {
>                          motion_x[1] = motion_x[0];
>                          motion_y[1] = motion_y[0];
> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
> index b69fe6c176..9359b48bc6 100644
> --- a/libavcodec/vp56.c
> +++ b/libavcodec/vp56.c
> @@ -197,8 +197,8 @@ static void vp56_decode_4mv(VP56Context *s, int row, int col)
>  
>      /* chroma vectors are average luma vectors */
>      if (s->avctx->codec->id == AV_CODEC_ID_VP5) {
> -        s->mv[4].x = s->mv[5].x = RSHIFT(mv.x,2);
> -        s->mv[4].y = s->mv[5].y = RSHIFT(mv.y,2);
> +        s->mv[4].x = s->mv[5].x = ROUNDED_RSHIFT(mv.x,2);
> +        s->mv[4].y = s->mv[5].y = ROUNDED_RSHIFT(mv.y,2);
>      } else {
>          s->mv[4] = s->mv[5] = (VP56mv) {mv.x/4, mv.y/4};
>      }
> diff --git a/libavutil/common.h b/libavutil/common.h
> index 8db0291170..0bff7f8f72 100644
> --- a/libavutil/common.h
> +++ b/libavutil/common.h
> @@ -51,7 +51,7 @@
>  #endif
>  
>  //rounded division & shift
> -#define RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) + ((1<<(b))>>1)-1)>>(b))
> +#define ROUNDED_RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) + ((1<<(b))>>1)-1)>>(b))

common.h is a public installed library, so this would be an API break.

There's also no good way to deprecate a define and replace it with
another while informing the library user, so for something purely
cosmetic like this i don't think it's worth the trouble.

>  /* assume b>0 */
>  #define ROUNDED_DIV(a,b) (((a)>0 ? (a) + ((b)>>1) : (a) - ((b)>>1))/(b))
>  /* Fast a/(1<<b) rounded toward +inf. Assume a>=0 and b>=0 */
>
FeRD (Frank Dana) Jan. 22, 2019, 2:32 a.m.
On Mon, Jan 21, 2019 at 3:54 PM James Almer <jamrial@gmail.com> wrote:

> On 1/21/2019 4:09 PM, FeRD (Frank Dana) wrote:
> > diff --git a/libavutil/common.h b/libavutil/common.h
> > index 8db0291170..0bff7f8f72 100644
> > --- a/libavutil/common.h
> > +++ b/libavutil/common.h
> > @@ -51,7 +51,7 @@
> >  #endif
> >
> >  //rounded division & shift
> > -#define RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) +
> ((1<<(b))>>1)-1)>>(b))
> > +#define ROUNDED_RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) :
> ((a) + ((1<<(b))>>1)-1)>>(b))
>
> common.h is a public installed library, so this would be an API break.
>
> There's also no good way to deprecate a define and replace it with
> another while informing the library user, so for something purely
> cosmetic like this i don't think it's worth the trouble.
>

Well... not *purely* cosmetic. See [1] for an example of one issue. Ruby's
`ruby/config.h` header
also defines an `RSHIFT` macro, with different semantics (it doesn't
round), so when building code
which includes both headers the macro ends up being redefined.

That being said, I can definitely accept "still not worth the trouble".
Thanks.

[1]: https://github.com/OpenShot/libopenshot/issues/164
Carl Eugen Hoyos Jan. 22, 2019, 10:58 a.m.
2019-01-21 21:47 GMT+01:00, James Almer <jamrial@gmail.com>:
> On 1/21/2019 4:09 PM, FeRD (Frank Dana) wrote:
>> The RSHIFT macro in libavutil/common.h does not actually perform
>> a bitwise right-shift, but rather a rounded version of the same
>> operation, as is noted by a comment above the macro. The rounded
>> divsion macro on the very next line is named ROUNDED_DIV, which
>> seems far more clear.
>>
>> This patch renames RSHIFT to ROUNDED_RSHIFT, then updates all
>> uses of the macro to use the new name. (These are all located
>> in three codec files under libavcodec/.)
>>
>> Signed-off-by: FeRD (Frank Dana) <ferdnyc@gmail.com>
>> ---
>>  libavcodec/mpeg4videodec.c |  4 ++--
>>  libavcodec/vp3.c           | 16 ++++++++--------
>>  libavcodec/vp56.c          |  4 ++--
>>  libavutil/common.h         |  2 +-
>>  4 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
>> index f44ee76bd4..5d63ba12ba 100644
>> --- a/libavcodec/mpeg4videodec.c
>> +++ b/libavcodec/mpeg4videodec.c
>> @@ -601,7 +601,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int n)
>>          if (ctx->divx_version == 500 && ctx->divx_build == 413 && a >=
>> s->quarter_sample)
>>              sum = s->sprite_offset[0][n] / (1 << (a -
>> s->quarter_sample));
>>          else
>> -            sum = RSHIFT(s->sprite_offset[0][n] * (1 <<
>> s->quarter_sample), a);
>> +            sum = ROUNDED_RSHIFT(s->sprite_offset[0][n] * (1 <<
>> s->quarter_sample), a);
>>      } else {
>>          dx    = s->sprite_delta[n][0];
>>          dy    = s->sprite_delta[n][1];
>> @@ -623,7 +623,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int n)
>>                  v   += dx;
>>              }
>>          }
>> -        sum = RSHIFT(sum, a + 8 - s->quarter_sample);
>> +        sum = ROUNDED_RSHIFT(sum, a + 8 - s->quarter_sample);
>>      }
>>
>>      if (sum < -len)
>> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
>> index a5d8c2ed0b..13b3d6e22a 100644
>> --- a/libavcodec/vp3.c
>> +++ b/libavcodec/vp3.c
>> @@ -861,10 +861,10 @@ static int unpack_vectors(Vp3DecodeContext *s,
>> GetBitContext *gb)
>>
>>                  if (s->chroma_y_shift) {
>>                      if (s->macroblock_coding[current_macroblock] ==
>> MODE_INTER_FOURMV) {
>> -                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1] +
>> -                                             motion_x[2] + motion_x[3],
>> 2);
>> -                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1] +
>> -                                             motion_y[2] + motion_y[3],
>> 2);
>> +                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] +
>> motion_x[1] +
>> +                                                     motion_x[2] +
>> motion_x[3], 2);
>> +                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] +
>> motion_y[1] +
>> +                                                     motion_y[2] +
>> motion_y[3], 2);
>>                      }
>>                      motion_x[0] = (motion_x[0] >> 1) | (motion_x[0] & 1);
>>                      motion_y[0] = (motion_y[0] >> 1) | (motion_y[0] & 1);
>> @@ -873,10 +873,10 @@ static int unpack_vectors(Vp3DecodeContext *s,
>> GetBitContext *gb)
>>                      s->motion_val[1][frag][1] = motion_y[0];
>>                  } else if (s->chroma_x_shift) {
>>                      if (s->macroblock_coding[current_macroblock] ==
>> MODE_INTER_FOURMV) {
>> -                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1],
>> 1);
>> -                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1],
>> 1);
>> -                        motion_x[1] = RSHIFT(motion_x[2] + motion_x[3],
>> 1);
>> -                        motion_y[1] = RSHIFT(motion_y[2] + motion_y[3],
>> 1);
>> +                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] +
>> motion_x[1], 1);
>> +                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] +
>> motion_y[1], 1);
>> +                        motion_x[1] = ROUNDED_RSHIFT(motion_x[2] +
>> motion_x[3], 1);
>> +                        motion_y[1] = ROUNDED_RSHIFT(motion_y[2] +
>> motion_y[3], 1);
>>                      } else {
>>                          motion_x[1] = motion_x[0];
>>                          motion_y[1] = motion_y[0];
>> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
>> index b69fe6c176..9359b48bc6 100644
>> --- a/libavcodec/vp56.c
>> +++ b/libavcodec/vp56.c
>> @@ -197,8 +197,8 @@ static void vp56_decode_4mv(VP56Context *s, int row,
>> int col)
>>
>>      /* chroma vectors are average luma vectors */
>>      if (s->avctx->codec->id == AV_CODEC_ID_VP5) {
>> -        s->mv[4].x = s->mv[5].x = RSHIFT(mv.x,2);
>> -        s->mv[4].y = s->mv[5].y = RSHIFT(mv.y,2);
>> +        s->mv[4].x = s->mv[5].x = ROUNDED_RSHIFT(mv.x,2);
>> +        s->mv[4].y = s->mv[5].y = ROUNDED_RSHIFT(mv.y,2);
>>      } else {
>>          s->mv[4] = s->mv[5] = (VP56mv) {mv.x/4, mv.y/4};
>>      }
>> diff --git a/libavutil/common.h b/libavutil/common.h
>> index 8db0291170..0bff7f8f72 100644
>> --- a/libavutil/common.h
>> +++ b/libavutil/common.h
>> @@ -51,7 +51,7 @@
>>  #endif
>>
>>  //rounded division & shift
>> -#define RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) +
>> ((1<<(b))>>1)-1)>>(b))
>> +#define ROUNDED_RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a)
>> + ((1<<(b))>>1)-1)>>(b))
>
> common.h is a public installed library, so this would be an API break.
>
> There's also no good way to deprecate a define and replace it with
> another while informing the library user, so for something purely
> cosmetic like this i don't think it's worth the trouble.

I wonder if we should change all macros that do not start with AV/FF
at the next version bump, this seems not unreasonable.

Carl Eugen
Hendrik Leppkes Jan. 22, 2019, 11:03 a.m.
On Tue, Jan 22, 2019 at 11:58 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2019-01-21 21:47 GMT+01:00, James Almer <jamrial@gmail.com>:
> > On 1/21/2019 4:09 PM, FeRD (Frank Dana) wrote:
> >> The RSHIFT macro in libavutil/common.h does not actually perform
> >> a bitwise right-shift, but rather a rounded version of the same
> >> operation, as is noted by a comment above the macro. The rounded
> >> divsion macro on the very next line is named ROUNDED_DIV, which
> >> seems far more clear.
> >>
> >> This patch renames RSHIFT to ROUNDED_RSHIFT, then updates all
> >> uses of the macro to use the new name. (These are all located
> >> in three codec files under libavcodec/.)
> >>
> >> Signed-off-by: FeRD (Frank Dana) <ferdnyc@gmail.com>
> >> ---
> >>  libavcodec/mpeg4videodec.c |  4 ++--
> >>  libavcodec/vp3.c           | 16 ++++++++--------
> >>  libavcodec/vp56.c          |  4 ++--
> >>  libavutil/common.h         |  2 +-
> >>  4 files changed, 13 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> >> index f44ee76bd4..5d63ba12ba 100644
> >> --- a/libavcodec/mpeg4videodec.c
> >> +++ b/libavcodec/mpeg4videodec.c
> >> @@ -601,7 +601,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int n)
> >>          if (ctx->divx_version == 500 && ctx->divx_build == 413 && a >=
> >> s->quarter_sample)
> >>              sum = s->sprite_offset[0][n] / (1 << (a -
> >> s->quarter_sample));
> >>          else
> >> -            sum = RSHIFT(s->sprite_offset[0][n] * (1 <<
> >> s->quarter_sample), a);
> >> +            sum = ROUNDED_RSHIFT(s->sprite_offset[0][n] * (1 <<
> >> s->quarter_sample), a);
> >>      } else {
> >>          dx    = s->sprite_delta[n][0];
> >>          dy    = s->sprite_delta[n][1];
> >> @@ -623,7 +623,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int n)
> >>                  v   += dx;
> >>              }
> >>          }
> >> -        sum = RSHIFT(sum, a + 8 - s->quarter_sample);
> >> +        sum = ROUNDED_RSHIFT(sum, a + 8 - s->quarter_sample);
> >>      }
> >>
> >>      if (sum < -len)
> >> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
> >> index a5d8c2ed0b..13b3d6e22a 100644
> >> --- a/libavcodec/vp3.c
> >> +++ b/libavcodec/vp3.c
> >> @@ -861,10 +861,10 @@ static int unpack_vectors(Vp3DecodeContext *s,
> >> GetBitContext *gb)
> >>
> >>                  if (s->chroma_y_shift) {
> >>                      if (s->macroblock_coding[current_macroblock] ==
> >> MODE_INTER_FOURMV) {
> >> -                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1] +
> >> -                                             motion_x[2] + motion_x[3],
> >> 2);
> >> -                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1] +
> >> -                                             motion_y[2] + motion_y[3],
> >> 2);
> >> +                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] +
> >> motion_x[1] +
> >> +                                                     motion_x[2] +
> >> motion_x[3], 2);
> >> +                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] +
> >> motion_y[1] +
> >> +                                                     motion_y[2] +
> >> motion_y[3], 2);
> >>                      }
> >>                      motion_x[0] = (motion_x[0] >> 1) | (motion_x[0] & 1);
> >>                      motion_y[0] = (motion_y[0] >> 1) | (motion_y[0] & 1);
> >> @@ -873,10 +873,10 @@ static int unpack_vectors(Vp3DecodeContext *s,
> >> GetBitContext *gb)
> >>                      s->motion_val[1][frag][1] = motion_y[0];
> >>                  } else if (s->chroma_x_shift) {
> >>                      if (s->macroblock_coding[current_macroblock] ==
> >> MODE_INTER_FOURMV) {
> >> -                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1],
> >> 1);
> >> -                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1],
> >> 1);
> >> -                        motion_x[1] = RSHIFT(motion_x[2] + motion_x[3],
> >> 1);
> >> -                        motion_y[1] = RSHIFT(motion_y[2] + motion_y[3],
> >> 1);
> >> +                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] +
> >> motion_x[1], 1);
> >> +                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] +
> >> motion_y[1], 1);
> >> +                        motion_x[1] = ROUNDED_RSHIFT(motion_x[2] +
> >> motion_x[3], 1);
> >> +                        motion_y[1] = ROUNDED_RSHIFT(motion_y[2] +
> >> motion_y[3], 1);
> >>                      } else {
> >>                          motion_x[1] = motion_x[0];
> >>                          motion_y[1] = motion_y[0];
> >> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
> >> index b69fe6c176..9359b48bc6 100644
> >> --- a/libavcodec/vp56.c
> >> +++ b/libavcodec/vp56.c
> >> @@ -197,8 +197,8 @@ static void vp56_decode_4mv(VP56Context *s, int row,
> >> int col)
> >>
> >>      /* chroma vectors are average luma vectors */
> >>      if (s->avctx->codec->id == AV_CODEC_ID_VP5) {
> >> -        s->mv[4].x = s->mv[5].x = RSHIFT(mv.x,2);
> >> -        s->mv[4].y = s->mv[5].y = RSHIFT(mv.y,2);
> >> +        s->mv[4].x = s->mv[5].x = ROUNDED_RSHIFT(mv.x,2);
> >> +        s->mv[4].y = s->mv[5].y = ROUNDED_RSHIFT(mv.y,2);
> >>      } else {
> >>          s->mv[4] = s->mv[5] = (VP56mv) {mv.x/4, mv.y/4};
> >>      }
> >> diff --git a/libavutil/common.h b/libavutil/common.h
> >> index 8db0291170..0bff7f8f72 100644
> >> --- a/libavutil/common.h
> >> +++ b/libavutil/common.h
> >> @@ -51,7 +51,7 @@
> >>  #endif
> >>
> >>  //rounded division & shift
> >> -#define RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) +
> >> ((1<<(b))>>1)-1)>>(b))
> >> +#define ROUNDED_RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a)
> >> + ((1<<(b))>>1)-1)>>(b))
> >
> > common.h is a public installed library, so this would be an API break.
> >
> > There's also no good way to deprecate a define and replace it with
> > another while informing the library user, so for something purely
> > cosmetic like this i don't think it's worth the trouble.
>
> I wonder if we should change all macros that do not start with AV/FF
> at the next version bump, this seems not unreasonable.
>

API changes should have proper deprecation markers and deprecation
periods. We don't tend to break user-code without warning and proper
run-up time.
As James already mentioned, properly deprecating macros is not
something compilers offer tools for, so thats a big headache.

- Hendrik
Carl Eugen Hoyos Jan. 22, 2019, 11:15 a.m.
2019-01-22 12:03 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> On Tue, Jan 22, 2019 at 11:58 AM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>>
>> 2019-01-21 21:47 GMT+01:00, James Almer <jamrial@gmail.com>:
>> > On 1/21/2019 4:09 PM, FeRD (Frank Dana) wrote:
>> >> The RSHIFT macro in libavutil/common.h does not actually perform
>> >> a bitwise right-shift, but rather a rounded version of the same
>> >> operation, as is noted by a comment above the macro. The rounded
>> >> divsion macro on the very next line is named ROUNDED_DIV, which
>> >> seems far more clear.
>> >>
>> >> This patch renames RSHIFT to ROUNDED_RSHIFT, then updates all
>> >> uses of the macro to use the new name. (These are all located
>> >> in three codec files under libavcodec/.)
>> >>
>> >> Signed-off-by: FeRD (Frank Dana) <ferdnyc@gmail.com>
>> >> ---
>> >>  libavcodec/mpeg4videodec.c |  4 ++--
>> >>  libavcodec/vp3.c           | 16 ++++++++--------
>> >>  libavcodec/vp56.c          |  4 ++--
>> >>  libavutil/common.h         |  2 +-
>> >>  4 files changed, 13 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
>> >> index f44ee76bd4..5d63ba12ba 100644
>> >> --- a/libavcodec/mpeg4videodec.c
>> >> +++ b/libavcodec/mpeg4videodec.c
>> >> @@ -601,7 +601,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int
>> >> n)
>> >>          if (ctx->divx_version == 500 && ctx->divx_build == 413 && a >=
>> >> s->quarter_sample)
>> >>              sum = s->sprite_offset[0][n] / (1 << (a -
>> >> s->quarter_sample));
>> >>          else
>> >> -            sum = RSHIFT(s->sprite_offset[0][n] * (1 <<
>> >> s->quarter_sample), a);
>> >> +            sum = ROUNDED_RSHIFT(s->sprite_offset[0][n] * (1 <<
>> >> s->quarter_sample), a);
>> >>      } else {
>> >>          dx    = s->sprite_delta[n][0];
>> >>          dy    = s->sprite_delta[n][1];
>> >> @@ -623,7 +623,7 @@ static inline int get_amv(Mpeg4DecContext *ctx, int
>> >> n)
>> >>                  v   += dx;
>> >>              }
>> >>          }
>> >> -        sum = RSHIFT(sum, a + 8 - s->quarter_sample);
>> >> +        sum = ROUNDED_RSHIFT(sum, a + 8 - s->quarter_sample);
>> >>      }
>> >>
>> >>      if (sum < -len)
>> >> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
>> >> index a5d8c2ed0b..13b3d6e22a 100644
>> >> --- a/libavcodec/vp3.c
>> >> +++ b/libavcodec/vp3.c
>> >> @@ -861,10 +861,10 @@ static int unpack_vectors(Vp3DecodeContext *s,
>> >> GetBitContext *gb)
>> >>
>> >>                  if (s->chroma_y_shift) {
>> >>                      if (s->macroblock_coding[current_macroblock] ==
>> >> MODE_INTER_FOURMV) {
>> >> -                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1]
>> >> +
>> >> -                                             motion_x[2] +
>> >> motion_x[3],
>> >> 2);
>> >> -                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1]
>> >> +
>> >> -                                             motion_y[2] +
>> >> motion_y[3],
>> >> 2);
>> >> +                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] +
>> >> motion_x[1] +
>> >> +                                                     motion_x[2] +
>> >> motion_x[3], 2);
>> >> +                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] +
>> >> motion_y[1] +
>> >> +                                                     motion_y[2] +
>> >> motion_y[3], 2);
>> >>                      }
>> >>                      motion_x[0] = (motion_x[0] >> 1) | (motion_x[0] &
>> >> 1);
>> >>                      motion_y[0] = (motion_y[0] >> 1) | (motion_y[0] &
>> >> 1);
>> >> @@ -873,10 +873,10 @@ static int unpack_vectors(Vp3DecodeContext *s,
>> >> GetBitContext *gb)
>> >>                      s->motion_val[1][frag][1] = motion_y[0];
>> >>                  } else if (s->chroma_x_shift) {
>> >>                      if (s->macroblock_coding[current_macroblock] ==
>> >> MODE_INTER_FOURMV) {
>> >> -                        motion_x[0] = RSHIFT(motion_x[0] +
>> >> motion_x[1],
>> >> 1);
>> >> -                        motion_y[0] = RSHIFT(motion_y[0] +
>> >> motion_y[1],
>> >> 1);
>> >> -                        motion_x[1] = RSHIFT(motion_x[2] +
>> >> motion_x[3],
>> >> 1);
>> >> -                        motion_y[1] = RSHIFT(motion_y[2] +
>> >> motion_y[3],
>> >> 1);
>> >> +                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] +
>> >> motion_x[1], 1);
>> >> +                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] +
>> >> motion_y[1], 1);
>> >> +                        motion_x[1] = ROUNDED_RSHIFT(motion_x[2] +
>> >> motion_x[3], 1);
>> >> +                        motion_y[1] = ROUNDED_RSHIFT(motion_y[2] +
>> >> motion_y[3], 1);
>> >>                      } else {
>> >>                          motion_x[1] = motion_x[0];
>> >>                          motion_y[1] = motion_y[0];
>> >> diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
>> >> index b69fe6c176..9359b48bc6 100644
>> >> --- a/libavcodec/vp56.c
>> >> +++ b/libavcodec/vp56.c
>> >> @@ -197,8 +197,8 @@ static void vp56_decode_4mv(VP56Context *s, int
>> >> row,
>> >> int col)
>> >>
>> >>      /* chroma vectors are average luma vectors */
>> >>      if (s->avctx->codec->id == AV_CODEC_ID_VP5) {
>> >> -        s->mv[4].x = s->mv[5].x = RSHIFT(mv.x,2);
>> >> -        s->mv[4].y = s->mv[5].y = RSHIFT(mv.y,2);
>> >> +        s->mv[4].x = s->mv[5].x = ROUNDED_RSHIFT(mv.x,2);
>> >> +        s->mv[4].y = s->mv[5].y = ROUNDED_RSHIFT(mv.y,2);
>> >>      } else {
>> >>          s->mv[4] = s->mv[5] = (VP56mv) {mv.x/4, mv.y/4};
>> >>      }
>> >> diff --git a/libavutil/common.h b/libavutil/common.h
>> >> index 8db0291170..0bff7f8f72 100644
>> >> --- a/libavutil/common.h
>> >> +++ b/libavutil/common.h
>> >> @@ -51,7 +51,7 @@
>> >>  #endif
>> >>
>> >>  //rounded division & shift
>> >> -#define RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) +
>> >> ((1<<(b))>>1)-1)>>(b))
>> >> +#define ROUNDED_RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) :
>> >> ((a)
>> >> + ((1<<(b))>>1)-1)>>(b))
>> >
>> > common.h is a public installed library, so this would be an API break.
>> >
>> > There's also no good way to deprecate a define and replace it with
>> > another while informing the library user, so for something purely
>> > cosmetic like this i don't think it's worth the trouble.
>>
>> I wonder if we should change all macros that do not start with AV/FF
>> at the next version bump, this seems not unreasonable.
>>
>
> API changes should have proper deprecation markers and deprecation
> periods.

Yes, they "should" but that "shouldn't" mean that some broken things
can never be changed.

> We don't tend to break user-code without warning and proper
> run-up time.

That's honestly news to me, sorry, the number of open regressions
was never bigger than currently and we regularly change things that
users find very unexpected.

The run-up time would be sufficient, btw.

> As James already mentioned, properly deprecating macros is not
> something compilers offer tools for, so thats a big headache.

So you are arguing we simply cannot change macros?
That sounds strange to me.

Carl Eugen
Moritz Barsnick Jan. 22, 2019, 1:15 p.m.
On Mon, Jan 21, 2019 at 21:32:46 -0500, FeRD wrote:
> Well... not *purely* cosmetic. See [1] for an example of one issue. Ruby's

> `ruby/config.h` header also defines an `RSHIFT` macro, with different
> semantics (it doesn't round), so when building code which includes
> both headers the macro ends up being redefined.
[...]
> [1]: https://github.com/OpenShot/libopenshot/issues/164

While I agree with the assessment that ffmpeg's macro should have been
named "FF_RSHIFT", "AV_RSHIFT" or even something more appropriate along
your suggestion, you failed to post an upstream patch with ruby to
rename their macro to "RUBY_RSHIFT". ;-) Honestly, these arbitrary
names (sometimes also public function names) really mess up inclusions.
(E.g. mutt moved from M_MACRONAME to MUTT_MACRONAME recently, for the
same reasons.)

Moritz
FeRD (Frank Dana) Jan. 26, 2019, 7:46 p.m.
On Tue, Jan 22, 2019 at 8:15 AM Moritz Barsnick <barsnick@gmx.net> wrote:

> On Mon, Jan 21, 2019 at 21:32:46 -0500, FeRD wrote:
> > Well... not *purely* cosmetic. See [1] for an example of one issue.
> Ruby's
>
> > `ruby/config.h` header also defines an `RSHIFT` macro, with different
> > semantics (it doesn't round), so when building code which includes
> > both headers the macro ends up being redefined.
> [...]
> > [1]: https://github.com/OpenShot/libopenshot/issues/164
>
> While I agree with the assessment that ffmpeg's macro should have been
> named "FF_RSHIFT", "AV_RSHIFT" or even something more appropriate along
> your suggestion, you failed to post an upstream patch with ruby to
> rename their macro to "RUBY_RSHIFT". ;-)


Fair, but that could be taken as a compliment! Changing it in either source
tree is sufficient to solve the conflict, so perhaps the FFMpeg project
seemed like the venue that would produce less friction. ;-)

No, honestly, the main reason is that... well, Ruby's RSHIFT defines a
classic bitwise right-shift, whereas FFMpeg's rounds. So, in my very
opinionated opinion, Ruby's RSHIFT is at least CORRECT, whereas FFMpeg's is
just plain wrong. (Wrong to be called "RSHIFT", when it's really
ROUNDED_RSHIFT.) So if there's going to be any macro defining RSHIFT, I'd
prefer it at least be one that fits the name.

Should Ruby's be named RUBY_RSHIFT? Sure, it'd certainly make life easier.
Should FFMpeg's be named AV_ROUNDED_RSHIFT? Also sure. But (again in my
opinionated opinion) not AV_RSHIFT, because that's still *not* what it
does! (And especially since the FFMpeg codebase already has AV_CEIL_RSHIFT
which always rounds upwards, setting a pattern/precedent for the naming.)
Peter Ross Jan. 26, 2019, 11:07 p.m.
On Sat, Jan 26, 2019 at 02:46:59PM -0500, FeRD wrote:
> On Tue, Jan 22, 2019 at 8:15 AM Moritz Barsnick <barsnick@gmx.net> wrote:
> 
> > On Mon, Jan 21, 2019 at 21:32:46 -0500, FeRD wrote:
> > > Well... not *purely* cosmetic. See [1] for an example of one issue.
> > Ruby's
> >
> > > `ruby/config.h` header also defines an `RSHIFT` macro, with different
> > > semantics (it doesn't round), so when building code which includes
> > > both headers the macro ends up being redefined.
> > [...]
> > > [1]: https://github.com/OpenShot/libopenshot/issues/164
> >
> > While I agree with the assessment that ffmpeg's macro should have been
> > named "FF_RSHIFT", "AV_RSHIFT" or even something more appropriate along
> > your suggestion, you failed to post an upstream patch with ruby to
> > rename their macro to "RUBY_RSHIFT". ;-)
> 
> 
> Fair, but that could be taken as a compliment! Changing it in either source
> tree is sufficient to solve the conflict, so perhaps the FFMpeg project
> seemed like the venue that would produce less friction. ;-)
> 
> No, honestly, the main reason is that... well, Ruby's RSHIFT defines a
> classic bitwise right-shift, whereas FFMpeg's rounds. So, in my very
> opinionated opinion, Ruby's RSHIFT is at least CORRECT, whereas FFMpeg's is
> just plain wrong. (Wrong to be called "RSHIFT", when it's really
> ROUNDED_RSHIFT.) So if there's going to be any macro defining RSHIFT, I'd
> prefer it at least be one that fits the name.
> 
> Should Ruby's be named RUBY_RSHIFT? Sure, it'd certainly make life easier.
> Should FFMpeg's be named AV_ROUNDED_RSHIFT? Also sure. But (again in my
> opinionated opinion) not AV_RSHIFT, because that's still *not* what it
> does! (And especially since the FFMpeg codebase already has AV_CEIL_RSHIFT
> which always rounds upwards, setting a pattern/precedent for the naming.)

only recently came into contact with this macro, as its used in vp56. the
name confused me initially me, and had to grep for it, and then read then
expansion.

i support renaming it to something more clear (ROUNDED_RSHIFT) and adding the
FF_ or AV_ namespace prefix.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Henrik Gramner Jan. 27, 2019, 11:04 p.m.
On Mon, Jan 21, 2019 at 9:54 PM James Almer <jamrial@gmail.com> wrote:
> There's also no good way to deprecate a define and replace it with
> another while informing the library user, so for something purely
> cosmetic like this i don't think it's worth the trouble.

Would it be possible to create a deprecated inlined function that does
nothing, and add a call to that function inside the old macro? Kind of
ugly though.
Uoti Urpala Jan. 28, 2019, 12:28 a.m.
On Mon, 2019-01-28 at 00:04 +0100, Henrik Gramner wrote:
> On Mon, Jan 21, 2019 at 9:54 PM James Almer <jamrial@gmail.com> wrote:
> > There's also no good way to deprecate a define and replace it with
> > another while informing the library user, so for something purely
> > cosmetic like this i don't think it's worth the trouble.
> 
> Would it be possible to create a deprecated inlined function that does
> nothing, and add a call to that function inside the old macro? Kind of
> ugly though.

I already posted that suggestion last Tuesday. It does work trigger a
deprecation warning, but an inline function has the drawback that it's
not a constant expression. If you want to keep backward compatibility
even for uses like initializing global tables or other variables, you
can use a deprecated variable in ways other than a call though. For
example this should be a constant expression that shows a deprecation
message:

// This variable does not need to really exist
extern int __attribute__ ((deprecated)) RSHIFT_is_deprecated;

#define RSHIFT(a, b) (0*(int)sizeof(RSHIFT_is_deprecated) + AV_ROUNDED_SHIFT(a, b))

sizeof counts as a "use" that shows the deprecation warning, while not
creating any actual reference to the variable and being a constant
expression. The (int) cast is there to make 100% sure that the addition
doesn't change anything by changing the type of the expression.

Patch hide | download patch | download mbox

diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
index f44ee76bd4..5d63ba12ba 100644
--- a/libavcodec/mpeg4videodec.c
+++ b/libavcodec/mpeg4videodec.c
@@ -601,7 +601,7 @@  static inline int get_amv(Mpeg4DecContext *ctx, int n)
         if (ctx->divx_version == 500 && ctx->divx_build == 413 && a >= s->quarter_sample)
             sum = s->sprite_offset[0][n] / (1 << (a - s->quarter_sample));
         else
-            sum = RSHIFT(s->sprite_offset[0][n] * (1 << s->quarter_sample), a);
+            sum = ROUNDED_RSHIFT(s->sprite_offset[0][n] * (1 << s->quarter_sample), a);
     } else {
         dx    = s->sprite_delta[n][0];
         dy    = s->sprite_delta[n][1];
@@ -623,7 +623,7 @@  static inline int get_amv(Mpeg4DecContext *ctx, int n)
                 v   += dx;
             }
         }
-        sum = RSHIFT(sum, a + 8 - s->quarter_sample);
+        sum = ROUNDED_RSHIFT(sum, a + 8 - s->quarter_sample);
     }
 
     if (sum < -len)
diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
index a5d8c2ed0b..13b3d6e22a 100644
--- a/libavcodec/vp3.c
+++ b/libavcodec/vp3.c
@@ -861,10 +861,10 @@  static int unpack_vectors(Vp3DecodeContext *s, GetBitContext *gb)
 
                 if (s->chroma_y_shift) {
                     if (s->macroblock_coding[current_macroblock] == MODE_INTER_FOURMV) {
-                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1] +
-                                             motion_x[2] + motion_x[3], 2);
-                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1] +
-                                             motion_y[2] + motion_y[3], 2);
+                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] + motion_x[1] +
+                                                     motion_x[2] + motion_x[3], 2);
+                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] + motion_y[1] +
+                                                     motion_y[2] + motion_y[3], 2);
                     }
                     motion_x[0] = (motion_x[0] >> 1) | (motion_x[0] & 1);
                     motion_y[0] = (motion_y[0] >> 1) | (motion_y[0] & 1);
@@ -873,10 +873,10 @@  static int unpack_vectors(Vp3DecodeContext *s, GetBitContext *gb)
                     s->motion_val[1][frag][1] = motion_y[0];
                 } else if (s->chroma_x_shift) {
                     if (s->macroblock_coding[current_macroblock] == MODE_INTER_FOURMV) {
-                        motion_x[0] = RSHIFT(motion_x[0] + motion_x[1], 1);
-                        motion_y[0] = RSHIFT(motion_y[0] + motion_y[1], 1);
-                        motion_x[1] = RSHIFT(motion_x[2] + motion_x[3], 1);
-                        motion_y[1] = RSHIFT(motion_y[2] + motion_y[3], 1);
+                        motion_x[0] = ROUNDED_RSHIFT(motion_x[0] + motion_x[1], 1);
+                        motion_y[0] = ROUNDED_RSHIFT(motion_y[0] + motion_y[1], 1);
+                        motion_x[1] = ROUNDED_RSHIFT(motion_x[2] + motion_x[3], 1);
+                        motion_y[1] = ROUNDED_RSHIFT(motion_y[2] + motion_y[3], 1);
                     } else {
                         motion_x[1] = motion_x[0];
                         motion_y[1] = motion_y[0];
diff --git a/libavcodec/vp56.c b/libavcodec/vp56.c
index b69fe6c176..9359b48bc6 100644
--- a/libavcodec/vp56.c
+++ b/libavcodec/vp56.c
@@ -197,8 +197,8 @@  static void vp56_decode_4mv(VP56Context *s, int row, int col)
 
     /* chroma vectors are average luma vectors */
     if (s->avctx->codec->id == AV_CODEC_ID_VP5) {
-        s->mv[4].x = s->mv[5].x = RSHIFT(mv.x,2);
-        s->mv[4].y = s->mv[5].y = RSHIFT(mv.y,2);
+        s->mv[4].x = s->mv[5].x = ROUNDED_RSHIFT(mv.x,2);
+        s->mv[4].y = s->mv[5].y = ROUNDED_RSHIFT(mv.y,2);
     } else {
         s->mv[4] = s->mv[5] = (VP56mv) {mv.x/4, mv.y/4};
     }
diff --git a/libavutil/common.h b/libavutil/common.h
index 8db0291170..0bff7f8f72 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -51,7 +51,7 @@ 
 #endif
 
 //rounded division & shift
-#define RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) + ((1<<(b))>>1)-1)>>(b))
+#define ROUNDED_RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) + ((1<<(b))>>1)-1)>>(b))
 /* assume b>0 */
 #define ROUNDED_DIV(a,b) (((a)>0 ? (a) + ((b)>>1) : (a) - ((b)>>1))/(b))
 /* Fast a/(1<<b) rounded toward +inf. Assume a>=0 and b>=0 */