[libcamera-devel] [RFC PATCH 2/6] android: camera_device: Report configuration changes from validate()

[Jacopo Mondi]

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 at 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 at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran