| Message ID | 20190828141343.10237-1-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Thanks for working on this! + uint8_t maxPipelineDepth = 1; Should this keep the original value, i.e. = 5? On Wed, Aug 28, 2019 at 10:12 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > Suggested-by: Ren-Pei Zeng <kamesan@google.com> > Signed-off-by: Jacopo Mondi <jacopo@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? > > --- > 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, > -- > 2.23.0 > >
Hello Ren-Pei, 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. Why would you prefer to keep a value of 5 here ? I'm not opposed to it, just for my better understanding! Thanks j > > On Wed, Aug 28, 2019 at 10:12 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > Suggested-by: Ren-Pei Zeng <kamesan@google.com> > > Signed-off-by: Jacopo Mondi <jacopo@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? > > > > --- > > 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, > > -- > > 2.23.0 > > > >
Thanks Jacopo for clarifying. I misthought this value will limit the available buffers on the consumer side (chrome capture stack in this case), but it should not. I'm not familiar with how frame control affects this, and the value is for this moment, so it's up to you. Thanks! On Wed, Aug 28, 2019 at 11:41 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hello Ren-Pei, > > 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. > > Why would you prefer to keep a value of 5 here ? I'm not opposed to > it, just for my better understanding! > > Thanks > j > > > > > On Wed, Aug 28, 2019 at 10:12 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > > Suggested-by: Ren-Pei Zeng <kamesan@google.com> > > > Signed-off-by: Jacopo Mondi <jacopo@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? > > > > > > --- > > > 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, > > > -- > > > 2.23.0 > > > > > > >
Hi Ren-Pei On Thu, Aug 29, 2019 at 12:23:57AM +0800, Ren-Pei Zeng wrote: > Thanks Jacopo for clarifying. I misthought this value will limit the > available buffers on the consumer side (chrome capture stack in this case), > but it should not. > I'm not familiar with how frame control affects this, and the value is for > this moment, so it's up to you. I'm not actually sure either here :) My understanding comes from reading the android.sync.frameNumber documentation https://jmondi.org/android_metadata_tags/docs.html#dynamic_android.sync.frameNumber but you have more experience than me in android/cros related things, so please have a look there too :) Thanks j > > Thanks! > > On Wed, Aug 28, 2019 at 11:41 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hello Ren-Pei, > > > > 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. > > > > Why would you prefer to keep a value of 5 here ? I'm not opposed to > > it, just for my better understanding! > > > > Thanks > > j > > > > > > > > On Wed, Aug 28, 2019 at 10:12 PM Jacopo Mondi <jacopo@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@ideasonboard.com> > > > > Suggested-by: Ren-Pei Zeng <kamesan@google.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@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? > > > > > > > > --- > > > > 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, > > > > -- > > > > 2.23.0 > > > > > > > > > >
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@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@ideasonboard.com> > > > Suggested-by: Ren-Pei Zeng <kamesan@google.com> > > > Signed-off-by: Jacopo Mondi <jacopo@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,
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@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@ideasonboard.com> > >>> Suggested-by: Ren-Pei Zeng <kamesan@google.com> > >>> Signed-off-by: Jacopo Mondi <jacopo@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@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,
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,
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@ideasonboard.com> Suggested-by: Ren-Pei Zeng <kamesan@google.com> Signed-off-by: Jacopo Mondi <jacopo@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? --- src/android/camera_device.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) -- 2.23.0