-
Notifications
You must be signed in to change notification settings - Fork 62
Refactor pybind_ops to only deal with file like context holders #889
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @scotts , left 2 areas to potentially investigate but that can be left as follow-ups. I agree this is a net improvement.
// pybind_ops.cpp with the global visibility flag as required. On the | ||
// custom_ops.cpp side we don't need to do that. However, we do need to ensure | ||
// that this class is also seen as the same visibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but if we can, we might as well set the hidden visibility flag for the whole pybind_ops lib and remove the VISIBILITY_HIDDEN
stuff:
target_compile_options(
${custom_ops_library_name}
PUBLIC
"-fvisibility=hidden"
)
This seems to work OK locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I did, I can confirm this compiles locally
diff --git a/src/torchcodec/_core/AVIOFileLikeContext.h b/src/torchcodec/_core/AVIOFileLikeContext.h
index 3146c00..fd7f534 100644
--- a/src/torchcodec/_core/AVIOFileLikeContext.h
+++ b/src/torchcodec/_core/AVIOFileLikeContext.h
@@ -11,27 +11,13 @@
#include "src/torchcodec/_core/AVIOContextHolder.h"
-// pybind11 has specific visibility rules; see:
-// https://pybind11.readthedocs.io/en/stable/faq.html#someclass-declared-with-greater-visibility-than-the-type-of-its-field-someclass-member-wattributes
-// This file is included in both pybind_ops.cpp and custom_ops.cpp. We compile
-// pybind_ops.cpp with the global visibility flag as required. On the
-// custom_ops.cpp side we don't need to do that. However, we do need to ensure
-// that this class is also seen as the same visibility.
-//
-// This only matters on Linux and Mac; on Windows we don't need to do anything.
-#ifdef _WIN32
-#define VISIBILITY_HIDDEN
-#else
-#define VISIBILITY_HIDDEN __attribute__((visibility("hidden")))
-#endif
-
namespace py = pybind11;
namespace facebook::torchcodec {
// Enables uers to pass in a Python file-like object. We then forward all read
// and seek calls back up to the methods on the Python object.
-class VISIBILITY_HIDDEN AVIOFileLikeContext : public AVIOContextHolder {
+class AVIOFileLikeContext : public AVIOContextHolder {
public:
explicit AVIOFileLikeContext(const py::object& fileLike, bool isForWriting);
diff --git a/src/torchcodec/_core/CMakeLists.txt b/src/torchcodec/_core/CMakeLists.txt
index 03f68f6..c45a614 100644
--- a/src/torchcodec/_core/CMakeLists.txt
+++ b/src/torchcodec/_core/CMakeLists.txt
@@ -137,6 +137,14 @@ function(make_torchcodec_libraries
"${custom_ops_dependencies}"
)
+ if(NOT WIN32)
+ target_compile_options(
+ ${custom_ops_library_name}
+ PUBLIC
+ "-fvisibility=hidden"
+ )
+ endif()
+
# 3. Create libtorchcodec_pybind_opsN.so.
set(pybind_ops_library_name "libtorchcodec_pybind_ops${ffmpeg_major_version}")
set(pybind_ops_sources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NicolasHug, yeah, I did the same. The one difference is that I put both of them under the same if(NOT WIN32)
and explaining comment. It breaks the convention of only doing one library at a time, but I think it helps understanding. All of the visibility stuff, with the same cause, are all in one place.
std::move(avioContextHolder), | ||
audioStreamOptions); | ||
encoder.encode(); | ||
int64_t create_file_like_context(py::object file_like, bool is_for_writing) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above was meant to be about "laundering into into tensors" because it was about the decoder object itself. Now we're laundering an int into an AVIOFileLikeContext
object IIUC, not a tensor, so the comment may need to be updated.
Separately from the comment, and since we shouldn't need the intermediate tensor representation, I wonder if we could actually work around all that by "cleanly" binding the AVIOFileLikeContext
to Python and just pass normal py::objects? I thought the whole laundering problem was due to a problem between pybind and torch tensors, but IIUC we don't need the tensor part at all anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe py::objects need to be existing kinds of objects Python already knows about. I thought about what you're suggesting, and I think that would mean actually defining a new kind of Python to C++ object through pybind. We can do it, but it will take some work to figure out if it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I improved the comment, referenced and created #896
Refactors
pybind_ops.cpp
so that it does not know aboutSingleStreamDecoder
at all, it only knows aboutAVIOFileLikeContext
. It now has a single function:Given a Python file-like object, it creates a
AVIOFileLikeContext
from it. We return a pointer to that object as a Python int. This means that all decoder creation is now incustom_ops.cpp
. They accept the int and cast it back into aAVIOFileLikeContext
pointer.Upsides:
pybind_ops.cpp
is now much simpler, and it's clear that it is only concerned with creating the context for file-like contexts.pybind_ops.cpp
returned an int.)encode_audio_to_file_like()
are now gone. We can pass the samples as a proper tensor.Downsides:
custom_ops.cpp
now has to accept an int from the Python side and do a cast.AVIOFileContextHolder
hidden in order to make visibility across bothpybind_ops.cpp
andcustom_ops.cpp
the same. I only roughly understand this part.I think that the upsides of this approach outweigh the downsides of how we were doing it before.