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

[Laurent Pinchart]

Hi Jacopo,

On Thu, Aug 29, 2019 at 06:00:56PM +0300, Laurent Pinchart wrote:
> 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.

It works \o/

Tested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >>> ---
> >>>  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