TinkerBoard / debian_kernel

Debian Kernel source for Tinker Board
Other
143 stars 64 forks source link

[imx219] Fix: set exposure and gain before start #32

Closed cristiklein closed 5 years ago

cristiklein commented 5 years ago

This patch makes the IMX219 sensor more usable with manual gain and exposure time. It ensures that both parameters are set according to the user's last selection before starting the video stream. Previously, the exposure time was reset to a default value before starting the video stream and the user had to set exposure after the first frame arrived.

jamess-huang commented 5 years ago

Dear, Cristiklein Thanks for your commit. This commit will be useful under the manual AE mode. However, our currently code base still cannot support the manual mode, or we should say we do not provide a good method for user do the manual AE on the user space. I am afraid the exposure setting will be unpredictable if we remove the default value and user does not set the manual AE. So I would not like to merge this commit, thanks for the understanding.

cristiklein commented 5 years ago

Hi @jamess-huang ,

Thank you for looking into my commit.

I am not sure to fully understand your concern. The default value for analog gain, digital gain and exposure are already set to 512, 256 and 1575, respectively, from here: https://github.com/TinkerBoard/debian_kernel/blob/develop/drivers/media/i2c/imx219.c#L956.

I double-checked on a freshly booted Tinker Board with the present commit, and here is the relevant output:

$ v4l2-ctl --device /dev/video1 --list-ctrls

User Controls

                       exposure (int)    : min=0 max=4095 step=1 default=1575 value=1575
                           gain (int)    : min=256 max=43663 step=1 default=256 value=256
                horizontal_flip (bool)   : default=0 value=0
                  vertical_flip (bool)   : default=0 value=0

Image Source Controls

              vertical_blanking (int)    : min=36 max=36 step=1 default=36 value=36
            horizontal_blanking (int)    : min=164 max=164 step=1 default=164 value=164
                  analogue_gain (int)    : min=256 max=2816 step=1 default=512 value=512

Image Processing Controls

                 link_frequency (intmenu): min=0 max=0 default=0 value=0
                     pixel_rate (int64)  : min=180810000 max=180810000 step=1 default=180810000 value=180810000 flags=read-only
                   test_pattern (menu)   : min=0 max=13 default=0 value=0

The commit simply ensures that, if the user did set exposure and gain manually, those values are preserved when the camera starts streaming.

Also note that, due to the way V4L2 subsystem works, the current experience for users who want manual exposure is really bad. They have to do something like:

./start-streaming
sleep 1 # just to make sure streaming started
v4l2-ctl --device /dev/video1 --set-ctrl=exposure=1100
./stop-streaming

./start-streaming
sleep 1 # just to make sure streaming started
v4l2-ctl --device /dev/video1 --set-ctrl=exposure=1101 # set a bogus value first, since the V4L2 subsystem will believe that the exposure did not change, hence no need to notify the camera
v4l2-ctl --device /dev/video1 --set-ctrl=exposure=1100 # now the user can set the desired value for exposure

With this commit, they only have to do:

v4l2-ctl --device /dev/video1 --set-ctrl=exposure=1100
./start-streaming
...
./stop-streaming

./start-streaming
...
./stop-streaming

Hence, the experience should be the same for users that do not set exposure manually (or that set exposure using isp-mode), but improves for users that want manual exposure.

Could you clarify what are your concerns with respect to this commit?

jamess-huang commented 5 years ago

understood and thanks.