| Message ID | 20210521105534.1125303-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thanks for your work. On 2021-05-21 19:55:33 +0900, Paul Elder wrote: > The sensor frame duration should be set by IPA. Get the information for > the result metadata from libcamera. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/android/camera_device.cpp | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b32e8be5..779ce554 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); > } > > + if (metadata.contains(controls::FrameDurations)) { > + Span<const int64_t> durations = > + metadata.get(controls::FrameDurations); > + if (durations[0] == durations[1]) { Do we need a todo here to expand this to work with min/max frame durations at a later stage? > + int64_t duration = durations[0] * 1000; > + resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > + duration); > + } > + } > + > if (metadata.contains(controls::ScalerCrop)) { > Rectangle crop = metadata.get(controls::ScalerCrop); > int32_t cropRect[] = { > -- > 2.27.0 >
Hi Paul, the Fix in subject is probably not required, as the ANDROID_SENSOR_FRAME_DURATION property was not registered at all. On Fri, May 21, 2021 at 07:55:33PM +0900, Paul Elder wrote: > The sensor frame duration should be set by IPA. Get the information for > the result metadata from libcamera. how about: "Libcamera reports the frame duration through the controls::FrameDuration controls. Populate the android.sensor.frameDuration result metadata using the libcamera control value" > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/android/camera_device.cpp | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b32e8be5..779ce554 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); > } > > + if (metadata.contains(controls::FrameDurations)) { > + Span<const int64_t> durations = > + metadata.get(controls::FrameDurations); > + if (durations[0] == durations[1]) { Yes, we have a bit of an unspecified behaviour here. What is the use case for reporting two different frame durations ? Indeed I see this one: src/ipa/raspberrypi/raspberrypi.cpp: libcameraMetadata_.set(controls::FrameDurations, src/ipa/raspberrypi/raspberrypi.cpp: { static_cast<int64_t>(minFrameDuration_), src/ipa/raspberrypi/raspberrypi.cpp: static_cast<int64_t>(maxFrameDuration_) }); Where the RPi IPA reports the min/max frame durations received as controls part of a Request before using them to clamp the AE computed exposure time and blankings. +Naush, +David Wouldn't it be better to report the actual duration by knowing the actual exposure+blanking values once AGC has run ? Thanks j > + int64_t duration = durations[0] * 1000; > + resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > + duration); > + } > + } > + > if (metadata.contains(controls::ScalerCrop)) { > Rectangle crop = metadata.get(controls::ScalerCrop); > int32_t cropRect[] = { > -- > 2.27.0 >
Hi Jacopo and Paul, On Fri, 21 May 2021 at 12:53, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Paul, > the Fix in subject is probably not required, as the > ANDROID_SENSOR_FRAME_DURATION property was not registered at all. > > On Fri, May 21, 2021 at 07:55:33PM +0900, Paul Elder wrote: > > The sensor frame duration should be set by IPA. Get the information for > > the result metadata from libcamera. > > how about: > > "Libcamera reports the frame duration through the > controls::FrameDuration controls. Populate the > android.sensor.frameDuration result metadata using the libcamera > control value" > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/src/android/camera_device.cpp > b/src/android/camera_device.cpp > > index b32e8be5..779ce554 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const > Camera3RequestDescriptor &descriptor) cons > > resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, > exposure); > > } > > > > + if (metadata.contains(controls::FrameDurations)) { > > + Span<const int64_t> durations = > > + metadata.get(controls::FrameDurations); > > + if (durations[0] == durations[1]) { > > Yes, we have a bit of an unspecified behaviour here. > > What is the use case for reporting two different frame durations ? > > Indeed I see this one: > src/ipa/raspberrypi/raspberrypi.cpp: > libcameraMetadata_.set(controls::FrameDurations, > src/ipa/raspberrypi/raspberrypi.cpp: { > static_cast<int64_t>(minFrameDuration_), > src/ipa/raspberrypi/raspberrypi.cpp: > static_cast<int64_t>(maxFrameDuration_) }); > > Where the RPi IPA reports the min/max frame durations received as > controls part of a Request before using them to clamp the AE computed > exposure time and blankings. > > +Naush, +David > > Wouldn't it be better to report the actual duration by knowing the > actual exposure+blanking values once AGC has run ? > This was discussed some time back, and I think we loosely agreed that the values returned here in the Request metadata are the min/max frame duration values that are actually going to be used, as the may have been clipped based on sensor limitations. Note that this gets put into the metadata only when a user request comes through, and not on every frame. Here is the description I worded in the yaml file: > When reported in metadata, the control expresses the minimum and maximum frame > durations used after being clipped to the sensor provided frame duration limits. Our IPA does not return controls::FrameDurations per-frame, instead we return controls::SensorTimestamp that (almost) gives you the inter-frame duration after exposure+blanking. Regards, Naush > > Thanks > j > > > + int64_t duration = durations[0] * 1000; > > + > resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > > + duration); > > + } > > + } > > + > > if (metadata.contains(controls::ScalerCrop)) { > > Rectangle crop = metadata.get(controls::ScalerCrop); > > int32_t cropRect[] = { > > -- > > 2.27.0 > > >
Hi Paul, On 5/21/21 4:25 PM, Paul Elder wrote: > The sensor frame duration should be set by IPA. Get the information for > the result metadata from libcamera. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/android/camera_device.cpp | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b32e8be5..779ce554 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); > } > > + if (metadata.contains(controls::FrameDurations)) { > + Span<const int64_t> durations = > + metadata.get(controls::FrameDurations); > + if (durations[0] == durations[1]) { > + int64_t duration = durations[0] * 1000; > + resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > + duration); > + } > + } > + I wonder if the hard coding patch [2/2] should live here instead (as a 'else' fallback), with a \todo in ipa/ipu3.cpp ofcourse. Since, we have two IPAs for IPU3 platform(one residing externally), we would need to introduce [2/2] in the external IPA as well, I think, sooner down the CTS line work. As far as I can see, there are already a few hard-coded values in camera_device.cpp for resultMetadata entries. I think this approach would serve a better "singular" place to look at regarding what entries are hard-coded and which one aren't. What do you think? > if (metadata.contains(controls::ScalerCrop)) { > Rectangle crop = metadata.get(controls::ScalerCrop); > int32_t cropRect[] = {
Hello everybody, On Fri, May 21, 2021 at 01:08:37PM +0100, Naushir Patuck wrote: > On Fri, 21 May 2021 at 12:53, Jacopo Mondi wrote: > > > Hi Paul, > > the Fix in subject is probably not required, as the > > ANDROID_SENSOR_FRAME_DURATION property was not registered at all. > > > > On Fri, May 21, 2021 at 07:55:33PM +0900, Paul Elder wrote: > > > The sensor frame duration should be set by IPA. Get the information for > > > the result metadata from libcamera. > > > > how about: > > > > "Libcamera reports the frame duration through the > > controls::FrameDuration controls. Populate the > > android.sensor.frameDuration result metadata using the libcamera > > control value" > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > src/android/camera_device.cpp | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index b32e8be5..779ce554 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > > > resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); > > > } > > > > > > + if (metadata.contains(controls::FrameDurations)) { > > > + Span<const int64_t> durations = > > > + metadata.get(controls::FrameDurations); > > > + if (durations[0] == durations[1]) { > > > > Yes, we have a bit of an unspecified behaviour here. > > > > What is the use case for reporting two different frame durations ? > > > > Indeed I see this one: > > src/ipa/raspberrypi/raspberrypi.cpp: > > libcameraMetadata_.set(controls::FrameDurations, > > src/ipa/raspberrypi/raspberrypi.cpp: { > > static_cast<int64_t>(minFrameDuration_), > > src/ipa/raspberrypi/raspberrypi.cpp: > > static_cast<int64_t>(maxFrameDuration_) }); > > > > Where the RPi IPA reports the min/max frame durations received as > > controls part of a Request before using them to clamp the AE computed > > exposure time and blankings. > > > > +Naush, +David > > > > Wouldn't it be better to report the actual duration by knowing the > > actual exposure+blanking values once AGC has run ? > > This was discussed some time back, and I think we loosely agreed that the values > returned here in the Request metadata are the min/max frame duration values that > are actually going to be used, as the may have been clipped based on sensor > limitations. I don't recall if you had actual use cases to consume this in applications, do you remember ? > Note that this gets put into the metadata only when a user request comes > through, and not on every frame. What metadata should be reported every frame or only in specific circumstances is still to be discussed, but that's a topic for later :-) > Here is the description I worded in the yaml file: > > > When reported in metadata, the control expresses the minimum and maximum frame > > durations used after being clipped to the sensor provided frame duration limits. > > Our IPA does not return controls::FrameDurations per-frame, instead we return > controls::SensorTimestamp that (almost) gives you the inter-frame duration after > exposure+blanking. That's subject to jitter. The timestamp is important, but reporting the nominal frame duration is also important. How about renaming FrameDurations to FrameDurationLimits, and adding a new FrameDuration control to report the nominal duration for the frame in the metadata ? > > > + int64_t duration = durations[0] * 1000; > > > + > > resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > > > + duration); > > > + } > > > + } > > > + > > > if (metadata.contains(controls::ScalerCrop)) { > > > Rectangle crop = metadata.get(controls::ScalerCrop); > > > int32_t cropRect[] = {
Hi all, On Sun, 23 May 2021, 1:14 am Laurent Pinchart, < laurent.pinchart@ideasonboard.com> wrote: > Hello everybody, > > On Fri, May 21, 2021 at 01:08:37PM +0100, Naushir Patuck wrote: > > On Fri, 21 May 2021 at 12:53, Jacopo Mondi wrote: > > > > > Hi Paul, > > > the Fix in subject is probably not required, as the > > > ANDROID_SENSOR_FRAME_DURATION property was not registered at all. > > > > > > On Fri, May 21, 2021 at 07:55:33PM +0900, Paul Elder wrote: > > > > The sensor frame duration should be set by IPA. Get the information > for > > > > the result metadata from libcamera. > > > > > > how about: > > > > > > "Libcamera reports the frame duration through the > > > controls::FrameDuration controls. Populate the > > > android.sensor.frameDuration result metadata using the libcamera > > > control value" > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > > > src/android/camera_device.cpp | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/src/android/camera_device.cpp > b/src/android/camera_device.cpp > > > > index b32e8be5..779ce554 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const > Camera3RequestDescriptor &descriptor) cons > > > > resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, > exposure); > > > > } > > > > > > > > + if (metadata.contains(controls::FrameDurations)) { > > > > + Span<const int64_t> durations = > > > > + metadata.get(controls::FrameDurations); > > > > + if (durations[0] == durations[1]) { > > > > > > Yes, we have a bit of an unspecified behaviour here. > > > > > > What is the use case for reporting two different frame durations ? > > > > > > Indeed I see this one: > > > src/ipa/raspberrypi/raspberrypi.cpp: > > > libcameraMetadata_.set(controls::FrameDurations, > > > src/ipa/raspberrypi/raspberrypi.cpp: { > > > static_cast<int64_t>(minFrameDuration_), > > > src/ipa/raspberrypi/raspberrypi.cpp: > > > static_cast<int64_t>(maxFrameDuration_) }); > > > > > > Where the RPi IPA reports the min/max frame durations received as > > > controls part of a Request before using them to clamp the AE computed > > > exposure time and blankings. > > > > > > +Naush, +David > > > > > > Wouldn't it be better to report the actual duration by knowing the > > > actual exposure+blanking values once AGC has run ? > > > > This was discussed some time back, and I think we loosely agreed that > the values > > returned here in the Request metadata are the min/max frame duration > values that > > are actually going to be used, as the may have been clipped based on > sensor > > limitations. > > I don't recall if you had actual use cases to consume this in > applications, do you remember ? > > > Note that this gets put into the metadata only when a user request comes > > through, and not on every frame. > > What metadata should be reported every frame or only in specific > circumstances is still to be discussed, but that's a topic for later :-) > > > Here is the description I worded in the yaml file: > > > > > When reported in metadata, the control expresses the minimum and > maximum frame > > > durations used after being clipped to the sensor provided frame > duration limits. > > > > Our IPA does not return controls::FrameDurations per-frame, instead we > return > > controls::SensorTimestamp that (almost) gives you the inter-frame > duration after > > exposure+blanking. > > That's subject to jitter. The timestamp is important, but reporting the > nominal frame duration is also important. > > How about renaming FrameDurations to FrameDurationLimits, and adding a > new FrameDuration control to report the nominal duration for the frame > in the metadata ? > Sounds good to me. We will need to sync the change with our libcamera-apps as they do use this control, but I'm sure that's manageable. Naush > > > > + int64_t duration = durations[0] * 1000; > > > > + > > > resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > > > > + duration); > > > > + } > > > > + } > > > > + > > > > if (metadata.contains(controls::ScalerCrop)) { > > > > Rectangle crop = metadata.get(controls::ScalerCrop); > > > > int32_t cropRect[] = { > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index b32e8be5..779ce554 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); } + if (metadata.contains(controls::FrameDurations)) { + Span<const int64_t> durations = + metadata.get(controls::FrameDurations); + if (durations[0] == durations[1]) { + int64_t duration = durations[0] * 1000; + resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, + duration); + } + } + if (metadata.contains(controls::ScalerCrop)) { Rectangle crop = metadata.get(controls::ScalerCrop); int32_t cropRect[] = {
The sensor frame duration should be set by IPA. Get the information for the result metadata from libcamera. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/android/camera_device.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+)