RSHIFT redefinition while building ruby

[ferdnyc]

In addition to the tests build issue I corrected with #163, this warning while building the ruby bindings concerns me a bit; seems Ruby and FFmpeg have competing definitions for the RSHIFT macro:

cd /home/ferd/rpmbuild/REPOS/libopenshot/build/src/bindings/ruby && /usr/lib64/ccache/c++  -DMAGICKCORE_HDRI_ENABLE=0 -DMAGICKCORE_QUANTUM_DEPTH=16 -DOPENSHOT_IMAGEMAGICK_COMPATIBILITY=0 -DQT_CORE_LIB -DQT_GUI_LIB -DQT_MULTIMEDIAWIDGETS_LIB -DQT_MULTIMEDIA_LIB -DQT_NETWORK_LIB -DQT_NO_DEBUG -DQT_WIDGETS_LIB -DUSE_IMAGEMAGICK=1 -Drbopenshot_EXPORTS -I/usr/include/ImageMagick-6 -I/usr/include/ffmpeg -I/usr/include/libopenshot-audio -isystem /usr/include/qt5 -isystem /usr/include/qt5/QtWidgets -isystem /usr/include/qt5/QtGui -isystem /usr/include/qt5/QtCore -isystem /usr/lib64/qt5/mkspecs/linux-g++ -isystem /usr/include/qt5/QtMultimedia -isystem /usr/include/qt5/QtNetwork -isystem /usr/include/qt5/QtMultimediaWidgets -I/home/ferd/rpmbuild/REPOS/libopenshot/src/../thirdparty/jsoncpp/include -I/home/ferd/rpmbuild/REPOS/libopenshot/src/bindings/ruby  -std=c++11           -g -ggdb  -fopenmp  -fPIC   -fPIC -std=gnu++11 -o CMakeFiles/rbopenshot.dir/openshotRUBY_wrap.cxx.o -c /home/ferd/rpmbuild/REPOS/libopenshot/build/src/bindings/ruby/openshotRUBY_wrap.cxx
In file included from /usr/include/ffmpeg/libavutil/avutil.h:296,
                 from /usr/include/ffmpeg/libavutil/samplefmt.h:24,
                 from /usr/include/ffmpeg/libavcodec/avcodec.h:31,
                 from /home/ferd/rpmbuild/REPOS/libopenshot/src/../thirdparty/jsoncpp/include/../../../include/FFmpegUtilities.h:43,
                 from /home/ferd/rpmbuild/REPOS/libopenshot/src/../thirdparty/jsoncpp/include/../../../include/ChannelLayouts.h:32,
                 from /home/ferd/rpmbuild/REPOS/libopenshot/src/../thirdparty/jsoncpp/include/../../../include/Frame.h:60,
                 from /home/ferd/rpmbuild/REPOS/libopenshot/src/../thirdparty/jsoncpp/include/../../../include/CacheBase.h:32,
                 from /home/ferd/rpmbuild/REPOS/libopenshot/src/../thirdparty/jsoncpp/include/../../../include/CacheMemory.h:34,
                 from /home/ferd/rpmbuild/REPOS/libopenshot/src/../thirdparty/jsoncpp/include/../../../include/ReaderBase.h:36,
                 from /home/ferd/rpmbuild/REPOS/libopenshot/build/src/bindings/ruby/openshotRUBY_wrap.cxx:3014:
/usr/include/ffmpeg/libavutil/common.h:54: warning: "RSHIFT" redefined
 #define RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) + ((1<<(b))>>1)-1)>>(b))

In file included from /usr/include/ruby/config.h:18,
                 from /usr/include/ruby/ruby.h:24,
                 from /usr/include/ruby.h:33,
                 from /home/ferd/rpmbuild/REPOS/libopenshot/build/src/bindings/ruby/openshotRUBY_wrap.cxx:879:
/usr/include/ruby/config-x86_64.h:367: note: this is the location of the previous definition
 #define RSHIFT(x,y) ((x)>>(int)(y))

Worse, the definitions are NOT equivalent.

• FFMpeg's seems to be rounding. (Ah, in fact, a comment in the header file indicates that RSHIFT and ROUNDED_DIV are //rounded division & shift — nice of them to use "RSHIFT" rather than "ROUNDED_SHIFT".)
• Ruby's, OTOH, is clearly just a macro to perform a simple bitwise right-shift.

A simple test program (expand for code)

// RSHIFT test program
#include <iostream>
#include <bitset>

#define RB_RSHIFT(x,y) ((x)>>(int)(y))
#define FF_RSHIFT(a,b) ((a) > 0 ? ((a) + ((1<<(b))>>1))>>(b) : ((a) + ((1<<(b))>>1)-1)>>(b))

int main()
{
  using namespace std;
  int num = 17;
  int dist = 10;
  int shift = 2;

  for (int i = num; i<(num+dist); i++) {  
    int ffout = FF_RSHIFT(i,shift);
    int rbout = RB_RSHIFT(i,shift);
    cout << "RSHIFT (" << bitset<8>(i) << "," << shift << "): ";
    cout << " FFmpeg: " << bitset<8>(ffout);
    cout << " Ruby: " << bitset<8>(rbout) << "\n";
  }  
}

Generates this output:

RSHIFT (00010001,2):  FFmpeg: 00000100 Ruby: 00000100
RSHIFT (00010010,2):  FFmpeg: 00000101 Ruby: 00000100
RSHIFT (00010011,2):  FFmpeg: 00000101 Ruby: 00000100
RSHIFT (00010100,2):  FFmpeg: 00000101 Ruby: 00000101
RSHIFT (00010101,2):  FFmpeg: 00000101 Ruby: 00000101
RSHIFT (00010110,2):  FFmpeg: 00000110 Ruby: 00000101
RSHIFT (00010111,2):  FFmpeg: 00000110 Ruby: 00000101
RSHIFT (00011000,2):  FFmpeg: 00000110 Ruby: 00000110
RSHIFT (00011001,2):  FFmpeg: 00000110 Ruby: 00000110
RSHIFT (00011010,2):  FFmpeg: 00000111 Ruby: 00000110

I don't know whether RSHIFT ever actually gets used anywhere in the ruby bindings, if it doesn't then it may not matter. Not sure of the best way to handle the collision, either (namespaces? It is C++...). But since I don't use the ruby bindings anyway (does anyone/anything?), and it's only a warning... "meh".

[ferdnyc]

I've submitted patches to ffmpeg to rename their non-rshift "RSHIFT" to ROUNDED_RSHIFT, which if accepted would let us dodge this issue (when building against ffmpeg 4.2+, or whatever release the patches appeared in...).

[ferdnyc]

They were not accepted on the grounds that they break the FFMpeg API, which is fair — and the developers are not unsympathetic to our plight. Still, since it'll take a couple of years for this to be dealt with in the FFMpeg codebase, even assuming they settle on a plan for doing it, we're kind of on our own for the moment.