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

Use Infer to detect image/video #101

Merged
merged 6 commits into from Dec 23, 2022

Conversation

Pyroglyph
Copy link
Contributor

Maintaining our own list of known filetypes seems pointless when there are other projects like Infer that do it for us.

Marked as draft until this is answered:
If Infer fails to load the file then ProbeErrors are created despite not being thrown by FFProbe. This is because the check happens in the probe function (for convenience). Is this okay or should a more specific error type be created for this? In any case, the whole thing would have failed anyway if the file is unable to be read.

Maintaining our own list of known filetypes seems pointless when there are other projects that do it for us.
@alexheretic
Copy link
Owner

Thanks! The additional binary bloat and sync-io blocking is undesirable, but overall this looks worth it.

@alexheretic
Copy link
Owner

alexheretic commented Dec 22, 2022

If Infer fails to load the file then ProbeErrors are created despite not being thrown by FFProbe. This is because the check happens in the probe function (for convenience). Is this okay or should a more specific error type be created for this? In any case, the whole thing would have failed anyway if the file is unable to be read.

Yeah it's kinda academic as we'd expect the probe to have failed. I probably would have just Err(_) => false to reduce the code, but returning an error as you have done is fine too and more defensively correct. I don't think we need to throw more code at this scenario though.

@Pyroglyph
Copy link
Contributor Author

Pyroglyph commented Dec 22, 2022

The additional binary bloat is undesirable

Infer can be imported with no default features to make it lighter. This means that we won't be able to use the infer::get_from_path helper any more, but if size is something we're concerned about then it should just be a matter of changing a few lines.

@alexheretic
Copy link
Owner

Size is a concern, if not a critical one. Just something I keep an eye on when adding new dependencies. As I said I think the trade off is worth it here.

This makes the final binary smaller by about 71kb
@Pyroglyph
Copy link
Contributor Author

I've done some measurements. When building in --release mode I get these file sizes:

Version File size
No Infer 2,363 KB
Infer (default features) 2,445 KB (+82 KB)
Infer (no default features) 2,374 KB (+11 KB)

I'd say that's quite a nice jump.

src/ffprobe.rs Outdated Show resolved Hide resolved
@alexheretic
Copy link
Owner

alexheretic commented Dec 23, 2022

It's a mild annoyance that we don't need no_std, we just want to avoid the cfb dependency. But infer has bundled them together in the "std" feature 🤷.

Although not using get_from_path will be easier to convert to async later, once ffprobe is also async (or sooner, but it's not a big deal).

@alexheretic alexheretic marked this pull request as ready for review December 23, 2022 10:40
Copy link
Owner

@alexheretic alexheretic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@alexheretic alexheretic merged commit 031db4a into alexheretic:main Dec 23, 2022
@Pyroglyph Pyroglyph deleted the better-image-detection branch December 23, 2022 11:44
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.

None yet

2 participants