| Message ID | 20200511100150.5205-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patches. On Mon, May 11, 2020 at 11:01:48AM +0100, Naushir Patuck wrote: > Hi, > > The following two patches add variable framerate control to a sensor > subdevice. This will allow shutter speeds below 33ms during > viewfinder (for example). > > I've currently hardcoded the maximum limit at 30.0fps, but we ought to > have a discussion on how a user can request fps limits - perhaps > though a Request, or stream configuration parameters? That's an API we need indeed. I think the frame rate would need to be modifiable at runtime, so that would require a control. Should we go for a frame duration, using the same units as the exposure time ? > Naushir Patuck (2): > libcamera: raspberrypi: Add control of sensor vblanking > libcamera: raspberrypi: Limit the maximum framerate to 30.0fps > > src/ipa/raspberrypi/cam_helper.hpp | 1 + > src/ipa/raspberrypi/cam_helper_imx219.cpp | 23 +++++++++++++++++++ > src/ipa/raspberrypi/cam_helper_imx477.cpp | 23 +++++++++++++++++++ > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 23 +++++++++++++++++++ > src/ipa/raspberrypi/raspberrypi.cpp | 14 ++++++++++- > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > 6 files changed, 85 insertions(+), 1 deletion(-)
Hi Laurent, On Tue, 12 May 2020 at 01:28, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patches. > > On Mon, May 11, 2020 at 11:01:48AM +0100, Naushir Patuck wrote: > > Hi, > > > > The following two patches add variable framerate control to a sensor > > subdevice. This will allow shutter speeds below 33ms during > > viewfinder (for example). > > > > I've currently hardcoded the maximum limit at 30.0fps, but we ought to > > have a discussion on how a user can request fps limits - perhaps > > though a Request, or stream configuration parameters? > > That's an API we need indeed. I think the frame rate would need to be > modifiable at runtime, so that would require a control. Should we go for > a frame duration, using the same units as the exposure time ? I can prepare an array control to provide min/max framerate and send out a v2 patch with this. Normally I prefer representing framerate as 1/t, e.g. 30 fps. No objections to using the same form (t) and units as exposure - it's your call. Thanks, Naush > > > Naushir Patuck (2): > > libcamera: raspberrypi: Add control of sensor vblanking > > libcamera: raspberrypi: Limit the maximum framerate to 30.0fps > > > > src/ipa/raspberrypi/cam_helper.hpp | 1 + > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 23 +++++++++++++++++++ > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 23 +++++++++++++++++++ > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 23 +++++++++++++++++++ > > src/ipa/raspberrypi/raspberrypi.cpp | 14 ++++++++++- > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > > 6 files changed, 85 insertions(+), 1 deletion(-) > > -- > Regards, > > Laurent Pinchart
Hi Naush, On Tue, May 12, 2020 at 11:22:39AM +0100, Naushir Patuck wrote: > On Tue, 12 May 2020 at 01:28, Laurent Pinchart wrote: > > On Mon, May 11, 2020 at 11:01:48AM +0100, Naushir Patuck wrote: > > > Hi, > > > > > > The following two patches add variable framerate control to a sensor > > > subdevice. This will allow shutter speeds below 33ms during > > > viewfinder (for example). > > > > > > I've currently hardcoded the maximum limit at 30.0fps, but we ought to > > > have a discussion on how a user can request fps limits - perhaps > > > though a Request, or stream configuration parameters? > > > > That's an API we need indeed. I think the frame rate would need to be > > modifiable at runtime, so that would require a control. Should we go for > > a frame duration, using the same units as the exposure time ? > > I can prepare an array control to provide min/max framerate and send > out a v2 patch with this. Normally I prefer representing framerate as > 1/t, e.g. 30 fps. No objections to using the same form (t) and units > as exposure - it's your call. I've used both, and they have pros and cons. FPS is generally more user-friendly, but I've also called for deprecating the V4L2 FPS-based subdev API for camera sensor in the most common cases, and frame duration is controlled there through the total h/v sizes. One issue with fps is that we'll have to deal with rounding issues, with different pipeline handlers ad IPAs doing so slightly differently. I've had to deal with this in the uvcvideo driver, as UVC is based on frame durations (as multiples of 100ns) and V4L2 on frame rates. 30fps is thus expressed as 333333, which I initially translated to 333333/10000000. That's not user-friendly, so I ended up with the following code (which I'm shamefully proud of): /* Simplify a fraction using a simple continued fraction decomposition. The * idea here is to convert fractions such as 333333/10000000 to 1/30 using * 32 bit arithmetic only. The algorithm is not perfect and relies upon two * arbitrary parameters to remove non-significative terms from the simple * continued fraction decomposition. Using 8 and 333 for n_terms and threshold * respectively seems to give nice results. */ void uvc_simplify_fraction(u32 *numerator, u32 *denominator, unsigned int n_terms, unsigned int threshold) { u32 *an; u32 x, y, r; unsigned int i, n; an = kmalloc_array(n_terms, sizeof(*an), GFP_KERNEL); if (an == NULL) return; /* Convert the fraction to a simple continued fraction. See * http://mathforum.org/dr.math/faq/faq.fractions.html * Stop if the current term is bigger than or equal to the given * threshold. */ x = *numerator; y = *denominator; for (n = 0; n < n_terms && y != 0; ++n) { an[n] = x / y; if (an[n] >= threshold) { if (n < 2) n++; break; } r = x - an[n] * y; x = y; y = r; } /* Expand the simple continued fraction back to an integer fraction. */ x = 0; y = 1; for (i = n; i > 0; --i) { r = y; y = an[i-1] * y + x; x = r; } *numerator = y; *denominator = x; kfree(an); } The function is called with 8 and 333 for n_terms and threshold, and manages to simplify 333333/10000000 to 1/30 while not simplifying 1001/60000 to 1/60 (and while conducting a few tests out of curiosity a few minutes ago, I realized the code simplifies 1001/30000 to 367/10999 :-S). We could also argue that a frame duration doesn't allow expressing NTSC frame rates exactly, but given that libcamera deals primarily with camera sensors, all this makes me favour frame durations instead of frame rates. Of course, I may be overlooking something, if you think we could come up with nice helpers to ensure that the conversions are handled nicely and consistently by pipeline handlers and IPAs, I would certainly be happy to discuss that. Frame durations would have the nice benefit of being aligned with the Android camera HAL API, but that's not a requirement by itself. > > > Naushir Patuck (2): > > > libcamera: raspberrypi: Add control of sensor vblanking > > > libcamera: raspberrypi: Limit the maximum framerate to 30.0fps > > > > > > src/ipa/raspberrypi/cam_helper.hpp | 1 + > > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 23 +++++++++++++++++++ > > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 23 +++++++++++++++++++ > > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 23 +++++++++++++++++++ > > > src/ipa/raspberrypi/raspberrypi.cpp | 14 ++++++++++- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > > > 6 files changed, 85 insertions(+), 1 deletion(-)