[libcamera-devel] [PATCH v2 1/4] libcamera: pipeline: uvcvideo: Treat all UVC cameras as external

[Kieran Bingham]

Hi Niklas,

On 13/08/2020 20:56, Niklas Söderlund wrote:
> Hi Kieran,
> 
> On 2020-08-13 20:19:13 +0100, Kieran Bingham wrote:
>>
>>
>> On 13/08/2020 20:13, Laurent Pinchart wrote:
>>> Hi Kieran,
>>>
>>> On Thu, Aug 13, 2020 at 08:11:18PM +0100, Kieran Bingham wrote:
>>>> On 13/08/2020 11:31, Niklas Söderlund wrote:
>>>>> On 2020-08-10 12:04:11 +0000, Umang Jain wrote:
>>>>>> Treat all UVC cameras as external for android framework to know that
>>>>>> they are external.
>>>>>
>>>>> nit: I would rewrite this to describe that as we currently have no way 
>>>>> to determine the location if a UVC camera mark it External to have a 
>>>>> starting point for the property.
>>>>>
>>>>> With or without this fixed but with the comments by Kieran and Laurent 
>>>>> fixed,
>>>>>
>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>>>
>>>> So, having seen the conflict this might cause with camera naming, I
>>>> wonder if we should introduce a new property called "hotpluggable" or
>>>> "removable" or such in a separate patch, and then set that property here.
>>>
>>> I'm not sure to follow you, what's the conflict ?
>>>
>>>> I'm not entirely sure yet, because I fear android expects the difference
>>>> to be 'internal/external' rather than 'hotpluggable/not-hotpluggable'.
>>>
>>> Android expects all internal cameras to not be hotpluggable, and all
>>> external cameras to be hotpluggable.
>>
>> You're right, it's not a 'conflict' as such, just very ambiguous naming
>> in cam: - this is the output I get with this change, and Niklas' naming
>> series:
>>
>> ./build/src/cam/cam -l
>>
>> Available cameras:
>> 1: External camera (\_SB_.PCI0.XHC_.RHUB.HS01-1:1.0-041e:406c)
>> 2: External camera (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251)
>> 3: External camera (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.2-0408:5251)
>>
>> However this is just the output rendered by cam, which is application
>> side, so I'm conflating the issue.
>>
>> Perhaps it really means I think cam should also always display the model
>> when available ;-)
> 
> I agree with you, camera names printed with both these series are 
> unsuitable for UVC cameras that are hot-pluggable. I think the name for 
> UVC cameras that are hot-pluggable should be the model name.
> 
> As the exact requirements for how the HAL should select if a camera is 
> hot-pluggable are still under debate I think we should merge the series 
> as soon as they become ready. The fix for the name generation rule 
> change introduce in this series is small and easy to squash in or fix 
> on-top, depending on the order the series are ready.

Indeed, I think the camera name improvement series is good to go
personally, and is an improvement on what we have right now.

We can reconvene when we work out how this series goes in, or if you
want to predict it and display the model for external camera's that's
fine for me too.

I think that's a good distinction though. For an internal camera, the
location is more important than the model which is not obvious, or clear
to the user.

For external cameras, that's different because the location is less
meaningful, and the model is more meaningful (and visible).


>> --
>> Kieran
>>
>>
>>>
>>>> What do you think ?
>>>>
>>>>>> Signed-off-by: Umang Jain <email at uajain.com>
>>>>>> ---
>>>>>>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>>>> index bc892ec..ed61d12 100644
>>>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>>>> @@ -14,6 +14,7 @@
>>>>>>  #include <libcamera/camera.h>
>>>>>>  #include <libcamera/control_ids.h>
>>>>>>  #include <libcamera/controls.h>
>>>>>> +#include <libcamera/property_ids.h>
>>>>>>  #include <libcamera/request.h>
>>>>>>  #include <libcamera/stream.h>
>>>>>>  
>>>>>> @@ -500,6 +501,9 @@ int UVCCameraData::init(MediaEntity *entity)
>>>>>>  
>>>>>>  	video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
>>>>>>  
>>>>>> +	/* Initialise Camera Property. Treat all UVC cameras as external. */
>>>>>> +	properties_.set(properties::Location, properties::CameraLocationExternal);
>>>>>> +
>>>>>>  	/* Initialise the supported controls. */
>>>>>>  	ControlInfoMap::Map ctrls;
>>>>>>  
>>>
>>
>> -- 
>> Regards
>> --
>> Kieran
> 

-- 
Regards
--
Kieran