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

Clipping subsampler refactor #275

Merged
merged 17 commits into from
Jan 24, 2024

Conversation

MattUnderscoreZhang
Copy link
Contributor

@MattUnderscoreZhang MattUnderscoreZhang commented Jan 19, 2024

I need to add ffmpeg pipes to speed up processing, and to do that I first needed to refactor some of the code to make it easier to reason about.

For context, I'm processing webvid-10M and the codebase is currently too slow. I'm hoping to speed it up by an order of magnitude by combining subsamplers into a single ffmpeg pipe operation, and adding other optimizations.

@MattUnderscoreZhang MattUnderscoreZhang force-pushed the clipping_subsampler_refactor branch from b202296 to 32fa4ea Compare January 19, 2024 03:17
@iejMac
Copy link
Owner

iejMac commented Jan 19, 2024

nice, yes this is a great idea

@iejMac
Copy link
Owner

iejMac commented Jan 19, 2024

is this ready to go? If so could you give a high level overview of the changes?

@MattUnderscoreZhang
Copy link
Contributor Author

MattUnderscoreZhang commented Jan 19, 2024

This is basically just a refactor. I broke the code into smaller functions, added types, only process metadata once per clip (instead of once per stream), renamed some stuff for clarity, and cleaned up the segment_times collection code.

I avoided making any functional changes, but I think I see a couple of places for improvement that I'll hit in later pull requests. The ffmpeg pipe stuff is also going to come later, but I may need to refactor the other subsamplers first.

@rom1504
Copy link
Collaborator

rom1504 commented Jan 21, 2024

looks much better

I think the smaller functions could be an opportunity to add some more tests. What do you think?

@rom1504
Copy link
Collaborator

rom1504 commented Jan 21, 2024

just merged #262 and having a bit of trouble to rebase here hmm

@rom1504
Copy link
Collaborator

rom1504 commented Jan 21, 2024

probably best if you handle this rebase @MattUnderscoreZhang ; I think it's mostly about replacing the code that's doing the extract subtitle by that new function

@MattUnderscoreZhang
Copy link
Contributor Author

Yeah no prob, I just merged changes.

@rom1504
Copy link
Collaborator

rom1504 commented Jan 22, 2024

@MattUnderscoreZhang did you test this to produce the same results as before? this is a lot of complex code that is changed

@MattUnderscoreZhang
Copy link
Contributor Author

MattUnderscoreZhang commented Jan 24, 2024

Yes, I get the exact same results as before for my use case, which involves a frame rate resampling, resizing, cropping, and clipping.

Plus, all the unit tests pass as expected.

@rom1504 rom1504 merged commit e7a4591 into iejMac:main Jan 24, 2024
2 checks passed
@pabl0 pabl0 mentioned this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants