search

lit-robotics / libcamera-rs

Experimental Rust bindings for libcamera
Apache License 2.0
46 stars 15 forks source link

Add support for video recording #5

Closed mfreeborn closed 1 year ago

mfreeborn commented 1 year ago

Closes #4

I've included a new example which just records 20 frames of MJPEG video which doubles as an integration test. That example could be padded out by adding an option for number of frames capture, etc.

chemicstry commented 1 year ago

Thank you for the PR!

Everything looks great, except that enum is not really fit for representing flags (i.e. you can't concatenante two different flags). I know that libcamera currently has only two values for ReuseFlag and this works for now, but it would be a breaking API change to add more flags in the future.

Could you implement this using bitflags crate? I'm open to suggestions if you think something else would be better.

mfreeborn commented 1 year ago

I've reimplemented the enum using bitflags - agreed it's the correct way forwards. Couple of review questions:

1) I pass the rust bitflags to the C api just by casting them to u32 using bitflags::bits(). This compiles fine, but it now seems redundant duplicating the flags in the C api, as the C api type is only being used as a fancy wrapper around an integer. Is there a better way to implement it than the way I've done?

2) I've gone with SCREAMING_SNAKE_CASE for the flag variants as they are const values. I wonder if PascalCase would read better or if it's best to stick with what we've got.

chemicstry commented 1 year ago

I've reimplemented the enum using bitflags - agreed it's the correct way forwards. Couple of review questions:

  1. I pass the rust bitflags to the C api just by casting them to u32 using bitflags::bits(). This compiles fine, but it now seems redundant duplicating the flags in the C api, as the C api type is only being used as a fancy wrapper around an integer. Is there a better way to implement it than the way I've done?

I think it's OK. C wrapper already has some redudant enums that are not used. The initial plan was to make C wrapper an official C API for libcamera, but it's too much work in terms of API coverage and documentation. Maybe we can replace it with cxx one day, like it was done in https://github.com/dennisss/libcamera-rs. Unless, libcamera gets an official C API we could use.

  1. I've gone with SCREAMING_SNAKE_CASE for the flag variants as they are const values. I wonder if PascalCase would read better or if it's best to stick with what we've got

PascalCase would require warning exceptions, let's just stick to the standard