Workaround for av context closing issue when using AUTO thread_type #8555
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
azhavoro
requested review from
bsekachev
and removed request for
nmanovic and
Marishka17
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
cvat/apps/engine/media_extractors.py (1)
555-556: Consider adding a reference to an issue tracker for context closing problemThe comments indicate that context closing may get stuck on some videos for an unknown reason, and the
thread_type == 'AUTO'setting is disabled for future investigation. To aid in tracking and resolving this issue, consider adding a TODO comment with a reference to an issue number or link to the issue tracker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- cvat/apps/engine/media_extractors.py (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
cvat/apps/engine/media_extractors.py (2)
640-641: Explicitly settingthread_typeto 'NONE' when threading is disabledThe addition of
video_stream.thread_type = 'NONE'ensures that threading is explicitly disabled whenallow_threadingisFalse. This change aligns with the intention to prevent threading-related issues during video processing.
802-803: Consistent threading behavior inVideoReaderWithManifestSetting
video_stream.thread_typeto 'NONE' in theVideoReaderWithManifestclass maintains consistency with theVideoReaderclass when threading is disabled. This ensures that both classes handle threading in the same manner whenallow_threadingisFalse.
| @@ -583,7 +585,7 @@ def __init__( | |||
| stop: Optional[int] = None, | |||
| dimension: DimensionType = DimensionType.DIM_2D, | |||
| *, | |||
| allow_threading: bool = True, | |||
| allow_threading: bool = False, | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: VideoReader Instantiation Missing allow_threading Parameter
An instance of VideoReader is instantiated without explicitly setting allow_threading, which may lead to unintended side effects due to the default value change.
cvat/apps/engine/migrations/0024_auto_20191023_1025.py:68:reader = VideoReader([original_video], get_frame_step(frame_filter), start_frame, _stop_frame)
🔗 Analysis chain
Changing default allow_threading to False may introduce unintended side effects
Changing the default value of allow_threading from True to False might impact existing functionality that relies on threading being enabled by default. Please verify that all usages of the VideoReader class are reviewed and updated if necessary to prevent any unintended behavior.
Run the following script to identify instances where VideoReader is instantiated without explicitly setting allow_threading:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instantiations of VideoReader without 'allow_threading' parameter specified.
# Expected Output: List of files and line numbers where 'VideoReader' is instantiated without 'allow_threading'.
ast-grep --lang python --pattern $'VideoReader($_, $_, $_, $_, $_)'
Length of output: 68
Script:
#!/bin/bash
# Description: Find all instantiations of VideoReader without 'allow_threading' parameter specified, regardless of the number of parameters.
# Expected Output: List of files and line numbers where 'VideoReader' is instantiated without 'allow_threading'.
ast-grep --lang python --pattern $'VideoReader($_, $$$)' | \
grep -v 'allow_threading='
Length of output: 257
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8555 +/- ##
===========================================
- Coverage 74.26% 74.26% -0.01%
===========================================
Files 400 400
Lines 43218 43224 +6
Branches 3909 3909
===========================================
+ Hits 32096 32100 +4
- Misses 11122 11124 +2
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Bug Fixes
New Features