[libcamera-devel] [PATCH] android: camera_device: Add pipeline max depth static metadata

[Laurent Pinchart]

Hi Jacopo,

On Wed, Aug 28, 2019 at 05:42:28PM +0200, Jacopo Mondi wrote:
> On Wed, Aug 28, 2019 at 11:16:20PM +0800, Ren-Pei Zeng wrote:
> > Thanks for working on this!
> >
> > +       uint8_t maxPipelineDepth = 1;
> > Should this keep the original value, i.e. = 5?
> 
> My understanding is that as long as the HAL reports to support LEGACY
> mode this should not be a big issue, as the maxDepth is relevant only
> in regards to the dynamic control android.sync.frameNumber, which we
> currently don't report and should be probably set to UNKNOWN as long
> as we work in backward compatibility mode.
> 
> Is my understanding correct here in your opinion?
> 
> I chose 1 because we actually don't apply any control at the moment,
> and the value there should depend on the platform specific pipeline
> handler characteristics.

The camera HAL documentation mentions that this value is usually at
least two, to cover the exposure stage and the readout stage. Would that
be a better default ?

> Why would you prefer to keep a value of 5 here ? I'm not opposed to
> it, just for my better understanding!
> 
> > On Wed, Aug 28, 2019 at 10:12 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > > The ANDROID_REQUEST_PIPELINE_MAX_DEPTH metadata tag was wrongly
> > > reported in the capture settings template, while it is actually a static
> > > camera metadata.
> > >
> > > As of Chromium R78 the absence of this specific metadata tag causes a
> > > system crash. Fix this by reporting the maximum pipeline depth in the
> > > static metadata pack and set its value to 1 as currently no control is
> > > applied to the image capture pipeline.
> > >
> > > Reported-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Suggested-by: Ren-Pei Zeng <kamesan at google.com>
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > ---
> > > Laurent, as you're running R78 and first reported the crash, could you
> > > please
> > > give this a spin and report if the issue is still present?

I'll test this tonight.

> > > ---
> > >  src/android/camera_device.cpp | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index c27175ac090d..7c69d0810eee 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -281,6 +281,13 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> > >                         ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> > >                         &supportedHWLevel, 1);
> > >
> > > +       /* Request static metadata. */
> > > +       uint8_t maxPipelineDepth = 1;
> > > +       ret = add_camera_metadata_entry(staticMetadata_,
> > > +                       ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > > +                       &maxPipelineDepth, 1);
> > > +       METADATA_ASSERT(ret);
> > > +
> > >         return staticMetadata_;
> > >  }
> > >
> > > @@ -340,12 +347,6 @@ const camera_metadata_t
> > > *CameraDevice::constructDefaultRequestSettings(int type)
> > >                         maxOutStream, 3);
> > >         METADATA_ASSERT(ret);
> > >
> > > -       uint8_t maxPipelineDepth = 5;
> > > -       ret = add_camera_metadata_entry(requestTemplate_,
> > > -                       ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > > -                       &maxPipelineDepth, 1);
> > > -       METADATA_ASSERT(ret);
> > > -
> > >         int32_t inputStreams = 0;
> > >         ret = add_camera_metadata_entry(requestTemplate_,
> > >                         ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,

-- 
Regards,

Laurent Pinchart