| Message ID | 20200721220126.202065-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On Tue, Jul 21, 2020 at 11:01:22PM +0100, Kieran Bingham wrote: > When we call validate on a configuration, if there are any adjustments > on the configuration, we fail without showing why. > > Display the stream configuration after the validate stage to aid > debugging stream startup failures. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/android/camera_device.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index f51542b282d5..3f3d7857f0ab 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1032,6 +1032,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > break; > case CameraConfiguration::Adjusted: > LOG(HAL, Info) << "Camera configuration adjusted"; > + > + for (unsigned int i = 0; i < stream_list->num_streams; ++i) { > + CameraStream *cameraStream = &streams_[i]; > + StreamConfiguration &cfg = config_->at(cameraStream->index); > + > + LOG(HAL, Info) << i << " : " << cfg.toString(); Debug maybe ? > + } > + > config_.reset(); > return -EINVAL; > case CameraConfiguration::Invalid: > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On 22/07/2020 11:36, Jacopo Mondi wrote: > Hi Kieran, > > On Tue, Jul 21, 2020 at 11:01:22PM +0100, Kieran Bingham wrote: >> When we call validate on a configuration, if there are any adjustments >> on the configuration, we fail without showing why. >> >> Display the stream configuration after the validate stage to aid >> debugging stream startup failures. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index f51542b282d5..3f3d7857f0ab 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -1032,6 +1032,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) >> break; >> case CameraConfiguration::Adjusted: >> LOG(HAL, Info) << "Camera configuration adjusted"; >> + >> + for (unsigned int i = 0; i < stream_list->num_streams; ++i) { >> + CameraStream *cameraStream = &streams_[i]; >> + StreamConfiguration &cfg = config_->at(cameraStream->index); >> + >> + LOG(HAL, Info) << i << " : " << cfg.toString(); > > Debug maybe ? This is on the fail path, so it's highly relevant to seeing why the stream was not constructed. The validation phase has failed. I chose 'Info' to match the warning statement above. This is a continuation of that statement IMO, It could be reduced to Debug ... but I would expect /any/ time this has printed, that it would be relevant to know... I'd almost put it as a Warning.... Unless Android uses this path as a means to test the capabilities of the device? If it's a 'probing' path and failures are expected then yes, this could be a Debug print ... but I sort of thought this code path expects success... so a validation failure is a fairly critical event. If that is true, I'd even put the "Camera configuration adjusted" up to warning/error though. > >> + } >> + >> config_.reset(); >> return -EINVAL; >> case CameraConfiguration::Invalid: >> -- >> 2.25.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Wed, Jul 22, 2020 at 12:08:13PM +0100, Kieran Bingham wrote: > On 22/07/2020 11:36, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Tue, Jul 21, 2020 at 11:01:22PM +0100, Kieran Bingham wrote: > >> When we call validate on a configuration, if there are any adjustments > >> on the configuration, we fail without showing why. > >> > >> Display the stream configuration after the validate stage to aid > >> debugging stream startup failures. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/android/camera_device.cpp | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index f51542b282d5..3f3d7857f0ab 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -1032,6 +1032,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > >> break; > >> case CameraConfiguration::Adjusted: > >> LOG(HAL, Info) << "Camera configuration adjusted"; > >> + > >> + for (unsigned int i = 0; i < stream_list->num_streams; ++i) { > >> + CameraStream *cameraStream = &streams_[i]; > >> + StreamConfiguration &cfg = config_->at(cameraStream->index); > >> + > >> + LOG(HAL, Info) << i << " : " << cfg.toString(); > > > > Debug maybe ? > > This is on the fail path, so it's highly relevant to seeing why the > stream was not constructed. The validation phase has failed. I chose > 'Info' to match the warning statement above. Oh well, we fail on adjusted stream, so it makes sense, I was afraid of messages being duplicated between here and the pipeline handler. > > This is a continuation of that statement IMO, It could be reduced to > Debug ... but I would expect /any/ time this has printed, that it would > be relevant to know... I'd almost put it as a Warning.... > > Unless Android uses this path as a means to test the capabilities of the > device? If it's a 'probing' path and failures are expected then yes, > this could be a Debug print ... but I sort of thought this code path > expects success... so a validation failure is a fairly critical event. I guess it does expect to succeed yes, as the HAL reports to the framework which streams are supported. Although their combination can fail, but I don't see Android application to 'try-and-see' up to the point of flooding the log with this error message :) > > If that is true, I'd even put the "Camera configuration adjusted" up to > warning/error though. we could, and if it's too invasive we can demote it later Thanks j > > > > > >> + } > >> + > >> config_.reset(); > >> return -EINVAL; > >> case CameraConfiguration::Invalid: > >> -- > >> 2.25.1 > >> > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index f51542b282d5..3f3d7857f0ab 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1032,6 +1032,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) break; case CameraConfiguration::Adjusted: LOG(HAL, Info) << "Camera configuration adjusted"; + + for (unsigned int i = 0; i < stream_list->num_streams; ++i) { + CameraStream *cameraStream = &streams_[i]; + StreamConfiguration &cfg = config_->at(cameraStream->index); + + LOG(HAL, Info) << i << " : " << cfg.toString(); + } + config_.reset(); return -EINVAL; case CameraConfiguration::Invalid:
When we call validate on a configuration, if there are any adjustments on the configuration, we fail without showing why. Display the stream configuration after the validate stage to aid debugging stream startup failures. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/android/camera_device.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)