Re: [PATCH RFC v3 2/6] v4l2-ctrl: Add helper function for control range update

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sylwester,

On Thursday 14 March 2013 12:39:34 Sylwester Nawrocki wrote:
> On 03/14/2013 08:10 AM, Hans Verkuil wrote:
> > On Tue March 12 2013 07:56:25 Hans Verkuil wrote:
> >> On Wed January 23 2013 23:21:57 Sylwester Nawrocki wrote:
> >>> This patch adds a helper function that allows to modify range,
> >>> i.e. minimum, maximum, step and default value of a v4l2 control,
> >>> after the control has been created and initialized. This is helpful
> >>> in situations when range of a control depends on user configurable
> >>> parameters, e.g. camera sensor absolute exposure time depending on
> >>> an output image resolution and frame rate.
> >>> 
> >>> v4l2_ctrl_modify_range() function allows to modify range of an
> >>> INTEGER, BOOL, MENU, INTEGER_MENU and BITMASK type controls.
> >>> 
> >>> Based on a patch from Hans Verkuil
> >>> http://patchwork.linuxtv.org/patch/8654.
> >>> 
> >>> Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> >>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> 
> >> I've been playing around with this a bit, using this vivi patch:
> >> 
> >> diff --git a/drivers/media/platform/vivi.c
> >> b/drivers/media/platform/vivi.c
> >> index c46d2e8..85bc314 100644
> >> --- a/drivers/media/platform/vivi.c
> >> +++ b/drivers/media/platform/vivi.c
> >> @@ -1093,6 +1093,15 @@ static int vidioc_s_input(struct file *file, void
> >> *priv, unsigned int i)>> 
> >>  		return 0;
> >>  	
> >>  	dev->input = i;
> >> 
> >> +	/*
> >> +	 * Modify the brightness range depending on the input.
> >> +	 * This makes it easy to use vivi to test if applications can
> >> +	 * handle control range modifications and is also how this is
> >> +	 * typically used in practice as different inputs may be hooked
> >> +	 * up to different receivers with different control ranges.
> >> +	 */
> >> +	v4l2_ctrl_modify_range(dev->brightness,
> >> +			128 * i, 255 + 128 * i, 1, 127 + 128 * i);
> >> 
> >>  	precalculate_bars(dev);
> >>  	precalculate_line(dev);
> >>  	return 0;
> >> 
> >> And it made me wonder if it wouldn't be more sensible if modify_range
> >> would also update the current value to the new default value?
> > 
> > Actually, thinking about it some more, I believe that modify_range should
> > actually include the new control value as argument. That way the caller
> > can decide what to do: use the current value (which then might be
> > clamped), use the default_value or use a remembered previous value.
> > 
> > If you agree with this, then I'll make a patch for it. I just need to know
> > what the only user of this call (ov9650.c) should do. I suspect it should
> > use the default value as the new value, but I'm not certain.
> 
> Sorry for the delay. I suppose if we choose either clamping the new value
> or setting it to the default value there will always be users that wanted
> other behaviour than currently implemented. In ov9650 case it seemed
> clamping the value of the exposure control to <min, max> when the image
> size changed a best option. Using the default value each time control's
> range changed would cause changes of the exposure time, even though current
> exposure value would still be inside of new range.
> Thus I think best option for ov9650 would be to use previous value of the
> control, which would then be clamped to <min, max>. This way changes of
> the exposure time caused by the output format change could be minimized.

I agree. If we don't make the new value configurable I would prefer clamping 
the current value. Adding an argument to the function is reasonable, but I 
don't know if we will have use cases for that. Maybe we can clamp the current 
value for now and add the argument if drivers need it in the future.

-- 
Regards,

Laurent Pinchart