<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jun 3, 2021 at 3:14 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Wed, Jun 02, 2021 at 02:01:24PM +0900, Hirokazu Honda wrote:<br>
> On Wed, Jun 2, 2021 at 12:32 PM <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>> wrote:<br>
> > On Wed, Jun 02, 2021 at 12:30:21PM +0900, <a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>  wrote:<br>
> > > On Tue, Jun 01, 2021 at 04:54:55PM +0530, Umang Jain wrote:<br>
> > > > Report ANDROID_SENSOR_FRAME_DURATION as an available key for CTS to<br>
> > ><br>
> > > s/available key/available result key/<br>
> > ><br>
> > > It's also a valid request key, and this patch doesn't add that, so I<br>
> > > think it should be specified (also in the subject).<br>
> ><br>
> > By "specified" I mean that "result" should be specified.<br>
> ><br>
> > As for the subject, s/key/result key/<br>
> ><br>
> > > > read out the value of frame duration we set in CameraDevice::getResultMetadata().<br>
> > > > Failing to do so might fail the CTS test:<br>
> > > >  - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl<br>
> > > ><br>
> > > > Fixes: 3beb1accac1d ("android: camera_device: Fix sensor frame duration")<br>
> > > > Signed-off-by: Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@ideasonboard.com</a>><br>
> <br>
> First, this code adds the missing request result key. So<br>
> Reviewed-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> <br>
> However, although this is not necessarily related to this change, I wonder<br>
> if those available keys should be there if and only if an entry with the<br>
> key is actually added.<br>
<br>
Generally speaking, we should only report available result keys that the<br>
camera can provide, yes. In this particular case however, I think we<br>
should make the frame duration mandatory for cameras to report, so we<br>
can hardcode ANDROID_SENSOR_FRAME_DURATION.<br>
<br></blockquote><div><br></div><div>I agree. So I am fine to push this as-is indeed.</div><div>Thanks for merging.</div><div><br></div><div>Regards, </div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Could you run android.hardware.camera2.cts.CaptureResultTest while some<br>
> entry is dropped and the entry is in available keys?<br>
> <br>
> > > ---<br>
> > > >  src/android/camera_device.cpp | 1 +<br>
> > > >  1 file changed, 1 insertion(+)<br>
> > > ><br>
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> > > > index fddc07ff..fe332ec3 100644<br>
> > > > --- a/src/android/camera_device.cpp<br>
> > > > +++ b/src/android/camera_device.cpp<br>
> > > > @@ -1422,6 +1422,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()<br>
> > > >  Â  Â  Â  Â  Â  Â ANDROID_REQUEST_PIPELINE_DEPTH,<br>
> > > >  Â  Â  Â  Â  Â  Â ANDROID_SCALER_CROP_REGION,<br>
> > > >  Â  Â  Â  Â  Â  Â ANDROID_SENSOR_EXPOSURE_TIME,<br>
> > > > +  Â  Â  Â  Â  Â ANDROID_SENSOR_FRAME_DURATION,<br>
> > > >  Â  Â  Â  Â  Â  Â ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,<br>
> > > >  Â  Â  Â  Â  Â  Â ANDROID_SENSOR_TEST_PATTERN_MODE,<br>
> > > >  Â  Â  Â  Â  Â  Â ANDROID_SENSOR_TIMESTAMP,<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>