Switch to fmt:string for precision

[mollyxu]

Preserve precision for double to string conversions by using fmt::to_string() instead of string::to_string()

Note: some tests are edited to match the behavior of fmt::to_string() instead of string::to_string()

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/meta-pytorch/torchcodec/1203

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 4ffb47f with merge base dea4e93 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[mollyxu]

Would FetchContent + setting CMAKE_POSITION_INDEPENDENT_CODE be the best approach for linking fmt? An alternative is we could just use fmt::fmt-header-only

[Dan-Flores]

It looks like pytorch/pytorch uses fmt-header-only.

Another concern is that we are hardcoding this URL and version of the fmt library, so we would have to handle updates to this third party library as pytorch/pytorch does.

I'm not sure what the best way to handle this is, @NicolasHug any thoughts?

[NicolasHug]

Yeah we should be using whatever pytorch uses. I think we might be able to just rely on the headers that torch itself is shipping:

~/dev/torchcodec (rotation*) » find /home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/args.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/base.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/chrono.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/color.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/compile.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/core.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/format-inl.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/format.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/os.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/ostream.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/printf.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/ranges.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/std.h
/home/nicolashug/miniconda3/envs/codec/lib/python3.11/site-packages/torch/include/fmt/xchar.h

(sorry if this is a bit cryptic, I'll try to share more details tomorrow but that might unblock you for the time being)

[Dan-Flores]

fmt::to_string is locale-independent, so this PR resolves the locale bug without requiring us to set precision manually.
We can close #1200 in favor of this.

But to make the reproducing example pass, "bit_rate" needs to be set with std::to_string as it is a double. I believe all the other double metadata fields are handled:

torchcodec/src/torchcodec/_core/custom_ops.cpp

Line 815 in 85d1dad

metadataMap["bitRate"] = std::to_string(videoMetadata.bitRate.value());

torchcodec/src/torchcodec/_core/custom_ops.cpp

Line 876 in 85d1dad

map["bitRate"] = std::to_string(*containerMetadata.bitRate);

torchcodec/src/torchcodec/_core/custom_ops.cpp

Line 918 in 85d1dad

map["bitRate"] = std::to_string(*streamMetadata.bitRate);

Please double check me on this!

python -c "
  import locale
  locale.setlocale(locale.LC_NUMERIC, 'de_DE.UTF-8')
  from torchcodec.decoders import VideoDecoder
  decoder = VideoDecoder('test/resources/nasa_13013.mp4')
  "

[NicolasHug]

Thanks a lot @mollyxu , and thanks @Dan-Flores for cross-checking with #1200 ! I agree we can merge this in favor of #1200 since it's quite a bit simpler.

LGTM but let's wait on @Dan-Flores to make a final pass.

---

We have a fair amount of uses of std::to_string in the rest of the codebase. I think we should just use fmt::to_string everywhere? This could be a follow-up PR!

[Dan-Flores]

Please check if the change to test/CMakeLists.txt is necessary, after that LGTM!