[FFmpeg-devel] [PATCH] avutil: Rename RSHIFT macro to ROUNDED_RSHIFT
Carl Eugen Hoyos
ceffmpeg at gmail.com
Tue Jan 22 13:15:00 EET 2019
2019-01-22 12:03 GMT+01:00, Hendrik Leppkes <h.leppkes at gmail.com>:
> On Tue, Jan 22, 2019 at 11:58 AM Carl Eugen Hoyos <ceffmpeg at gmail.com>
> wrote:
>>
>> 2019-01-21 21:47 GMT+01:00, James Almer <jamrial at 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 at 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
More information about the ffmpeg-devel
mailing list