Refactor the CameraService #489
Conversation
This is a refactor of the CameraService. It fixes many issues and hopefully adds few. This should decrease CPU usage and increase reliability.
|
Hmm, at some point during test print on my Galaxy S5 the camera stopped responding, so there are still some issues lurking. |
|
This overall seems to be working except... Known issues:
|
|
This is an amazing contribution, thanks a lot for your work! I'll review and test it out this week |
I'm not convinced if my camera is even autofocusing or if it's just being shaken around by the printer... but this is the closest I've been able to get to 'fixed focus'
Clearing the Preview surface provider before unbinding the use case seems to fix most of the issues.
|
This might be about as good as it gets for now. I kind of suspect some of the remaining issues may just be CameraX 1.0.0.
I've given it my best shot to get it working properly, but it acts differently between devices and only affects the preview window, so it may just be what it is.
I'm interested to hear how it runs for you. It's definitely not perfect, but I think it's an improvement. |
Setting the state triggers a callback which uses the cameraControl, so should probably set it first.
|
Ugh, test print kept putting camera in error state. |
I don't know why. New versions of CameraX it does nothing, old versions of CameraX it closes the image, which is manually done immediately after the call anyway. But it seems to be stopping my camera from going into an error state by calling it...
cobryan05
force-pushed
the
cameraRefactor
branch
from
217d742 to
cb58983
Compare
|
It's actually is working well right now, just from re-adding that super.onImageCaptureSuccess ... I have no idea why. Even looking through the CameraX source history I can't find any period of time in which it should matter... (Camera stopped responding an hour into the print.... I think I might just periodically dump all camera use cases so none of the can get 'stuck on') |
I really hate this thing. It works great on my cheap, but new, BluView4 phone, but I *will* get this stupid thing to work on my Galaxy S5. The camera on my Galaxy S5 will crash and require a full phone reboot if an 'enableTorch' call is called while an imageCapture with flashmode on attempts to take a picture... so now I'm manually turning on the flash for ImageCapture. This can sometimes mess with image focus or exposure, as when FlashMode was set on the ImageCapture object the phone would go through its AutoExposure/AutoFocus routine before snapping a picture. Turning the flash on for a bit before the capture call seems to help for some reason but isn't perfect. I don't know why it helps at all because the camera shouldn't be 'seeing' anything different as long as the flash is on before the camera, but it seems to help. I found that when ImageCapture has flash mode on that calling enableTorch on the CameraControl returned from binding ImageCapture would cause the ImageCapture to take a snapshot (this is what was crashing my camera: calling enableTorch(false) would *trigger* ImageCapture to take a picture, which would try to control the flash and crash). So now when a use case references the flash it passes along its CameraControl (instead of grabbing the first non-null CameraControl in the list). With the change to make ImageCapture use manual flash this shouldn't really have any affect.
|
At this point I am convinced that my Galaxy S5 simply doesn't support disabling autofocus. Seems like my camera crashes were caused by an ImageCapture use case attempting to auto-flash while a manual use case called enableFlash(). Doesn't seem to affect the newer phone, so maybe its a CameraX issue. ImageCapture seems to trigger itself when enableFlash() is called (on its CameraControl? On any CameraControl? Not sure if it matters), and since enableFlash at the same time as a trigger crashes my phone's camera this didn't go well. I now manually turn on the camera flash before calling snapshot. ImageCapture still sometimes gets triggered when disabling the flash (probably if the ImageCapture was the last to unreference the flash). Sometimes when this happens the flash will turn off, even if you have a live stream going. In this case to get the flash back you'll need to either: 1. Wait for the next snapshot to re-enable the flash and hope it doesn't turn it off, or 2. Close all streams and re-open them. I made it through an entire test print, continually stopping and starting live streams and snapshots, and it successfully complete with all timelapse images successful. If this continues to work through the weekend I am going to call this "good enough to submit." |
Previously enableTorch(true) would only be called on the first refcnt. However, sometimes ImageCapture will decide to turn off the flash, which would require all streams to close to get refcnt down to 0 so that it could go back up to 1 again to turn back on. By always calling enableTorch, even when it should already be on, it will allow a new use case to turn the torch back on (although it must be a new use case, eg a snapshot. Just opening up new streams without closing existing streams will just share the same use case and the flash will still be off...)
|
Ok, this ran through the weekend well. Ran a few test prints and it wasn't perfect, but no major issues. I never had any significant issue being able to remotely monitor the stream with the flash, and in the few spot checks I did to visit the printer in person the flash was never seen stuck on. The issues:
I'm going to mark this as 'ready to submit' with the known warts above. The focus/exposure makes my timelapses not-pretty. If people were getting pretty timelapses before this may be a regression for them. However this issue may entirely be my janky old phone as well. My primary use case of being able to reliably remotely monitor the printer is accomplished with this patch. I will probably try to address the other issues as they annoy me, but this is currently "good enough for me for now". If you test this and it seems like an overall regression you can just leave it open and I may eventually improve it :-P Otherwise if it seems like an improvement then this seems ready for review |
|
Tested it out in an emulator and a Google Pixel XL phone - works well in both cases, I can see quite a bit of performance improvements from the previous version. The autofocus issues were very much present in the earlier codebase too - they're quite specific to a given model phone / camera FW. I'll try to do some more testing of the autofocus logic - but so far, this is a massive improvement over the previous code, and I really appreciate the time spent on this refactor :) |
|
Glad to hear the autofocus isn't just me. I'd like for my timelapses to come out pretty, but that's in the 'cosmetic' bucket of my priority list. I'm glad this isn't causing a regression for people with other priorities. Thanks for all of the work you put into putting octo4a together. Phones seem like the ideal platform for octoprint, with builtin USB, camera and flash, and its nice to be able to give my Galaxy S5 another lease on life |
|
Changes moved to #496. Thanks a lot for your contribution, it will make a huge difference to people using their phone's cameras :) |
A "small" change to refactor of the CameraService. It fixes many issues and hopefully adds very few.
This should decrease CPU usage and increase reliability.
I haven't yet ran this hooked up to the printer, but everything seems to be working well standalone.