| Message ID | 20200810120406.52654-2-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 10/08/2020 13:04, Umang Jain wrote: > Treat all UVC cameras as external for android framework to know that > they are external. > > Signed-off-by: Umang Jain <email@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. */ Probably don't need to say "Initialise Camera Property", but either way. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + properties_.set(properties::Location, properties::CameraLocationExternal); > + > /* Initialise the supported controls. */ > ControlInfoMap::Map ctrls; > >
Hello, On Tue, Aug 11, 2020 at 03:19:24PM +0100, Kieran Bingham wrote: > On 10/08/2020 13:04, Umang Jain wrote: > > Treat all UVC cameras as external for android framework to know that > > they are external. > > > > Signed-off-by: Umang Jain <email@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. */ > > Probably don't need to say "Initialise Camera Property", but either way. Best would be to add a todo item to explain what will need to change. /* * \todo Find a way to tell internal and external UVC cameras apart. * Until then, treat all UVC cameras as external. */ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + properties_.set(properties::Location, properties::CameraLocationExternal); > > + > > /* Initialise the supported controls. */ > > ControlInfoMap::Map ctrls; > >
Hi Umang, Thanks for your work. 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@ragnatech.se> > > Signed-off-by: Umang Jain <email@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; > > -- > 2.26.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Umang, On 13/08/2020 11:31, Niklas Söderlund wrote: > Hi Umang, > > Thanks for your work. > > 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@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 entirely sure yet, because I fear android expects the difference to be 'internal/external' rather than 'hotpluggable/not-hotpluggable'. What do you think ? -- Kieran >> >> Signed-off-by: Umang Jain <email@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; >> >> -- >> 2.26.2 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
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@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. > What do you think ? > > >> Signed-off-by: Umang Jain <email@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; > >>
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@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 ;-) -- Kieran > >> What do you think ? >> >>>> Signed-off-by: Umang Jain <email@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; >>>> >
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@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. > > -- > Kieran > > > > > >> What do you think ? > >> > >>>> Signed-off-by: Umang Jain <email@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
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@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@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 >
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;
Treat all UVC cameras as external for android framework to know that they are external. Signed-off-by: Umang Jain <email@uajain.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 ++++ 1 file changed, 4 insertions(+)