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

2019-01-26 Thread FeRD 
On Tue, Jan 22, 2019 at 8:15 AM Moritz Barsnick  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.)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2019-01-21 Thread FeRD 
On Mon, Jan 21, 2019 at 3:54 PM James Almer  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2019-01-21 Thread FeRD (Frank Dana) 
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) 
---
 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<<(

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

2019-01-21 Thread FeRD 
On Mon, Jan 21, 2019 at 1:55 PM Moritz Barsnick  wrote:

> On Mon, Jan 21, 2019 at 12:38:58 -0500, FeRD (Frank Dana) wrote:
>
> > After applying both patches, 'make fate' succeeds and ffmpeg is still
> > functional.
>
> You're not allowed to break fate (or compilation). So the two pathes
> need to be merged.


Aha, thanks. I'll resubmit squashed into a single patch.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec/tests: Add codec_desc to .gitignore

2019-01-21 Thread FeRD (Frank Dana) 
The compiled libavcodec/tests/codec_desc was left out of that dir's
.gitignore when the test was added, so it shows up in 'git status'
as an untracked file if it's been built.

Signed-off-by: FeRD (Frank Dana) 
---
 libavcodec/tests/.gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/tests/.gitignore b/libavcodec/tests/.gitignore
index 73945a7c82..56ddb2cbeb 100644
--- a/libavcodec/tests/.gitignore
+++ b/libavcodec/tests/.gitignore
@@ -2,6 +2,7 @@
 /avpacket
 /cabac
 /celp_math
+/codec_desc
 /dct
 /fft
 /fft-fixed
-- 
2.20.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] avutil: Rename RSHIFT macro to ROUNDED_RSHIFT

2019-01-21 Thread FeRD (Frank Dana) 
libavutil/common.h contains a pair of macros on lines 54-55, which
the comment on line 53 describes as "rounded division & shift".

The division macro is named ROUNDED_DIV. The shift macro is named
RSHIFT. Since "RSHIFT" is a common name for a bitwise right-shift
operation without rounding (see e.g. FORTRAN, IBM XL C/C++), this
seems needlessly confusing.

This change renames RSHIFT to ROUNDED_RSHIFT, matching the naming
style of the ROUNDED_DIV macro.

Signed-off-by: FeRD (Frank Dana) 
---
 libavutil/common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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<=0 and b>=0 */
-- 
2.20.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] avcodec: Change uses of RSHIFT to ROUNDED_RSHIFT

2019-01-21 Thread FeRD (Frank Dana) 
Three files in libavcodec/ use the RSHIFT macro from libavutil:
- mpeg4videodec.c
- vp3.c
- vp56.c

All instances of RSHIFT are updated to follow the name-change
in libavutil/common.h (previous commit).

Signed-off-by: FeRD (Frank Dana) 
---
 libavcodec/mpeg4videodec.c |  4 ++--
 libavcodec/vp3.c   | 16 
 libavcodec/vp56.c  |  4 ++--
 3 files changed, 12 insertions(+), 12 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};
 }
-- 
2.20.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] Rename RSHIFT macro to ROUNDED_RSHIFT

2019-01-21 Thread FeRD (Frank Dana) 
Patches to follow:

[PATCH 1/2] avutil: Rename RSHIFT macro to ROUNDED_RSHIFT
[PATCH 2/2] avcodec: Change uses of RSHIFT to ROUNDED_RSHIFT

This is my first patch submission to ffmpeg (and at a tense
moment, it seems, but soldiering on...), please bear with me.

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.

So, the first of these two patches renames RSHIFT to ROUNDED_RSHIFT
for clarity. The second updates all uses of the macro which are
internal to the ffmpeg source tree (which occur in only three
codecs under libavcodec/). After applying both patches, 'make fate'
succeeds and ffmpeg is still functional.

An example of the name causing issues (due to a conflict with
the RSHIFT macro in the Ruby source, which does perform a
standard bitwise right-shift) can be found at [1].

[1]: https://github.com/OpenShot/libopenshot/issues/164

Signed-off-by: FeRD (Frank Dana) 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel