| Message ID | 20200929164000.15429-3-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, On 29/09/2020 17:39, David Plowman wrote: > Add a default initialisation according to the sensor resolution, > though it will need updating when the camera mode changes. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/camera_sensor.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index d2679a4b..c9a19b02 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -277,6 +277,12 @@ int CameraSensor::init() > */ > resolution_ = sizes_.back(); > > + /* > + * Set a default value for the SensorOutputSize, though it will have to > + * be updated when new camera modes are chosen. > + */ > + properties_.set(properties::SensorOutputSize, resolution_); I guess here, this is just an initialisation, but doesn't reflect what's actually set on the sensor yet. Should we also set this during setFormat() ? > + > return 0; > } > >
On 30/09/2020 09:32, Jacopo Mondi wrote: > Hi Kieran, > > On Tue, Sep 29, 2020 at 08:53:55PM +0100, Kieran Bingham wrote: >> Hi David, >> >> On 29/09/2020 17:39, David Plowman wrote: >>> Add a default initialisation according to the sensor resolution, >>> though it will need updating when the camera mode changes. >>> >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> src/libcamera/camera_sensor.cpp | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >>> index d2679a4b..c9a19b02 100644 >>> --- a/src/libcamera/camera_sensor.cpp >>> +++ b/src/libcamera/camera_sensor.cpp >>> @@ -277,6 +277,12 @@ int CameraSensor::init() >>> */ >>> resolution_ = sizes_.back(); >>> >>> + /* >>> + * Set a default value for the SensorOutputSize, though it will have to >>> + * be updated when new camera modes are chosen. >>> + */ >>> + properties_.set(properties::SensorOutputSize, resolution_); >> >> I guess here, this is just an initialisation, but doesn't reflect what's >> actually set on the sensor yet. >> >> Should we also set this during setFormat() ? > > Before the Camera is configured the value of the property is not > relevant. I think the property documentation reports that. My point/question was - is the 'real' sensor output size modified (or potentially modified) when ever a call is made through setFormat, and thus - should the SensorOutputSize be updated to reflect that. Or is SensorOutputSize a fixed immutable thing that never changes? Maybe that's my real question. *when* and *where* does this get updated when things change. -- Kieran > >> >>> + >>> return 0; >>> } >>> >>> >> >> -- >> Regards >> -- >> Kieran >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Tue, Sep 29, 2020 at 08:53:55PM +0100, Kieran Bingham wrote: > Hi David, > > On 29/09/2020 17:39, David Plowman wrote: > > Add a default initialisation according to the sensor resolution, > > though it will need updating when the camera mode changes. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/camera_sensor.cpp | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index d2679a4b..c9a19b02 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -277,6 +277,12 @@ int CameraSensor::init() > > */ > > resolution_ = sizes_.back(); > > > > + /* > > + * Set a default value for the SensorOutputSize, though it will have to > > + * be updated when new camera modes are chosen. > > + */ > > + properties_.set(properties::SensorOutputSize, resolution_); > > I guess here, this is just an initialisation, but doesn't reflect what's > actually set on the sensor yet. > > Should we also set this during setFormat() ? Before the Camera is configured the value of the property is not relevant. I think the property documentation reports that. > > > + > > return 0; > > } > > > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Wed, Sep 30, 2020 at 09:31:22AM +0100, Kieran Bingham wrote: > > > On 30/09/2020 09:32, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Tue, Sep 29, 2020 at 08:53:55PM +0100, Kieran Bingham wrote: > >> Hi David, > >> > >> On 29/09/2020 17:39, David Plowman wrote: > >>> Add a default initialisation according to the sensor resolution, > >>> though it will need updating when the camera mode changes. > >>> > >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> src/libcamera/camera_sensor.cpp | 6 ++++++ > >>> 1 file changed, 6 insertions(+) > >>> > >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > >>> index d2679a4b..c9a19b02 100644 > >>> --- a/src/libcamera/camera_sensor.cpp > >>> +++ b/src/libcamera/camera_sensor.cpp > >>> @@ -277,6 +277,12 @@ int CameraSensor::init() > >>> */ > >>> resolution_ = sizes_.back(); > >>> > >>> + /* > >>> + * Set a default value for the SensorOutputSize, though it will have to > >>> + * be updated when new camera modes are chosen. > >>> + */ > >>> + properties_.set(properties::SensorOutputSize, resolution_); > >> > >> I guess here, this is just an initialisation, but doesn't reflect what's > >> actually set on the sensor yet. > >> > >> Should we also set this during setFormat() ? > > > > Before the Camera is configured the value of the property is not > > relevant. I think the property documentation reports that. > > My point/question was - is the 'real' sensor output size modified (or > potentially modified) when ever a call is made through setFormat, and > thus - should the SensorOutputSize be updated to reflect that. Assuming the discussion on the other patch is settled (have the property in Camera and not in CameraConfiguration and thus the property is updated at Camera::configure() time), the way the property is updated is up to the pipeline handler. RPi in example does not set the sensor format on the CameraSensor at all, as it happens as a consequence of a setFormat on the CSI-2 video device node and the format is propagated in kernel space. > > Or is SensorOutputSize a fixed immutable thing that never changes? It does indeed change here, some pipeline might decide they always go full size. Anyway applications are meant to re-query the property after a Camera::configure() > > Maybe that's my real question. > > *when* and *where* does this get updated when things change. On pipeline handler side. Some pipeline handlers might want to calculate it themselves, other will fetch the value directly from CameraSensor after a CameraSensor::setFormat() (in which case the operation should update the property, but at the moment it is not required for the RPi specificities described here above). Thanks j > -- > Kieran > > > > > >> > >>> + > >>> return 0; > >>> } > >>> > >>> > >> > >> -- > >> Regards > >> -- > >> Kieran > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index d2679a4b..c9a19b02 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -277,6 +277,12 @@ int CameraSensor::init() */ resolution_ = sizes_.back(); + /* + * Set a default value for the SensorOutputSize, though it will have to + * be updated when new camera modes are chosen. + */ + properties_.set(properties::SensorOutputSize, resolution_); + return 0; }