fix some custom build issues with ffmpeg

[markreidvfx]

Linked issues

n/a

Summarize your change.

I had some difficulties building ffmpeg with custom configuration options and this is what resolved it for me. RV_FFMPEG_CONFIG_OPTIONS not being converted to a list was the issue. This can happen when you pass it as an option via cmake commandline like this.

cmake -DRV_FFMPEG_CONFIG_OPTIONS="--enable-decoder=vp9 --enable-decoder=vp8" ..

There could still be issue doing it this way. When passing an option via a commandline cmake does confusing things with escaping quotes that I don't entirely understand. If for example if you want to add a custom build option like
--extra-cflags="-I/include/dir1 -I/incude/dir2" I still can't figure out how to get cmake to escape the quotes properly.

The other issues was ffmpeg not finding the install packages because the script wasn't using the PKG_CONFIG_PATH env var.

Describe the reason for the change.

Describe what you have tested and on which operating system.

I've only tested this on centos 7

Add a list of changes, and note any that might need special attention during the review.

If possible, provide screenshots.

[bernie-laberge]

Hello Mark, thanks for the fix !
We apologize for the delay in approving your PR but your fix breaks our internal CI Windows build.
We need to figure out what is causing this Windows build break with your PR (although there seems to be nothing wrong with your PR).

[markreidvfx]

It has been I while since I've attempted to build the windows version. I gave it a go, I've gotten it to build (though the exe always segfaults).

This being in quotes was a issue when building ffmpeg.

OpenRV/cmake/dependencies/ffmpeg.cmake

Line 232 in aef2b06

"${_disabled_decoders} ${_disabled_encoders} ${_disabled_filters} ${_disabled_parsers} ${_disabled_protocols}"

cmake combines some of the args together with quotes. This messes up the configure script.

I also haven't been able to get ffmpeg to build yet with multiple PKG_CONFIG_PATH paths. The colon or something is causing issues.

[meepzh]

I had an unexpected need to build RV for Windows and a ton of time to kill, so I tried addressing the notes in my fork of Mark's fork here: https://github.com/meepzh/OpenRV/tree/al_ffmpeg_config_v1

RV_FFMPEG_CONFIG_OPTIONS Quotations

Mark, your changes were really helpful and got me looking deeper into how CMake handles lists. To confirm, with the quotation marks retained, it seems that CMake for Visual Studio was kindly recording the spaces between lists under C:\OpenRV\_build\cmake\dependencies\RV_DEPS_FFMPEG.vcxproj:

...\cmake.exe -E env ... sh ./configure ... --disable-decoder=aac_latm "--disable-decoder=dvvideo --disable-encoder=dnxhd" --disable-encoder=prores ...

leading to this error:

./configure: eval: line 582: syntax error near unexpected token `--disable-encoder=dnxhd_decoder'

which your solution solves by removing the quotation marks, resolving #67 (comment).

RV_FFMPEG_CONFIG_OPTIONS Args

For lists in CMake, I just learned that they're delimited by semicolons:

cmake "-DRV_FFMPEG_CONFIG_OPTIONS=--enable-decoder=vp9;--enable-decoder=vp8;" ...
cmake "-DRV_FFMPEG_CONFIG_OPTIONS=--extra-cflags=-I/include/dir1;--extra-cflags=-I/include/dir2" ...

so I suppose that it maybe isn't necessary to add SEPARATE_ARGUMENTS to RV_FFMPEG_CONFIG_OPTIONS, also considering how CMake only documents its use for commands, though I would appreciate any other thoughts.

RV_FFMPEG_PATCH_COMMAND_STEP

However, using SEPARATE_ARGUMENTS fixes RV_FFMPEG_PATCH_COMMAND_STEP, since it would otherwise cd to the command string like so:

$ rvcfg -DRV_FFMPEG_PATCH_COMMAND_STEP="PATCH_COMMAND echo 'foo' && echo 'bar'"
      <Command Condition="'$(Configuration)|$(Platform)'=='Release|x64'">setlocal
cd "C:\OpenRV\_build\cmake\dependencies\PATCH_COMMAND echo 'foo' &amp;&amp; echo 'bar'"
...

SEPARATE_ARGUMENTS plus the accompanying changes finally let me pass mildly long and complex commands from the terminal:

cmake ... "-DRV_FFMPEG_PATCH_COMMAND_STEP=touch foobar && git cherry-pick -n 988f2e9eb063db7c1a678729f58aab6eba59a55b 031f1561cd286596cdb374da32f8aa816ce3b135 effadce6c756247ea8bae32dc13bb3e6f464f0eb 03823ac0c6a38bd6ba972539e3203a592579792f d2b46c1ef768bc31ba9180f6d469d5b8be677500"

which addresses #15 (comment)!

PKG_CONFIG_PATH

The PKG_CONFIG_PATH error was driving me nuts before I saw Mark's PR, which included errors such as:

88>CUSTOMBUILD : error : dav1d >= 0.5.0 not found using pkg-config

since there was no way to un-escape $ or %:

...\cmake.exe -E env PKG_CONFIG_PATH="$"PKG_CONFIG_PATH:/C/OpenRV/_build/RV_DEPS_DAV1D/install/lib/pkgconfig sh ./configure ...
...\cmake.exe -E env PKG_CONFIG_PATH=%%PKG_CONFIG_PATH%%:/C/OpenRV/_build/RV_DEPS_DAV1D/install/lib/pkgconfig sh ./configure ...

After using the PR, I noticed that CMake was treating $ENV{PKG_CONFIG_PATH} as a list of DOS paths and joining them with spaces:

... env PKG_CONFIG_PATH=C:\msys64\mingw64\lib\pkgconfig C:\msys64\mingw64\share\pkgconfig:/C/OpenRV/_build3/RV_DEPS_DAV1D/install/lib/pkgconfig sh ./configure

despite how MINGW itself was storing the values

$ echo $PKG_CONFIG_PATH
/mingw64/lib/pkgconfig:/mingw64/share/pkgconfig

so my changes kind of awkwardly iterate through the paths and convert them back to POSIX and then join them into a single path string at the end:

... PKG_CONFIG_PATH=/C/msys64/mingw64/lib/pkgconfig:/C/msys64/mingw64/share/pkgconfig:/C/OpenRV/_build2/RV_DEPS_DAV1D/install/lib/pkgconfig sh ./configure

though I'm keen to know if there's a better way to CMake it.

Testing

I was able to get a version built for both Windows 10.0.19045 and Arch Linux 6.6.8 and play the bird video again without issues.

Submission

Hopefully that all looks okay! I'm not sure how to proceed with this, but since Mark already opened a PR, I'd be happy to put in a PR to his fork if that sounds good. Thanks!

[BrianHanke]

Just a quick comment on this topic. I ran into the FFmpeg problem last night on Windows. I didn't see this PR when I was looking for a fix, but I did find a several-month old issue that linked to this repo: https://github.com/ThomBern/OpenRV/tree/fix-build-ffmpeg

I updated dav1d.cmake, ffmpeg.cmake and openssl.cmake since those three had the "Fixing Open RV FFMpeg multiplatform" note. Long story short, that did the trick and I was able to build successfully.

[bernie-laberge]

@markreidvfx : I believe meepzh's PR that was just merged, combined your PR with his fix on Windows.
Do you think we can close this PR ?
Thanks!

[markreidvfx]

@bernie-laberge yes we can close this PR. Meepzh's PR solves my issues too.