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

Fix result saving issue when multi-stream detection #19297

Merged
merged 5 commits into from
Mar 9, 2025

Conversation

XevenQC
Copy link
Contributor

@XevenQC XevenQC commented Feb 18, 2025

It would save the results of all streams in the same video file with name 101.avi when the RTSP addresses with the same suffix, such as below

rtsp://admin:hik12345@10.224.191.241:5560/Streaming/Channels/101 rtsp://admin:hik12345@10.224.191.241:5564/Streaming/Channels/101

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Improves handling of video source paths in the loaders.py module to ensure better compatibility across platforms. 🔧

📊 Key Changes

  • Adjusted sources cleaning to replace OS-specific path separators (e.g., / or \) with underscores (_) in video source names.

🎯 Purpose & Impact

  • 🛠 Purpose: Ensures consistent handling of file paths regardless of operating system (Windows, Linux, macOS).
  • 🌍 Impact: Reduces errors related to path formatting when loading video streams, especially for cross-platform projects.
  • 🚀 User Benefit: Smoother and more reliable experience when working with diverse file sources.

Copy link

sentry-io bot commented Feb 18, 2025

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: ultralytics/engine/predictor.py

Function Unhandled Issue
write_results TypeError: must be real number, not str ultralyti...
Event Count: 2
write_results AttributeError: 'dict' object has no attribute 'pose_palette' ultralytics.utils.plottin...
Event Count: 2
write_results TypeError: can only concatenate tuple (not "str") to tuple ultralytics.engine.results in...
Event Count: 1
write_results TypeError: unsupported operand type(s) for +=: 'Tensor' and 'tuple' ultralytics.engine....
Event Count: 1

Did you find this useful? React with a 👍 or 👎

Copy link

github-actions bot commented Feb 18, 2025

All Contributors have signed the CLA. ✅
Posted by the CLA Assistant Lite bot.

@UltralyticsAssistant UltralyticsAssistant added detect Object Detection issues, PR's enhancement New feature or request labels Feb 18, 2025
@UltralyticsAssistant
Copy link
Member

👋 Hello @XevenQC, thank you for submitting an ultralytics/ultralytics 🚀 PR fixing the result saving issue with multi-webcam detection! This is an automated response to help you ensure your PR is ready for review. One of our Ultralytics engineers will also assist you soon. 🛠️

To streamline the review process, please review the following checklist:

  • Define a Purpose: You've provided an excellent description of the bug and explained the need for the improvement. If applicable, please link to any relevant issues, so we can track related discussions.
  • Synchronize with Source: Make sure your PR is synchronized with the main branch of the ultralytics/ultralytics repository. If your branch is outdated, update it either by clicking the 'Update branch' button or running git pull and git merge main locally.
  • Ensure CI Checks Pass: Verify all Ultralytics Continuous Integration (CI) checks are passing. If any checks are failing, please investigate and resolve the underlying issues.
  • Update Documentation: If your changes impact how the library is used, take a moment to update the relevant documentation to assist other users.
  • Add Tests: Include or update automated tests to cover your changes and confirm all tests pass.
  • Sign the CLA: If this is your first contribution, please ensure you have signed our Contributor License Agreement (CLA). You can do this by commenting "I have read the CLA Document and I sign the CLA" in this PR. This is required before merge approval. 🙏
  • Minimize Changes: Limit the scope of your changes to what's strictly necessary for resolving this issue or enhancing this feature.

Additional Notes:

If possible, please provide a minimal reproducible example (MRE) or additional context to help us test your fix more efficiently. For example:

  • A short code snippet demonstrating the bug or use case
  • Example RTSP addresses or similar configurations that reproduce the scenario

For more guidance, please refer to our Contributing Guide. If you have any questions or face any issues, feel free to leave a comment. We're here to help. 🚀

Thank you again for your contribution to Ultralytics! 🌟

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.25%. Comparing base (d390988) to head (ff090b8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ultralytics/data/loaders.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #19297   +/-   ##
=======================================
  Coverage   73.25%   73.25%           
=======================================
  Files         129      129           
  Lines       17590    17590           
=======================================
  Hits        12886    12886           
  Misses       4704     4704           
Flag Coverage Δ
Benchmarks 34.15% <0.00%> (ø)
GPU 37.86% <0.00%> (ø)
Tests 67.07% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@XevenQC
Copy link
Contributor Author

XevenQC commented Feb 18, 2025

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I sign the CLA

1 out of 2 committers have signed the CLA.✅ (UltralyticsAssistant)[https://github.com/UltralyticsAssistant]❌ @XevenQCYou can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@XevenQC XevenQC closed this Feb 18, 2025
@XevenQC XevenQC reopened this Feb 18, 2025
@XevenQC
Copy link
Contributor Author

XevenQC commented Feb 18, 2025

I have read the CLA Document and I sign the CLA

@XevenQC XevenQC changed the title Fix result saving issue when multi webcam detection Fix result saving issue when multi-stream detection Feb 18, 2025
@Laughing-q
Copy link
Member

Laughing-q commented Feb 28, 2025

@XevenQC Hi thanks for the PR! How about we directly add the modifications to the LoadStreams class where we load all the streams and record its names.
Actually we've already have a function clean_str to clean all the special characters with '_' character in stream address. Perhaps we could also add the updates here to further replace / to _.

self.sources = [ops.clean_str(x) for x in sources] # clean source names for later

something like:

        self.sources = [ops.clean_str(x).replace(os.path.sep, "_").replace(".", "_") for x in sources]  # clean source names for later

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
It would save the results of all webcam in the same video file
with name 101.avi when the RTSP addresses with the same suffix,
such as below

rtsp://admin:hik12345@10.224.191.241:5560/Streaming/Channels/101
rtsp://admin:hik12345@10.224.191.241:5564/Streaming/Channels/101

Signed-off-by: Chao Qin <chaoqin_cmkj@163.com>
XevenQC and others added 2 commits March 3, 2025 01:07
@Laughing-q
Copy link
Member

@XevenQC Thanks for the updates! I removed the update in clean_str though since we might use it to file path and / symbol is needed in that case. I think the update here replacing / in LoadStreams would be applied to stream urls only and it should work: https://github.com/XevenQC/ultralytics/blob/39d1aa7f4eff55fa8bf8cd0525ad8d541192f647/ultralytics/data/loaders.py#L109

@Laughing-q Laughing-q added the TODO High priority items label Mar 3, 2025
Signed-off-by: Glenn Jocher <glenn.jocher@ultralytics.com>
@glenn-jocher
Copy link
Member

@Laughing-q thanks for the review and updates, looks good, merging!

@glenn-jocher glenn-jocher merged commit bf35015 into ultralytics:main Mar 9, 2025
17 checks passed
@UltralyticsAssistant UltralyticsAssistant removed the TODO High priority items label Mar 9, 2025
@UltralyticsAssistant
Copy link
Member

🎉 This PR is officially merged! Huge thanks to @XevenQC for leading the charge on improving cross-platform compatibility, and to @glenn-jocher, @Y-T-G, and @Laughing-q for your valuable contributions and insights. 🛠️🌍 Your work ensures smoother file handling and a better experience for everyone in the community—true craftsmanship in action! 👏

As Marcus Aurelius said, "What stands in the way becomes the way." By tackling and refining these nuanced challenges, you've paved the way for a more resilient system, empowering users to create without barriers. 🚀

Your contributions matter and truly make a difference. Here's to more impactful collaborations ahead! 💡🌟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
detect Object Detection issues, PR's enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants