Feat/image folder support #39
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis PR introduces support for sampling images from folders by implementing a new ImageSampler class alongside the existing VideoSampler. The implementation includes a significant refactoring of the sampler architecture, introducing a BaseSampler class for shared functionality. Buffer handling has been enhanced with additional methods and improved configuration validation. The changes are accompanied by comprehensive test coverage for the new functionality. Class diagram for the new ImageSampler and refactored samplersclassDiagram
class BaseSampler {
+SamplerConfig cfg
+FrameBuffer frame_buffer
+BlurGate | ClipGate | PassGate gate
+Counter stats
+sample(str) Iterable~list~FrameObject~~
+write_queue(str, Queue, str) void
+init_sampler() void
+flush_buffer() Iterable~list~FrameObject~~
+process_frame(int, Image, float) Iterable~list~FrameObject~~
}
class ImageSampler {
+ImageSamplerConfig cfg
+Regex rgx
+extract_frame_time(str, str) str
+sample(str) Iterable~list~FrameObject~~
+write_queue(str, Queue, str) void
}
class VideoSampler {
+SamplerConfig cfg
+sample(str, str) Iterable~list~FrameObject~~
+write_queue(str, Queue, str) void
}
class SegmentSampler {
+SamplerConfig cfg
+sample(str, str) Iterable~list~FrameObject~~
+write_queue(str, Queue, str) void
}
BaseSampler <|-- ImageSampler
BaseSampler <|-- VideoSampler
VideoSampler <|-- SegmentSampler
class ImageSamplerConfig {
+str frame_time_regex
}
class SamplerConfig {
+float min_frame_interval_sec
+bool keyframes_only
+dict buffer_config
+dict gate_config
}
SamplerConfig <|-- ImageSamplerConfig
SamplerConfig <|-- BaseSampler
SamplerConfig <|-- VideoSampler
SamplerConfig <|-- SegmentSampler
Class diagram for the updated Buffer classesclassDiagram
class FrameBuffer {
+add(Image, dict) tuple~Image, dict~~
+final_flush() Iterable~tuple~Image, dict~~
+clear() void
+__len__() int
+__iter__() Iterator
+__getitem__(str) tuple~Image, dict~~
+popitem(bool) tuple~Image, dict~~
}
class HashBuffer {
+add(Image, dict) tuple~Image, dict~~
+final_flush() Iterable~tuple~Image, dict~~
+clear() void
+__len__() int
+__iter__() Iterator
+__getitem__(str) tuple~Image, dict~~
+popitem(bool) tuple~Image, dict~~
}
class GridBuffer {
+add(Image, dict) tuple~Image, dict~~
+final_flush() Iterable~tuple~Image, dict~~
+clear() void
+__len__() int
+__iter__() Iterator
+__getitem__(str) tuple~Image, dict~~
+popitem(bool) tuple~Image, dict~~
}
FrameBuffer <|-- HashBuffer
FrameBuffer <|-- GridBuffer
note for FrameBuffer "Enhanced with new methods for length, iteration, and item access"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @LemurPwned - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider standardizing error handling across samplers - the video sampler uses specific av.error exceptions while the image sampler uses broad Exception catching which could mask issues
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @LemurPwned - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ft = f"{vbsn}_{ft}" | ||
| return os.path.join(output_path, f"{ft}.jpg") | ||
|
|
||
| def queue_reader(self, output_path, read_interval=0.1) -> None: |
There was a problem hiding this comment.
issue (complexity): Consider extracting summary generation logic into a dedicated SummaryManager class to improve code organization.
The queue_reader method mixes frame processing with summary generation logic, making it harder to maintain. Consider extracting the summary generation into a dedicated class:
class SummaryManager:
def __init__(self, pool, desc_client, config, debug=False):
self.pool = pool
self.desc_client = desc_client
self.config = config
self.debug = debug
self.futures = {}
self.last_summary_time = -10
def maybe_submit_summary(self, frame, frame_time):
if not self.pool:
return
if frame_time - self.last_summary_time < self.config.get("min_sum_interval", 30):
return
future = self.pool.submit(self.desc_client.summarise_image, frame)
if self.debug:
console.print(f"Submitting summary for frame {frame_time}", style="bold yellow")
self.futures[frame_time] = future
self.last_summary_time = frame_timeThis simplifies the queue_reader to:
def queue_reader(self, output_path, read_interval=0.1) -> None:
summary_mgr = SummaryManager(self.pool, self.desc_client, self.cfg.summary_config, self.cfg.debug)
while True:
if not self.q.empty():
for frame_object in self.q.get():
if frame_object.metadata.get("end", False):
self.futures = summary_mgr.futures # preserve futures for collection
return
if frame_object.frame is not None and not self.devnull and isinstance(frame_object.frame, Image.Image):
frame_object.frame.save(
self.format_output_path(output_path, frame_object.metadata["frame_time"])
)
summary_mgr.maybe_submit_summary(frame_object.frame, frame_object.metadata["frame_time"])
time.sleep(read_interval)This separation makes the code more maintainable by:
- Isolating summary generation logic and state
- Simplifying the main loop's flow
- Making the timing constraints more explicit
|
@fcakyon python3 -m video_sampler config ./configs/image_base.yaml "./folder-frames/worlds-smallest-cat-bbc" ./sampled-output/ --images |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce support for image folder input in the sampler, allowing processing of images directly from directories. Refactor buffer management and validation to improve robustness and flexibility. Enhance the sampler to handle both video and image inputs with the addition of the ImageSampler class. Add comprehensive tests for new and existing functionalities to ensure reliability.
New Features:
Enhancements:
Tests: