Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tiff lib and remove stub ffmpeg libraries for macOS support #119

Merged
merged 2 commits into from
Feb 25, 2019

Conversation

yongtang
Copy link
Member

The video ops used to be linked to a stub (empty) library so that in compile time it is possible to build tensorflow-io without actually providing ffmpeg dynamic library. This causes issues in bazel's sandboxed mode because sandboxed one need to know all files in advance. So far we have been using standalone mode to address this issue.

It is still desirable to use sandbox mode as there might be some unintended effects. For example, when we compile tiff we assume jpeg is not supported, but in Linux it is almost certain it will
include a header from system library. This is one of the reasons causing MacOS to fail (include path different from linux).

In short, we should use sandboxed mode for Bazel.

This PR fixes the TIFF issue, and also uses dlopen(RTLD_GLOBAL) to resolve ffmpeg symbols in run time, effectively avoids the stub (empty) libary we have.

This PR is part of the effort to spport macOS for tensorflow-io.

Signed-off-by: Yong Tang [email protected]

The video ops used to be linked to a stub (empty) library
so that in compile time it is possible to build tensorflow-io
without actually providing ffmpeg dynamic library.
This causes issues in bazel's sandboxed mode because sandboxed
one need to know all files in advance. So far we have been
using standalone mode to address this issue. It is still
desirable to use sandbox mode as there might be some unintended
effects. For example, when we compile tiff we assume
jpeg is not supported, but in Linux it is almost certain it will
include a header from system library. Which is one of the
reasons causeing MacOS to fail (include path different from linux).

In short, we should use sandboxed mode for Bazel.

This PR fixes the TIFF issue, and also uses dlopen(RTLD_GLOBAL)
to resolve ffmpeg symbols in run time, effectively avoids
the stub (empty) libary we have.

This PR is part of the effort to spport macOS for tensorflow-io.

Signed-off-by: Yong Tang <[email protected]>
@yongtang
Copy link
Member Author

/cc @terrytangyuan @BryanCutler

@yongtang
Copy link
Member Author

We will need this fix for macOS support in #116

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using sandboxed mode consistent with what's done in other projects?

@yongtang
Copy link
Member Author

@terrytangyuan Yes sandboxed mode is the preferred (and default) mode in bazel and cross aboard. As sandboxed mode gives you a more repeatable result without the impact of the machine you compile (e.g., having some package installed on your linux or not).

We were forced to use standalone mode + stub ffmpeg .so because bazel does not support soname like so.57. I created a bug/feature request in Bazel (bazelbuild/bazel#7059) some time ago. It is labeled with bazel 1.0 but I don't know when this will be released.

In our previous setup, we use a bare-bone Ubuntu 14.04 to build the binary so the binary is (to an extent repeatable). However, because our Ubuntu 14.04 also comes with python and python consists many modules, a lot of header files from other packages are exposed in system directory. This is causing the tiff/jpeg issue when the same code is compiled in macOS.

This fix will help make sure header files from installed packages like matplotlib or numpy will not impact our compiled binary (their version could differ).

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks for the clarification.

@terrytangyuan terrytangyuan merged commit 57e0859 into tensorflow:master Feb 25, 2019
@yongtang yongtang deleted the dlopen branch February 25, 2019 16:22
@BryanCutler
Copy link
Member

After this, should I remove --spawn_strategy standalone from #104 README?

@yongtang
Copy link
Member Author

@BryanCutler Yes there is no need to run with --spawn_strategy standalone anymore. Even ffmpeg is not needed to compile (only needed to run tests).

i-ony pushed a commit to i-ony/io that referenced this pull request Feb 8, 2021
Fix tiff lib and remove stub ffmpeg libraries for macOS support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants