Skip to content

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 meta-pytorch:main Dec 14, 2024
36 of 37 checks passed
@scotts scotts deleted the approx branch December 14, 2024 01:54
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