| Message ID | 20210601112456.118755-2-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to > read out the value of frame duration we set in CameraDevice::getResultMetadata(). > Failing to do so might fail the CTS test: > - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration") > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Looks good to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Do I understand that this can be applied separately from 2/2 ? > --- > src/android/camera_device.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index fddc07ff..fe332ec3 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > ANDROID_REQUEST_PIPELINE_DEPTH, > ANDROID_SCALER_CROP_REGION, > ANDROID_SENSOR_EXPOSURE_TIME, > + ANDROID_SENSOR_FRAME_DURATION, > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > ANDROID_SENSOR_TEST_PATTERN_MODE, > ANDROID_SENSOR_TIMESTAMP,
Hi Laurent, On 6/2/21 6:43 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: >> Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to >> read out the value of frame duration we set in CameraDevice::getResultMetadata(). >> Failing to do so might fail the CTS test: >> - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl >> >> Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration") >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Looks good to me. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Do I understand that this can be applied separately from 2/2 ? As far as I understand, yes it can be applied. > >> --- >> src/android/camera_device.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index fddc07ff..fe332ec3 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() >> ANDROID_REQUEST_PIPELINE_DEPTH, >> ANDROID_SCALER_CROP_REGION, >> ANDROID_SENSOR_EXPOSURE_TIME, >> + ANDROID_SENSOR_FRAME_DURATION, >> ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, >> ANDROID_SENSOR_TEST_PATTERN_MODE, >> ANDROID_SENSOR_TIMESTAMP,
Hi Umang, On Wed, Jun 02, 2021 at 08:20:24AM +0530, Umang Jain wrote: > Hi Laurent, > > On 6/2/21 6:43 AM, Laurent Pinchart wrote: > > Hi Umang, > > > > Thank you for the patch. > > > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to > > > read out the value of frame duration we set in CameraDevice::getResultMetadata(). > > > Failing to do so might fail the CTS test: > > > - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration") Sorry about that oversight :/ I'm not sure how I missed this in the first place. > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > Looks good to me. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Do I understand that this can be applied separately from 2/2 ? > As far as I understand, yes it can be applied. Yeah it can go in on its own :D > > > > > --- > > > src/android/camera_device.cpp | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index fddc07ff..fe332ec3 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > ANDROID_SCALER_CROP_REGION, > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > + ANDROID_SENSOR_FRAME_DURATION, > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > ANDROID_SENSOR_TIMESTAMP,
Hi Umang, On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to s/available key/available result key/ It's also a valid request key, and this patch doesn't add that, so I think it should be specified (also in the subject). Paul > read out the value of frame duration we set in CameraDevice::getResultMetadata(). > Failing to do so might fail the CTS test: > - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration") > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index fddc07ff..fe332ec3 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > ANDROID_REQUEST_PIPELINE_DEPTH, > ANDROID_SCALER_CROP_REGION, > ANDROID_SENSOR_EXPOSURE_TIME, > + ANDROID_SENSOR_FRAME_DURATION, > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > ANDROID_SENSOR_TEST_PATTERN_MODE, > ANDROID_SENSOR_TIMESTAMP, > -- > 2.31.1 >
On Wed, Jun 02, 2021 at 12:30:21PM +0900, paul.elder@ideasonboard.com wrote: > Hi Umang, > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to > > s/available key/available result key/ > > It's also a valid request key, and this patch doesn't add that, so I > think it should be specified (also in the subject). By "specified" I mean that "result" should be specified. As for the subject, s/key/result key/ Paul > > > read out the value of frame duration we set in CameraDevice::getResultMetadata(). > > Failing to do so might fail the CTS test: > > - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration") > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index fddc07ff..fe332ec3 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > ANDROID_REQUEST_PIPELINE_DEPTH, > > ANDROID_SCALER_CROP_REGION, > > ANDROID_SENSOR_EXPOSURE_TIME, > > + ANDROID_SENSOR_FRAME_DURATION, > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > ANDROID_SENSOR_TIMESTAMP, > > -- > > 2.31.1 > >
Hi Paul, thank you for the patch. On Wed, Jun 2, 2021 at 12:32 PM <paul.elder@ideasonboard.com> wrote: > On Wed, Jun 02, 2021 at 12:30:21PM +0900, paul.elder@ideasonboard.com > wrote: > > Hi Umang, > > > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to > > > > s/available key/available result key/ > > > > It's also a valid request key, and this patch doesn't add that, so I > > think it should be specified (also in the subject). > > By "specified" I mean that "result" should be specified. > > As for the subject, s/key/result key/ > > > Paul > > > > > > read out the value of frame duration we set in > CameraDevice::getResultMetadata(). > > > Failing to do so might fail the CTS test: > > > - > android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame > duration") > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > First, this code adds the missing request result key. So Reviewed-by: Hirokazu Honda <hiroh@chromium.org> However, although this is not necessarily related to this change, I wonder if those available keys should be there if and only if an entry with the key is actually added. Could you run android.hardware.camera2.cts.CaptureResultTest while some entry is dropped and the entry is in available keys? -Hiro > > --- > > > src/android/camera_device.cpp | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/src/android/camera_device.cpp > b/src/android/camera_device.cpp > > > index fddc07ff..fe332ec3 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t > *CameraDevice::getStaticMetadata() > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > ANDROID_SCALER_CROP_REGION, > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > + ANDROID_SENSOR_FRAME_DURATION, > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > ANDROID_SENSOR_TIMESTAMP, > > > -- > > > 2.31.1 > > > >
Hi Hiro, On Wed, Jun 02, 2021 at 02:01:24PM +0900, Hirokazu Honda wrote: > Hi Paul, thank you for the patch. > > On Wed, Jun 2, 2021 at 12:32 PM <paul.elder@ideasonboard.com> wrote: > > On Wed, Jun 02, 2021 at 12:30:21PM +0900, paul.elder@ideasonboard.com > wrote: > > Hi Umang, > > > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to > > > > s/available key/available result key/ > > > > It's also a valid request key, and this patch doesn't add that, so I > > think it should be specified (also in the subject). > > By "specified" I mean that "result" should be specified. > > As for the subject, s/key/result key/ > > > Paul > > > > > > read out the value of frame duration we set in > CameraDevice::getResultMetadata(). > > > Failing to do so might fail the CTS test: > > > - android.hardware.camera2.cts.CaptureRequestTest# > testNoiseReductionModeControl > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame > duration") > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > First, this code adds the missing request result key. So > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > However, although this is not necessarily related to this change, I wonder if > those available keys should be there if and only if an entry with the key is > actually added. I think you're right. We might need to rethink where the keys and values come from :/ I guess if we declare that we support some key then the application can expect that it's always present. > Could you run android.hardware.camera2.cts.CaptureResultTest while some entry > is dropped and the entry is in available keys? android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl fails complaining that the key that it expects to be present is not. The other tests don't seem to care. Paul > > > > --- > > > src/android/camera_device.cpp | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/ > camera_device.cpp > > > index fddc07ff..fe332ec3 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t > *CameraDevice::getStaticMetadata() > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > ANDROID_SCALER_CROP_REGION, > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > + ANDROID_SENSOR_FRAME_DURATION, > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > ANDROID_SENSOR_TIMESTAMP, > > > -- > > > 2.31.1 > > > >
Hi Paul, On Wed, Jun 2, 2021 at 3:32 PM <paul.elder@ideasonboard.com> wrote: > Hi Hiro, > > On Wed, Jun 02, 2021 at 02:01:24PM +0900, Hirokazu Honda wrote: > > Hi Paul, thank you for the patch. > > > > On Wed, Jun 2, 2021 at 12:32 PM <paul.elder@ideasonboard.com> wrote: > > > > On Wed, Jun 02, 2021 at 12:30:21PM +0900, > paul.elder@ideasonboard.com > > wrote: > > > Hi Umang, > > > > > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS > to > > > > > > s/available key/available result key/ > > > > > > It's also a valid request key, and this patch doesn't add that, so > I > > > think it should be specified (also in the subject). > > > > By "specified" I mean that "result" should be specified. > > > > As for the subject, s/key/result key/ > > > > > > Paul > > > > > > > > > read out the value of frame duration we set in > > CameraDevice::getResultMetadata(). > > > > Failing to do so might fail the CTS test: > > > > - android.hardware.camera2.cts.CaptureRequestTest# > > testNoiseReductionModeControl > > > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame > > duration") > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > First, this code adds the missing request result key. So > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > However, although this is not necessarily related to this change, I > wonder if > > those available keys should be there if and only if an entry with the > key is > > actually added. > > I think you're right. We might need to rethink where the keys and values > come from :/ > > I guess if we declare that we support some key then the application > can expect that it's always present. > > I wonder if we should dynamically construct the available keys upon CameraMetadata::get() from keys that have been added. We should look up the hard-code tables, if there is no smart way, to find what each key is added to, ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS, ANDROID_REQUEST_AVAILABLE_RESULT_KEYS or ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS. -Hiro > > Could you run android.hardware.camera2.cts.CaptureResultTest while some > entry > > is dropped and the entry is in available keys? > > > android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > fails complaining that the key that it expects to be present is not. The > other tests don't seem to care. > > > Paul > > > > > > > --- > > > > src/android/camera_device.cpp | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/ > > camera_device.cpp > > > > index fddc07ff..fe332ec3 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t > > *CameraDevice::getStaticMetadata() > > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > > ANDROID_SCALER_CROP_REGION, > > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > > + ANDROID_SENSOR_FRAME_DURATION, > > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > > ANDROID_SENSOR_TIMESTAMP, > > > > -- > > > > 2.31.1 > > > > > > >
Hi Hiro, On Wed, Jun 02, 2021 at 04:57:58PM +0900, Hirokazu Honda wrote: > Hi Paul, > > On Wed, Jun 2, 2021 at 3:32 PM <paul.elder@ideasonboard.com> wrote: > > Hi Hiro, > > On Wed, Jun 02, 2021 at 02:01:24PM +0900, Hirokazu Honda wrote: > > Hi Paul, thank you for the patch. > > > > On Wed, Jun 2, 2021 at 12:32 PM <paul.elder@ideasonboard.com> wrote: > > > > On Wed, Jun 02, 2021 at 12:30:21PM +0900, paul.elder@ideasonboard.com > > wrote: > > > Hi Umang, > > > > > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS > to > > > > > > s/available key/available result key/ > > > > > > It's also a valid request key, and this patch doesn't add that, so > I > > > think it should be specified (also in the subject). > > > > By "specified" I mean that "result" should be specified. > > > > As for the subject, s/key/result key/ > > > > > > Paul > > > > > > > > > read out the value of frame duration we set in > > CameraDevice::getResultMetadata(). > > > > Failing to do so might fail the CTS test: > > > > - android.hardware.camera2.cts.CaptureRequestTest# > > testNoiseReductionModeControl > > > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame > > duration") > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > First, this code adds the missing request result key. So > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > However, although this is not necessarily related to this change, I > wonder if > > those available keys should be there if and only if an entry with the key > is > > actually added. > > I think you're right. We might need to rethink where the keys and values > come from :/ > > I guess if we declare that we support some key then the application > can expect that it's always present. > > > > I wonder if we should dynamically construct the available keys upon > CameraMetadata::get() from keys that have been added. The static metadata is constructed and returned way before any result metadata is ever returned though. I'm currently redesigning how we populate the static metadata based on Camera capabilities (eg. what libcamera controls the camera supports). It'll probably also connect into what we return in the result metadata, and what we accept in the request metadata. These both will of course affect the available keys that we report in the static metadata. I'll keep this in mind in the redesign. Paul > We should look up the hard-code tables, if there is no smart way, to find what > each key is added to, ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS, > ANDROID_REQUEST_AVAILABLE_RESULT_KEYS or > ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS. > > -Hiro > > > Could you run android.hardware.camera2.cts.CaptureResultTest while some > entry > > is dropped and the entry is in available keys? > > android.hardware.camera2.cts.CaptureRequestTest# > testNoiseReductionModeControl > fails complaining that the key that it expects to be present is not. The > other tests don't seem to care. > > > Paul > > > > > > > --- > > > > src/android/camera_device.cpp | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/ > > camera_device.cpp > > > > index fddc07ff..fe332ec3 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t > > *CameraDevice::getStaticMetadata() > > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > > ANDROID_SCALER_CROP_REGION, > > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > > + ANDROID_SENSOR_FRAME_DURATION, > > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > > ANDROID_SENSOR_TIMESTAMP, > > > > -- > > > > 2.31.1 > > > > > > >
Hi Hiro, On Wed, Jun 02, 2021 at 02:01:24PM +0900, Hirokazu Honda wrote: > On Wed, Jun 2, 2021 at 12:32 PM <paul.elder@ideasonboard.com> wrote: > > On Wed, Jun 02, 2021 at 12:30:21PM +0900, paul.elder@ideasonboard.com wrote: > > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to > > > > > > s/available key/available result key/ > > > > > > It's also a valid request key, and this patch doesn't add that, so I > > > think it should be specified (also in the subject). > > > > By "specified" I mean that "result" should be specified. > > > > As for the subject, s/key/result key/ > > > > > > read out the value of frame duration we set in CameraDevice::getResultMetadata(). > > > > Failing to do so might fail the CTS test: > > > > - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration") > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > First, this code adds the missing request result key. So > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > However, although this is not necessarily related to this change, I wonder > if those available keys should be there if and only if an entry with the > key is actually added. Generally speaking, we should only report available result keys that the camera can provide, yes. In this particular case however, I think we should make the frame duration mandatory for cameras to report, so we can hardcode ANDROID_SENSOR_FRAME_DURATION. > Could you run android.hardware.camera2.cts.CaptureResultTest while some > entry is dropped and the entry is in available keys? > > > > --- > > > > src/android/camera_device.cpp | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index fddc07ff..fe332ec3 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > > ANDROID_SCALER_CROP_REGION, > > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > > + ANDROID_SENSOR_FRAME_DURATION, > > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > > ANDROID_SENSOR_TIMESTAMP,
On Wed, Jun 02, 2021 at 12:32:24PM +0900, paul.elder@ideasonboard.com wrote: > On Wed, Jun 02, 2021 at 12:30:21PM +0900, paul.elder@ideasonboard.com wrote: > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to > > > > s/available key/available result key/ > > > > It's also a valid request key, and this patch doesn't add that, so I > > think it should be specified (also in the subject). > > By "specified" I mean that "result" should be specified. > > As for the subject, s/key/result key/ Pushed with an updated commit message. > > > read out the value of frame duration we set in CameraDevice::getResultMetadata(). > > > Failing to do so might fail the CTS test: > > > - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration") > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > src/android/camera_device.cpp | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index fddc07ff..fe332ec3 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > ANDROID_SCALER_CROP_REGION, > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > + ANDROID_SENSOR_FRAME_DURATION, > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > ANDROID_SENSOR_TIMESTAMP,
Hi Paul, On Wed, Jun 2, 2021 at 6:52 PM <paul.elder@ideasonboard.com> wrote: > Hi Hiro, > > On Wed, Jun 02, 2021 at 04:57:58PM +0900, Hirokazu Honda wrote: > > Hi Paul, > > > > On Wed, Jun 2, 2021 at 3:32 PM <paul.elder@ideasonboard.com> wrote: > > > > Hi Hiro, > > > > On Wed, Jun 02, 2021 at 02:01:24PM +0900, Hirokazu Honda wrote: > > > Hi Paul, thank you for the patch. > > > > > > On Wed, Jun 2, 2021 at 12:32 PM <paul.elder@ideasonboard.com> > wrote: > > > > > > On Wed, Jun 02, 2021 at 12:30:21PM +0900, > paul.elder@ideasonboard.com > > > wrote: > > > > Hi Umang, > > > > > > > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key > for CTS > > to > > > > > > > > s/available key/available result key/ > > > > > > > > It's also a valid request key, and this patch doesn't add > that, so > > I > > > > think it should be specified (also in the subject). > > > > > > By "specified" I mean that "result" should be specified. > > > > > > As for the subject, s/key/result key/ > > > > > > > > > Paul > > > > > > > > > > > > read out the value of frame duration we set in > > > CameraDevice::getResultMetadata(). > > > > > Failing to do so might fail the CTS test: > > > > > - android.hardware.camera2.cts.CaptureRequestTest# > > > testNoiseReductionModeControl > > > > > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor > frame > > > duration") > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > > > > First, this code adds the missing request result key. So > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > However, although this is not necessarily related to this change, I > > wonder if > > > those available keys should be there if and only if an entry with > the key > > is > > > actually added. > > > > I think you're right. We might need to rethink where the keys and > values > > come from :/ > > > > I guess if we declare that we support some key then the application > > can expect that it's always present. > > > > > > > > I wonder if we should dynamically construct the available keys upon > > CameraMetadata::get() from keys that have been added. > > The static metadata is constructed and returned way before any result > metadata is ever returned though. > > I'm currently redesigning how we populate the static metadata based on > Camera capabilities (eg. what libcamera controls the camera supports). > It'll probably also connect into what we return in the result metadata, > and what we accept in the request metadata. These both will of course > affect the available keys that we report in the static metadata. > I'll keep this in mind in the redesign. > > Ack. I look forward to your redesign. Thanks, -Hiro > > Paul > > > We should look up the hard-code tables, if there is no smart way, to > find what > > each key is added to, ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS, > > ANDROID_REQUEST_AVAILABLE_RESULT_KEYS or > > ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS. > > > > -Hiro > > > > > Could you run android.hardware.camera2.cts.CaptureResultTest while > some > > entry > > > is dropped and the entry is in available keys? > > > > android.hardware.camera2.cts.CaptureRequestTest# > > testNoiseReductionModeControl > > fails complaining that the key that it expects to be present is not. > The > > other tests don't seem to care. > > > > > > Paul > > > > > > > > > > --- > > > > > src/android/camera_device.cpp | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/ > > > camera_device.cpp > > > > > index fddc07ff..fe332ec3 100644 > > > > > --- a/src/android/camera_device.cpp > > > > > +++ b/src/android/camera_device.cpp > > > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t > > > *CameraDevice::getStaticMetadata() > > > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > > > ANDROID_SCALER_CROP_REGION, > > > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > > > + ANDROID_SENSOR_FRAME_DURATION, > > > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > > > ANDROID_SENSOR_TIMESTAMP, > > > > > -- > > > > > 2.31.1 > > > > > > > > > > >
Hi Laurent, On Thu, Jun 3, 2021 at 3:14 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Hiro, > > On Wed, Jun 02, 2021 at 02:01:24PM +0900, Hirokazu Honda wrote: > > On Wed, Jun 2, 2021 at 12:32 PM <paul.elder@ideasonboard.com> wrote: > > > On Wed, Jun 02, 2021 at 12:30:21PM +0900, paul.elder@ideasonboard.com > wrote: > > > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote: > > > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to > > > > > > > > s/available key/available result key/ > > > > > > > > It's also a valid request key, and this patch doesn't add that, so I > > > > think it should be specified (also in the subject). > > > > > > By "specified" I mean that "result" should be specified. > > > > > > As for the subject, s/key/result key/ > > > > > > > > read out the value of frame duration we set in > CameraDevice::getResultMetadata(). > > > > > Failing to do so might fail the CTS test: > > > > > - > android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > > > > > > > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame > duration") > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > First, this code adds the missing request result key. So > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > However, although this is not necessarily related to this change, I > wonder > > if those available keys should be there if and only if an entry with the > > key is actually added. > > Generally speaking, we should only report available result keys that the > camera can provide, yes. In this particular case however, I think we > should make the frame duration mandatory for cameras to report, so we > can hardcode ANDROID_SENSOR_FRAME_DURATION. > > I agree. So I am fine to push this as-is indeed. Thanks for merging. Regards, -Hiro > Could you run android.hardware.camera2.cts.CaptureResultTest while some > > entry is dropped and the entry is in available keys? > > > > > > --- > > > > > src/android/camera_device.cpp | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/src/android/camera_device.cpp > b/src/android/camera_device.cpp > > > > > index fddc07ff..fe332ec3 100644 > > > > > --- a/src/android/camera_device.cpp > > > > > +++ b/src/android/camera_device.cpp > > > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t > *CameraDevice::getStaticMetadata() > > > > > ANDROID_REQUEST_PIPELINE_DEPTH, > > > > > ANDROID_SCALER_CROP_REGION, > > > > > ANDROID_SENSOR_EXPOSURE_TIME, > > > > > + ANDROID_SENSOR_FRAME_DURATION, > > > > > ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, > > > > > ANDROID_SENSOR_TEST_PATTERN_MODE, > > > > > ANDROID_SENSOR_TIMESTAMP, > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index fddc07ff..fe332ec3 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() ANDROID_REQUEST_PIPELINE_DEPTH, ANDROID_SCALER_CROP_REGION, ANDROID_SENSOR_EXPOSURE_TIME, + ANDROID_SENSOR_FRAME_DURATION, ANDROID_SENSOR_ROLLING_SHUTTER_SKEW, ANDROID_SENSOR_TEST_PATTERN_MODE, ANDROID_SENSOR_TIMESTAMP,
Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to read out the value of frame duration we set in CameraDevice::getResultMetadata(). Failing to do so might fail the CTS test: - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration") Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/camera_device.cpp | 1 + 1 file changed, 1 insertion(+)