Extend picamera with more parameters #521
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #521 +/- ##
==========================================
+ Coverage 37.11% 37.18% +0.07%
==========================================
Files 30 30
Lines 5510 5548 +38
Branches 635 636 +1
==========================================
+ Hits 2045 2063 +18
- Misses 3317 3337 +20
Partials 148 148
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you for sending us the PR.
However, I think there are too many parameters being exposed in the config, and I am not sure what the eventual goal is. It might also be too confusing, to someone who does not know what they are doing.
My main problem with this change is that it exposes a lot of implementation detail via config, so if ever we moved away from our current camera driver, we would have to break backward compatibility which would be extremely unfortunate.
I will let in the other core contributors chime in for their opinion. I think in general, changes like these are better a part of your fork, so you can point people to it if they would like to know how that works.
The list of parameters is indeed long but I tried to keep the variables in config prefixed with PICAMERA_ so that it is easily distinguishable from other cameras.
4.There are already parameters in the configs that are not usable for everyone (just take example of parameters for different cameras). Maybe it is time to think about refactoring the config itself? |
|
I agree it’s useful to have these options. My disagreement is about config bloat. I was actually not thrilled about some of the other configuration options that you mention before. And I think we might want to undo those changes as well. I think making everything a configuration has diminishing returns because at this point the amount of config for the camera actually exceeds the lines of code for the camera part. I would much rather advanced users forked the repo to add their camera optimizations. |
|
Would breaking this out into a separate config file for camera settings make sense? I know that can cause other issues down the line, but at least it could be retired as we move to new cameras etc. |
|
I like that the camera part has all the options available. I'm not 100% confident those defaults are the ones we want. Have you looked at the driver code to see what the defaults are? |
|
Yes, I've taken defaults from the python PiCamera library. |
There was a problem hiding this comment.
I think this change is good but should be a little bit re-factored, addressing previous comments. I believe, having access to the camera parameters can be quite useful, maybe we can improve the camera settings to make donkey's eyesight better - I experience a significant impact from light conditions in the inferencing performance. Maybe some settings here can help to create more uniform pictures? Would be great if folks could play around with that.
| @@ -11,7 +11,7 @@ def run_threaded(self): | |||
| return self.frame | |||
|
|
|||
| class PiCamera(BaseCamera): | |||
| def __init__(self, image_w=160, image_h=120, image_d=3, framerate=20, vflip=False, hflip=False): | |||
| def __init__(self, image_w=160, image_h=120, image_d=3, framerate=20, vflip=False, hflip=False, awb_mode='auto', awb_gains=(1.0, 1.0), brightness=50, color_effects=None, contrast=0, exposure_compensation=0, exposure_mode='auto', image_denoise=True, image_effect='none', image_effect_params=None, iso=0, meter_mode='average', rotation=0, saturation=0, sharpness=0, video_denoise=True, video_stabilization=False, zoom=(0.0, 0.0, 1.0, 1.0)): | |||
There was a problem hiding this comment.
I think all the additional parameters should be passed by dictionary (i.e. **kwargs) here, otherwise the interface is overloaded.
| @@ -22,6 +22,32 @@ def __init__(self, image_w=160, image_h=120, image_d=3, framerate=20, vflip=Fals | |||
| self.camera.framerate = framerate | |||
| self.camera.vflip = vflip | |||
| self.camera.hflip = hflip | |||
| self.camera.awb_mode = awb_mode | |||
There was a problem hiding this comment.
Then here, you would iterate through all the possible parameters of the camera using get() and providing a default values. Hence only required overwrites would need to be in the dictionary.
| @@ -32,6 +32,25 @@ | |||
| CAMERA_FRAMERATE = DRIVE_LOOP_HZ | |||
| CAMERA_VFLIP = False | |||
| CAMERA_HFLIP = False | |||
| # PI Camera specitic options, for more details see https://picamera.readthedocs.io/en/release-1.13/api_camera.html | |||
There was a problem hiding this comment.
And here, instead of passing all camera parameters, you would add a single dictionary as camera parameters. This should be empty, here and then different users can overwrite this to their needs. This should also address @tikurahul comment.
| @@ -98,7 +98,7 @@ def drive(cfg, model_path=None, use_joystick=False, model_type=None, camera_type | |||
| inputs = ['angle', 'throttle'] | |||
| elif cfg.CAMERA_TYPE == "PICAM": | |||
| from donkeycar.parts.camera import PiCamera | |||
| cam = PiCamera(image_w=cfg.IMAGE_W, image_h=cfg.IMAGE_H, image_d=cfg.IMAGE_DEPTH, framerate=cfg.CAMERA_FRAMERATE, vflip=cfg.CAMERA_VFLIP, hflip=cfg.CAMERA_HFLIP) | |||
| cam = PiCamera(image_w=cfg.IMAGE_W, image_h=cfg.IMAGE_H, image_d=cfg.IMAGE_DEPTH, framerate=cfg.CAMERA_FRAMERATE, vflip=cfg.CAMERA_VFLIP, hflip=cfg.CAMERA_HFLIP, awb_mode=cfg.PICAMERA_AWB_MODE, awb_gains=cfg.PICAMERA_AWB_GAINS, brightness=cfg.PICAMERA_BRIGHTNESS, color_effects=cfg.PICAMERA_COLOR_EFFECTS, contrast=cfg.PICAMERA_CONTRAST, exposure_compensation=cfg.PICAMERA_EXPOSURE_COMPENSATION, exposure_mode=cfg.PICAMERA_EXPOSURE_MODE, image_denoise=cfg.PICAMERA_IMAGE_DENOISE, image_effect=cfg.PICAMERA_IMAGE_EFFECT, image_effect_params=cfg.PICAMERA_IMAGE_EFFECT_PARAMS, iso=cfg.PICAMERA_ISO, meter_mode=cfg.PICAMERA_METER_MODE, rotation=cfg.PICAMERA_ROTATION, saturation=cfg.PICAMERA_SATURATION, sharpness=cfg.PICAMERA_SHARPNESS, video_denoise=cfg.PICAMERA_VIDEO_DENOISE, video_stabilization=cfg.PICAMERA_VIDEO_STABILIZATION, zoom=cfg.PICAMERA_ZOOM) | |||
There was a problem hiding this comment.
Then here you would just add cfg.CAMERA_ARGS - or whatever you want to call it.
There was a problem hiding this comment.
Thinking about var naming:
CAMERA_ARGSone could be used by any camera (but then it would be on user side to keep compatible parameters)PICAMERA_ARGSwould expliclty control picamera parameters, based on the module version. But this inroduces more config options bloat.
Moreover:
- should the args contain the full set of args, such as image_w, image_h options or just extra args (which means image_w, image_h, framerate and some other would be added 'by default'?
There was a problem hiding this comment.
I would opt for PICAMERA_ARGS and keep the current image_w, etc parameters to maintain backward compatibility.
I needed to adjust donkey to work with NoIR camera, so I extended picamera class with extra options.
I suggest experimenting with PICAMERA_IMAGE_EFFECT='emboss' or PICAMERA_ROTATION=180 :)