| Message ID | 20190121190936.24902-1-ferdnyc@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 */ >
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
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
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
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
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
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.)
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)
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.
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.
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 */
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(-)