Skip to content

Conversation

NicolasHug
Copy link
Member

Closes #679

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 27, 2025
@@ -123,6 +123,7 @@ function(make_torchcodec_libraries
set(custom_ops_sources
AVIOTensorContext.cpp
custom_ops.cpp
ValidationUtils.cpp
Copy link
Member Author

Choose a reason for hiding this comment

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

Not a fan creating a new cpp file for this, but I'm not sure where to have it otherwise. It's needed for both the pybind_ops and the custom_ops. Happy to consider alternatives.

Also, instead of listing ValidationUtils.cpp for both the custom_ops and pybind_ops libs, we could just list it once as part of the core lib.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to create a new cpp file for this, we may add more validations functions in the future.

Since it is used in both libraries I think it makes sense to add ValidationUtils.cpp to the core lib, as the other shared libraries are also there (SingleStreamDecoder.cpp, Encoder.cpp).

Copy link
Contributor

Choose a reason for hiding this comment

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

A new cpp for this is the best practice, as things are currently implemented. However, we might end up removing the cpp in the future because if we find ourselves implementing variants of these validation functions, we should probably instead define something like:

template <SrcT, DestT>
DestT validateIntegerConversion(SrcT value, const std::string& parameterName) {
  TORCH_CHECK(
      value >= std::numeric_limits<DestT>::min() &&
          value <= std::numeric_limits<DestT>::max(),
      parameterName,
      "=",
      value,
      " is out of range for int type.");

  return static_cast<DestT>(value);
}

And the definition of template functions must live in the header files. I don't think we should make this change yet because:

  1. My code above may not generalize correctly! Testing it's general correctness is not worth it right now.
  2. We may never need anything beyond this specific validation of types. (YAGNI.)

Yeah, I didn't do this in #830, but it was one function, and one could argue I was just being lazy. :)

@NicolasHug NicolasHug merged commit 1bd0c85 into pytorch:main Aug 28, 2025
54 checks passed
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.

Fix implicit conversions in VideoStreamOptions and AudioStreamOptions
3 participants