Restructure the video/video_reader C++ codebase #3311
Conversation
datumbox
force-pushed
the
refactor/video
branch
from
f1b474d to
9f8ca89
Compare
| .def("set_current_stream", &Video::setCurrentStream) | ||
| .def("get_metadata", &Video::getStreamMetadata) | ||
| .def("seek", &Video::Seek) | ||
| .def("next", &Video::Next); |
There was a problem hiding this comment.
Moved from register.cpp
| @@ -417,95 +418,6 @@ torch::List<torch::Tensor> readVideo( | |||
| return result; | |||
| } | |||
|
|
|||
| torch::List<torch::Tensor> readVideoFromMemory( | |||
There was a problem hiding this comment.
Moving out of the anonymous namespace.
| torch::List<torch::Tensor> probeVideoFromMemory(torch::Tensor input_video) { | ||
| } // namespace | ||
|
|
||
| torch::List<torch::Tensor> read_video_from_memory( |
| m.def("read_video_from_memory", read_video_from_memory); | ||
| m.def("read_video_from_file", read_video_from_file); | ||
| m.def("probe_video_from_memory", probe_video_from_memory); | ||
| m.def("probe_video_from_file", probe_video_from_file); |
There was a problem hiding this comment.
Registration of methods within the namespaces at the end of the file.
| @@ -18,8 +14,13 @@ PyMODINIT_FUNC PyInit_video_reader(void) { | |||
| } | |||
| #endif | |||
|
|
|||
| using namespace ffmpeg; | |||
|
|
|||
| namespace vision { | |||
There was a problem hiding this comment.
Follow similar pattern as vision::ops
| namespace vision { | ||
| namespace video_reader { | ||
|
|
||
| torch::List<torch::Tensor> read_video_from_memory( |
There was a problem hiding this comment.
public methods in header
Codecov Report
@@ Coverage Diff @@
## master #3311 +/- ##
=======================================
Coverage 73.93% 73.93%
=======================================
Files 104 104
Lines 9594 9594
Branches 1531 1531
=======================================
Hits 7093 7093
Misses 2024 2024
Partials 477 477 Continue to review full report at Codecov.
|
datumbox
changed the title
There was a problem hiding this comment.
Looks great to me, thanks!
One thing I was thinking was if in the future we would want to move decoder inside video, and have both video_reader and video.cpp within the video folder.
Something like
io/
image/
video/
decoder/
video.cpp
video_reader_legacy.cpp (renamed from video_reader)
...
This is minor though, but just passed through my mind as something which could improve in clarity about the role of the different video decoders
|
@fmassa Do you think it makes sense to move decoder and video_reader in Edit: We agreed to do it on a separate PR to take care of building code on FBcode. Note: This PR will need special handling for merging into FBcode. See D26110952. |
Summary: * Moving registration of video methods in Video.cpp and removing unnecessary includes. * Rename files according to cpp styles. * Adding namespaces and moving private methods to anonymous namespaces. * Syncing method names. * Fixing minor issues. Reviewed By: fmassa Differential Revision: D26197746 fbshipit-source-id: dfeaa3144574899e5dfe32fee21575a8c3602cd0
Fixes #3155