Refactor the constructors of pointer wrappers #2373
Conversation
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
2 similar comments
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
This commit refactor the constructor of wrapper classes so that
wrapper classes are only responsible for deallocation of underlying
FFmpeg custom structures.
The responsibility of custom initialization is moved to helper functions.
Context:
FFmpeg API uses bunch of raw pointers, which require dedicated allocater
and deallcoator. In torchaudio we wrap these pointers with
`std::unique_ptr<>` to adopt RAII semantics.
Currently all of the customization logics required for `Streamer` are
handled by the constructor of wrapper class. Like the following;
```
AVFormatContextPtr(
const std::string& src,
const std::string& device,
const std::map<std::string, std::string>& option);
```
This constructor allocates the raw `AVFormatContext*` pointer,
while initializing it with the given option, then it parses the
input media.
As we consider the write/encode features, which require different way
of initializing the `AVFormatContext*`, making it the responsibility
of constructors of `AVFormatContextPtr` reduce the flexibility.
Thus this commit moves the customization to helper factory function.
- `AVFormatContextPtr(...)` -> `get_input_format_context(...)`
- `AVCodecContextPtr(...)` -> `get_decode_context(...)`
Pull Request resolved: pytorch#2373
Differential Revision: D36230148
Pulled By: mthrok
fbshipit-source-id: 285892a3aa0a8676592bf4ac996aa8e8642f38f8
|
This pull request was exported from Phabricator. Differential Revision: D36230148 |
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| decoder_option, | ||
| device, | ||
| pHWBufferRef); | ||
| } |
There was a problem hiding this comment.
Originally the codec context was initialized inside of CodecContextPtr class constructor, now it's done in call site.
| @@ -7,6 +7,7 @@ namespace ffmpeg { | |||
|
|
|||
| class Decoder { | |||
| AVCodecContextPtr pCodecContext; | |||
| AVBufferRefPtr pHWBufferRef; | |||
There was a problem hiding this comment.
AVBufferRefPtr has been moved out of AVCodecContextPtr to call site.
| @@ -62,7 +62,9 @@ std::string join(std::vector<std::string> vars) { | |||
| #define AVINPUT_FORMAT_CONST | |||
| #endif | |||
|
|
|||
| AVFormatContext* get_format_context( | |||
There was a problem hiding this comment.
get_format_context is now the factory function used by call site.
| throw std::runtime_error("Failed to find stream information."); | ||
| } | ||
| AVFormatContextPtr::AVFormatContextPtr(AVFormatContext* p) | ||
| : Wrapper<AVFormatContext, AVFormatContextDeleter>(p) {} |
There was a problem hiding this comment.
AVFormatContextPtr is only responsible for pointer clean-up. The initialization logic has been moved to call site.
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
This commit refactor the constructor of wrapper classes so that
wrapper classes are only responsible for deallocation of underlying
FFmpeg custom structures.
The responsibility of custom initialization is moved to helper functions.
Context:
FFmpeg API uses bunch of raw pointers, which require dedicated allocater
and deallcoator. In torchaudio we wrap these pointers with
`std::unique_ptr<>` to adopt RAII semantics.
Currently all of the customization logics required for `Streamer` are
handled by the constructor of wrapper class. Like the following;
```
AVFormatContextPtr(
const std::string& src,
const std::string& device,
const std::map<std::string, std::string>& option);
```
This constructor allocates the raw `AVFormatContext*` pointer,
while initializing it with the given option, then it parses the
input media.
As we consider the write/encode features, which require different way
of initializing the `AVFormatContext*`, making it the responsibility
of constructors of `AVFormatContextPtr` reduce the flexibility.
Thus this commit moves the customization to helper factory function.
- `AVFormatContextPtr(...)` -> `get_input_format_context(...)`
- `AVCodecContextPtr(...)` -> `get_decode_context(...)`
Pull Request resolved: pytorch#2373
Reviewed By: hwangjeff
Differential Revision: D36230148
Pulled By: mthrok
fbshipit-source-id: 289426662c12d42ff9c7d956c58b8cef7e6c9221
|
This pull request was exported from Phabricator. Differential Revision: D36230148 |
|
Hey @mthrok. |
Summary: This commits move helper functions/definitions around so that better locality of logics are achieved. ## Detail `ffmpeg.[h|cpp]` implements classes that convert FFmpeg structures into RAII semantics. Initially it these classes included the construction logic in their constructors, but such logics were extracted to factory functions in #2373. Now the reason why the factory functions stayed in `ffmpeg.[h|cpp]` was because the logic for the initialization and clean-up of AVDictionary class was only available in `ffmpeg.cpp`. Now AVDictionary class handling is properly defined in #2507, the factory functions, which are not that reusable better stay with the implementation that use them. This makes `ffmpeg.h` lean and clean, makes it easier to see what can be reused. Pull Request resolved: #2512 Reviewed By: hwangjeff Differential Revision: D37477592 Pulled By: mthrok fbshipit-source-id: 8c1b5059ea5f44649cc0eb1f82d1a92877ef186e
This commit refactor the constructor of wrapper classes so that
wrapper classes are only responsible for deallocation of underlying
FFmpeg custom structures.
The responsibility of custom initialization is moved to helper functions.
Context:
FFmpeg API uses bunch of raw pointers, which require dedicated allocater
and deallcoator. In torchaudio we wrap these pointers with
std::unique_ptr<>to adopt RAII semantics.Currently all of the customization logics required for
Streamerarehandled by the constructor of wrapper class. Like the following;
This constructor allocates the raw
AVFormatContext*pointer,while initializing it with the given option, then it parses the
input media.
As we consider the write/encode features, which require different way
of initializing the
AVFormatContext*, making it the responsibilityof constructors of
AVFormatContextPtrreduce the flexibility.Thus this commit moves the customization to helper factory function.
AVFormatContextPtr(...)->get_input_format_context(...)AVCodecContextPtr(...)->get_decode_context(...)