Avoid extra ioctl by allowing control query by description

raymanfx / libv4l-rs

Video4Linux2 bindings for Rust

MIT License

155 stars 65 forks source link

Avoid extra ioctl by allowing control query by description #62

Closed donkeyteethUX closed 1 year ago

donkeyteethUX commented 2 years ago

Changes based on discussion on #60, specifically comments from @MarijnS95.

Add get_control which takes a &Description and requires only 1 ioctl.
Deprecate control getter in favor of more explicit get_control_from_id (and document preference for get_control).
Bump minor version.

raymanfx commented 2 years ago

I am not entirely convinced increasing the API surface is worth it just to avoid a single ioctl. This might very well be a case of premature optimization. Are you running into some actual issues that motivate this change?

MarijnS95 commented 2 years ago

I am not entirely convinced increasing the API surface is worth it just to avoid a single ioctl. This might very well be a case of premature optimization. Are you running into some actual issues that motivate this change?

I only suggested this to be able to pass an &Description instead of an untyped u32 which seemed a bit more clear to me. It's not premature optimization as much as it is removing unnecessary code that shouldn't have run/been there in the first place, IMO.

donkeyteethUX commented 2 years ago

@raymanfx agreed that it's not a huge difference. Still, my app calls control() for each available control inside an egui render loop. This might seem excessive, but it's simpler and safer to keep the driver as the single source of truth rather than caching anything. strace shows around 1000 ioctl / second before, and 600 after, and the code is slightly more explicit as @MarijnS95 points out.

donkeyteethUX commented 1 year ago

Bump, after addressing old comment & rebasing this.

MarijnS95 commented 1 year ago

@donkeyteethUX reverse bump: this is close to ready if you address the minor but remaining isuses.

donkeyteethUX commented 1 year ago

@donkeyteethUX reverse bump: this is close to ready if you address the minor but remaining isuses.

Thanks -- will-do when I get a chance. I'm swamped at work but haven't forgotten about it.

raymanfx commented 1 year ago

Looks good to me. Any objections?

MarijnS95 commented 1 year ago

Ping again, we still have two comments open and shouldn't conflate this PR with unrelated clippy changes.

MarijnS95 commented 1 year ago

@raymanfx do you mean the API added by this PR specifically, or the control API in general? If anything it seems this function is now more high-level by taking something typed instead of a generic i32.

raymanfx commented 1 year ago

I was talking about the API in general. This change is fine and I agree with its premises.