[libcamera-devel] Problems with getControls() and listControls()

[Kieran Bingham]

On 05/03/2021 15:23, David Plowman wrote:
> Hi again Kieran
> 
> On Fri, 5 Mar 2021 at 11:10, Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
>>
>> Hi David,
>>
>> On 05/03/2021 11:06, Kieran Bingham wrote:
>>> Hi David,
>>>
>>> On 05/03/2021 10:47, David Plowman wrote:
>>>> To fill in the details on some of this, the controls that can have
>>>> their ranges altered (looking at current drivers) are:
>>>>
>>>> Exposure: this range can change whenever the vblanking is updated, so
>>>> it can happen every frame. Luckily I don't think anyone cares about
>>>> the ranges here so everything seems to work as expected...
>>>>
>>>> The remaining ones normally get updated only when the camera mode is changed.
>>>>
>>>> Pixel Rate
>>>> Hblanking
>>>> Vblanking
>>>>
>>>> As far as I'm aware, only vblanking is a problem as we use those
>>>> limits to calculate possible frame timings. For the pixel rate and
>>>> hblanking we use the value, not the limits, so I believe we get the
>>>> right numbers back there.
>>>>
>>>> I'm inclined to agree with Kieran that watching for changes to values
>>>> probably won't work. A modification of the range (I take it there's no
>>>> event for that directly..?) doesn't necessarily cause a change in the
>>>> value. Pragmatically speaking, regenerating the ControlInfo list after
>>>> a setFormat doesn't sound like a terrible idea. Alternatively, and
>>>> 100% safe, perhaps we should give up caching max/min values and
>>>> re-fetch them every time they're wanted? Do we query these limits
>>>> often?
>>>
>>> I'd also be a bit weary of some sort of race between when the kernel
>>> updates us, to when we process the control event update, vs when some
>>> other code would read it.
>>
>> So I've just been told there is an event V4L2_EVENT_CTRL_CH_RANGE which
>> we can subscribe to, to know when a controls range is changed.
>>
>> So we can handle these updates 'automagically' ...
>>
>> The next question is ... is that the right way ;-) or will that be racy...
> 
> I think this is probably a question for the libcamera team. If it were
> solely down to me, I feel that events, callbacks and race conditions
> are starting to get just a little fussy for something that seemed like
> it should have been simple, and I would  be inclined just to query
> them every time I want them. But this is taking us away from the
> original idea of (cached lists of) ControlInfo objects, and I'm not
> really clear how much use other pipeline handlers are making of them,
> which leaves me hesitating...

Yup, I spoke with Laurent earlier, this thread is on his todo list - but
equally he doesn't think parsing the updates through events would be
right from my understanding.

So we'll have to make sure we update the value when we need to.

--
Kieran

> David
> 
>>
>> --
>> KB
>>
>>
>>>
>>> What about marking some controls as 'volatile' such that it must always
>>> get the full control info, (or whatever it needs), or would 'volatile'
>>> be too much?
>>>
>>> I don't know if it's something we need to update all values once per
>>> frame, or if we only need 'some' ?
>>>
>>>
>>> Otherwise we could have an explicit function to manually reload min/max
>>> values on a per-control basis ?
>>>  (if there's only 'one' that needs to be reloaded?)
>>>
>>>
>>> --
>>> Kieran
>>>
>>>
>>>> David
>>>>
>>>> On Fri, 5 Mar 2021 at 09:11, Naushir Patuck <naush at raspberrypi.com> wrote:
>>>>>
>>>>> Hi Kieran,
>>>>>
>>>>> On Thu, 4 Mar 2021 at 20:05, Kieran Bingham <kieran.bingham at ideasonboard.com> wrote:
>>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> On 03/03/2021 15:57, David Plowman wrote:
>>>>>>> Hi everyone
>>>>>>>
>>>>>>> I've been chasing down problems with a camera driver (sometimes) not
>>>>>>> running at the correct framerates. It seems that I've tracked it down
>>>>>>> to libcamera's control list mechanism. Please correct me if I've got
>>>>>>> anything wrong in my explanation below.
>>>>>>>
>>>>>>> V4L2 devices produce a list of all their controls, including min and
>>>>>>> max values, when they are opened - as the V4L2Device constructor calls
>>>>>>> the private listControls() method.
>>>>>>>
>>>>>>> The CameraInfo::sensorInfo() method calculates the minimum and maximum
>>>>>>> frame lengths using the min and max vblank values. These values were
>>>>>>> read and stored when the device was opened. Since then, however, the
>>>>>>> format requested from the sensor may well have changed which may mean
>>>>>>> these values are now wrong.
>>>>>>
>>>>>> This sounds troublesome indeed.
>>>>>>
>>>>>>> So firstly, have I understood this correctly? And secondly, what do
>>>>>>> you think would be the preferred way to fix it? Perhaps the
>>>>>>> listControls() method should be made "protected" and called at the end
>>>>>>> of V4L2Subdevice::setFormat? Or should getControls() always update the
>>>>>>> ranges..?
>>>>>>
>>>>>> I suspect the 'right' way to do this is to get the kernel to tell us if
>>>>>> the control has changed. V4L2 devices can expose events:
>>>>>>
>>>>>> https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-event.html
>>>>>>
>>>>>> We should be able to receive events for control changes through
>>>>>> V4L2_EVENT_CTRL:
>>>>>>
>>>>>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-dqevent.html#event-type
>>>>>>
>>>>>> We already have a means to subscribe to events in our V4L2Device and
>>>>>> V4L2VideoDevice classes, so handling control updates there should be a
>>>>>> case of extending from there.
>>>>>>
>>>>>> However, I have one concern on your reports above.
>>>>>>
>>>>>> It seems it's not the value that has changed, but the minimum and
>>>>>> maximums? It looks like the events are reported when the 'value'
>>>>>> changes, but I don't know if the min/max can be changed? I thought those
>>>>>> were initialised at control creation time. But perhaps I've just never
>>>>>> done it ;-)
>>>>>
>>>>>
>>>>> This is correct.  The VBLANK min/max limits change when we change
>>>>> sensor modes.  When we call V4L2Subdevice::getControl(VBLANK), it
>>>>> does return the correct "value", but the min/max remain unchanged.
>>>>> In the current codebase, the min/max values are only updated on a
>>>>> V4L2Device:listControl() call, which only happens when you open the device.
>>>>>
>>>>> Regards,
>>>>> Naush
>>>>>
>>>>>>
>>>>>>
>>>>>> Hope this helps (or at least bumps the discussions).
>>>>>>
>>>>>> Kieran
>>>>>>
>>>>>>
>>>>>>> Thanks!
>>>>>>> David
>>>>>>> _______________________________________________
>>>>>>> libcamera-devel mailing list
>>>>>>> libcamera-devel at lists.libcamera.org
>>>>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Regards
>>>>>> --
>>>>>> Kieran
>>>>>> _______________________________________________
>>>>>> libcamera-devel mailing list
>>>>>> libcamera-devel at lists.libcamera.org
>>>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran