[libcamera-devel] [PATCH 1/2] android: Make FRAME_DURATION key available in static metadata

[paul.elder at ideasonboard.com]

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 at 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 at ideasonboard.com> wrote:
>     >
>     >     On Wed, Jun 02, 2021 at 12:30:21PM +0900, paul.elder at 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 at ideasonboard.com>
>     >
>     >
>     > First, this code adds the missing request result key. So
>     > Reviewed-by: Hirokazu Honda <hiroh at 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
>     >     > >
>     >
>