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

Expose constructors in C++ #434

Merged
merged 3 commits into from
Dec 14, 2024
Merged

Expose constructors in C++ #434

merged 3 commits into from
Dec 14, 2024

Conversation

scotts
Copy link
Contributor

@scotts scotts commented Dec 14, 2024

Prior to this PR, the VideoDecoder class in C++ had a single private constructor that did not do anything. The static create family of functions were effectively the constructors because they would set up the fields and do the initialization. We also had an empty DecoderOptions struct that was intentioned to be filled with parameters we would need at construction time. This PR:

  1. Adds two public, explicit constructors.
  2. Implements the create family of functions by calling the constructors.
  3. Removes the empty options struct.

The value of doing this:

  1. C++ has rich semantics and features around object construction. We were not taking advantage of it, and instead on a path to replicate some it by hand.
  2. Before this change, no one could instantiate a VideoDecoder object on the stack. There's no need to prevent that use.
  3. It's easier to reason about C++ objects when they obey the principle that the object is valid if the constructor has succeeded.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Dec 14, 2024
@scotts scotts marked this pull request as ready for review December 14, 2024 01:54
@scotts scotts merged commit 2bc0f8c into pytorch:main Dec 14, 2024
36 of 37 checks passed
@scotts scotts deleted the approx branch December 14, 2024 01:54
}

std::unique_ptr<VideoDecoder> VideoDecoder::createFromBuffer(
const void* buffer,
size_t length,
const VideoDecoder::DecoderOptions& options) {
TORCH_CHECK(buffer != nullptr, "Video buffer cannot be nullptr!");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this TORCH_CHECK was almost certainly optimized out in optimized builds. Because we already used buffer, it being null at this point would be undefined behavior, and the C++ compiler is allowed to assume it won't happen. Null checks must always be before use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants