| Message ID | 20201009122101.73858-8-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 09/10/2020 13:20, Jacopo Mondi wrote: > Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property > inspecting what the value returned by the pipeline handler > through the properties::draft::PipelineMaxDepth libcamera property. > > Report the result metadata ANDROID_REQUEST_PIPELINE_DEPTH that > reports the processing stage a frame went through by inspecting the > controls::draft::PipelineDepth libcamera control reported in the > completed Request. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 28 ++++++++++++++++++++++------ > src/android/camera_device.h | 3 ++- > 2 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 0a94c1ae17ac..bccec358ea13 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -12,6 +12,7 @@ > #include <tuple> > #include <vector> > > +#include <libcamera/control_ids.h> > #include <libcamera/controls.h> > #include <libcamera/formats.h> > #include <libcamera/property_ids.h> > @@ -576,6 +577,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > return nullptr; > } > > + const ControlList &cameraProperties = camera_->properties(); > + > /* Color correction static metadata. */ > std::vector<uint8_t> aberrationModes = { > ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF, > @@ -867,9 +870,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT, > &partialResultCount, 1); > > - uint8_t maxPipelineDepth = 2; > - staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, > - &maxPipelineDepth, 1); > + if (cameraProperties.contains(properties::draft::PipelineMaxDepth)) { > + uint8_t maxPipelineDepth = > + cameraProperties.get<int32_t>(properties::draft::PipelineMaxDepth); > + staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, > + &maxPipelineDepth, 1); > + } I assume if the pipeline handler doesn't specify a max depth, and one doesn't get added here, that's not a problem for android. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > /* LIMITED does not support reprocessing. */ > uint32_t maxNumInputStreams = 0; > @@ -1480,7 +1486,8 @@ void CameraDevice::requestComplete(Request *request) > * pipeline handlers) timestamp in the Request itself. > */ > FrameBuffer *buffer = buffers.begin()->second; > - resultMetadata = getResultMetadata(descriptor->frameNumber, > + resultMetadata = getResultMetadata(request->metadata(), > + descriptor->frameNumber, > buffer->metadata().timestamp); > > /* Handle any JPEG compression. */ > @@ -1600,7 +1607,8 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) > * Produce a set of fixed result metadata. > */ > std::unique_ptr<CameraMetadata> > -CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > +CameraDevice::getResultMetadata(ControlList &metadata, > + [[maybe_unused]] int frame_number, > int64_t timestamp) > { > /* > @@ -1608,7 +1616,7 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > * Currently: 18 entries, 62 bytes > */ > std::unique_ptr<CameraMetadata> resultMetadata = > - std::make_unique<CameraMetadata>(18, 62); > + std::make_unique<CameraMetadata>(19, 63); > if (!resultMetadata->isValid()) { > LOG(HAL, Error) << "Failed to allocate static metadata"; > return nullptr; > @@ -1658,6 +1666,14 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER, > &scene_flicker, 1); > > + /* Add metadata tags reported by libcamera. */ > + if (metadata.contains(controls::draft::PipelineDepth)) { > + uint8_t pipeline_depth = > + metadata.get<int32_t>(controls::draft::PipelineDepth); > + resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, > + &pipeline_depth, 1); > + } > + > /* > * Return the result metadata pack even is not valid: get() will return > * nullptr. > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 777d1a35e801..8cf1006c0840 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -102,7 +102,8 @@ private: > void notifyError(uint32_t frameNumber, camera3_stream_t *stream); > CameraMetadata *requestTemplatePreview(); > libcamera::PixelFormat toPixelFormat(int format); > - std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number, > + std::unique_ptr<CameraMetadata> getResultMetadata(libcamera::ControlList &metadata, > + int frame_number, > int64_t timestamp); > > unsigned int id_; >
Hi Jacopo, On 09/10/2020 14:19, Jacopo Mondi wrote: > Hi Kieran, > > On Fri, Oct 09, 2020 at 01:44:19PM +0100, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 09/10/2020 13:20, Jacopo Mondi wrote: >>> Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property >>> inspecting what the value returned by the pipeline handler >>> through the properties::draft::PipelineMaxDepth libcamera property. >>> >>> Report the result metadata ANDROID_REQUEST_PIPELINE_DEPTH that >>> reports the processing stage a frame went through by inspecting the >>> controls::draft::PipelineDepth libcamera control reported in the >>> completed Request. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> src/android/camera_device.cpp | 28 ++++++++++++++++++++++------ >>> src/android/camera_device.h | 3 ++- >>> 2 files changed, 24 insertions(+), 7 deletions(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index 0a94c1ae17ac..bccec358ea13 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -12,6 +12,7 @@ >>> #include <tuple> >>> #include <vector> >>> >>> +#include <libcamera/control_ids.h> >>> #include <libcamera/controls.h> >>> #include <libcamera/formats.h> >>> #include <libcamera/property_ids.h> >>> @@ -576,6 +577,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() >>> return nullptr; >>> } >>> >>> + const ControlList &cameraProperties = camera_->properties(); >>> + >>> /* Color correction static metadata. */ >>> std::vector<uint8_t> aberrationModes = { >>> ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF, >>> @@ -867,9 +870,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() >>> staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT, >>> &partialResultCount, 1); >>> >>> - uint8_t maxPipelineDepth = 2; >>> - staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, >>> - &maxPipelineDepth, 1); >>> + if (cameraProperties.contains(properties::draft::PipelineMaxDepth)) { >>> + uint8_t maxPipelineDepth = >>> + cameraProperties.get<int32_t>(properties::draft::PipelineMaxDepth); >>> + staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, >>> + &maxPipelineDepth, 1); >>> + } >> >> I assume if the pipeline handler doesn't specify a max depth, and one >> doesn't get added here, that's not a problem for android. > > That's the right question to ask! > > By points: > 1) Is this is problem for Android: it depends, Do you want to be > compliant with the Android requirements for the reported Android HW > level ? Yes, that might be a problem, depending on which > property/control the pipeline handler does not report. > > 2) What policy do we want ? It boils down to the concept of > "android-aware" pipeline handler. The Android HAL cannot, in my > opinion, be fully Android-compliant without support in the pipeline > handler. It cannot produce 'default' values for properties, controls > and metadata without breaking compliance with CTS and other tests. > The pipeline handler needs to report and handle the values required > by Android, I don't see many ways around it :) To me, I see that we would handle this with the "Pipeline Validation Tool" *1 We can simply add tests there for any android specific requirements. then a pipeline can be 'passed' as android capabable if it implements and passes those tests, and - simply not otherwise. *1 - ah yes the fabled tool that we really should start ... - Lets discuss next week, and see if we can get at least a small one started!, then it's easier to continue if it exists ;-) -- Kieran > >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> >>> /* LIMITED does not support reprocessing. */ >>> uint32_t maxNumInputStreams = 0; >>> @@ -1480,7 +1486,8 @@ void CameraDevice::requestComplete(Request *request) >>> * pipeline handlers) timestamp in the Request itself. >>> */ >>> FrameBuffer *buffer = buffers.begin()->second; >>> - resultMetadata = getResultMetadata(descriptor->frameNumber, >>> + resultMetadata = getResultMetadata(request->metadata(), >>> + descriptor->frameNumber, >>> buffer->metadata().timestamp); >>> >>> /* Handle any JPEG compression. */ >>> @@ -1600,7 +1607,8 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) >>> * Produce a set of fixed result metadata. >>> */ >>> std::unique_ptr<CameraMetadata> >>> -CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, >>> +CameraDevice::getResultMetadata(ControlList &metadata, >>> + [[maybe_unused]] int frame_number, >>> int64_t timestamp) >>> { >>> /* >>> @@ -1608,7 +1616,7 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, >>> * Currently: 18 entries, 62 bytes >>> */ >>> std::unique_ptr<CameraMetadata> resultMetadata = >>> - std::make_unique<CameraMetadata>(18, 62); >>> + std::make_unique<CameraMetadata>(19, 63); >>> if (!resultMetadata->isValid()) { >>> LOG(HAL, Error) << "Failed to allocate static metadata"; >>> return nullptr; >>> @@ -1658,6 +1666,14 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, >>> resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER, >>> &scene_flicker, 1); >>> >>> + /* Add metadata tags reported by libcamera. */ >>> + if (metadata.contains(controls::draft::PipelineDepth)) { >>> + uint8_t pipeline_depth = >>> + metadata.get<int32_t>(controls::draft::PipelineDepth); >>> + resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, >>> + &pipeline_depth, 1); >>> + } >>> + >>> /* >>> * Return the result metadata pack even is not valid: get() will return >>> * nullptr. >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >>> index 777d1a35e801..8cf1006c0840 100644 >>> --- a/src/android/camera_device.h >>> +++ b/src/android/camera_device.h >>> @@ -102,7 +102,8 @@ private: >>> void notifyError(uint32_t frameNumber, camera3_stream_t *stream); >>> CameraMetadata *requestTemplatePreview(); >>> libcamera::PixelFormat toPixelFormat(int format); >>> - std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number, >>> + std::unique_ptr<CameraMetadata> getResultMetadata(libcamera::ControlList &metadata, >>> + int frame_number, >>> int64_t timestamp); >>> >>> unsigned int id_; >>> >> >> -- >> Regards >> -- >> Kieran
Hi Kieran, On Fri, Oct 09, 2020 at 01:44:19PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 09/10/2020 13:20, Jacopo Mondi wrote: > > Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property > > inspecting what the value returned by the pipeline handler > > through the properties::draft::PipelineMaxDepth libcamera property. > > > > Report the result metadata ANDROID_REQUEST_PIPELINE_DEPTH that > > reports the processing stage a frame went through by inspecting the > > controls::draft::PipelineDepth libcamera control reported in the > > completed Request. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 28 ++++++++++++++++++++++------ > > src/android/camera_device.h | 3 ++- > > 2 files changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 0a94c1ae17ac..bccec358ea13 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -12,6 +12,7 @@ > > #include <tuple> > > #include <vector> > > > > +#include <libcamera/control_ids.h> > > #include <libcamera/controls.h> > > #include <libcamera/formats.h> > > #include <libcamera/property_ids.h> > > @@ -576,6 +577,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > return nullptr; > > } > > > > + const ControlList &cameraProperties = camera_->properties(); > > + > > /* Color correction static metadata. */ > > std::vector<uint8_t> aberrationModes = { > > ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF, > > @@ -867,9 +870,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT, > > &partialResultCount, 1); > > > > - uint8_t maxPipelineDepth = 2; > > - staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, > > - &maxPipelineDepth, 1); > > + if (cameraProperties.contains(properties::draft::PipelineMaxDepth)) { > > + uint8_t maxPipelineDepth = > > + cameraProperties.get<int32_t>(properties::draft::PipelineMaxDepth); > > + staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, > > + &maxPipelineDepth, 1); > > + } > > I assume if the pipeline handler doesn't specify a max depth, and one > doesn't get added here, that's not a problem for android. That's the right question to ask! By points: 1) Is this is problem for Android: it depends, Do you want to be compliant with the Android requirements for the reported Android HW level ? Yes, that might be a problem, depending on which property/control the pipeline handler does not report. 2) What policy do we want ? It boils down to the concept of "android-aware" pipeline handler. The Android HAL cannot, in my opinion, be fully Android-compliant without support in the pipeline handler. It cannot produce 'default' values for properties, controls and metadata without breaking compliance with CTS and other tests. The pipeline handler needs to report and handle the values required by Android, I don't see many ways around it :) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > /* LIMITED does not support reprocessing. */ > > uint32_t maxNumInputStreams = 0; > > @@ -1480,7 +1486,8 @@ void CameraDevice::requestComplete(Request *request) > > * pipeline handlers) timestamp in the Request itself. > > */ > > FrameBuffer *buffer = buffers.begin()->second; > > - resultMetadata = getResultMetadata(descriptor->frameNumber, > > + resultMetadata = getResultMetadata(request->metadata(), > > + descriptor->frameNumber, > > buffer->metadata().timestamp); > > > > /* Handle any JPEG compression. */ > > @@ -1600,7 +1607,8 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) > > * Produce a set of fixed result metadata. > > */ > > std::unique_ptr<CameraMetadata> > > -CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > > +CameraDevice::getResultMetadata(ControlList &metadata, > > + [[maybe_unused]] int frame_number, > > int64_t timestamp) > > { > > /* > > @@ -1608,7 +1616,7 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > > * Currently: 18 entries, 62 bytes > > */ > > std::unique_ptr<CameraMetadata> resultMetadata = > > - std::make_unique<CameraMetadata>(18, 62); > > + std::make_unique<CameraMetadata>(19, 63); > > if (!resultMetadata->isValid()) { > > LOG(HAL, Error) << "Failed to allocate static metadata"; > > return nullptr; > > @@ -1658,6 +1666,14 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > > resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER, > > &scene_flicker, 1); > > > > + /* Add metadata tags reported by libcamera. */ > > + if (metadata.contains(controls::draft::PipelineDepth)) { > > + uint8_t pipeline_depth = > > + metadata.get<int32_t>(controls::draft::PipelineDepth); > > + resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, > > + &pipeline_depth, 1); > > + } > > + > > /* > > * Return the result metadata pack even is not valid: get() will return > > * nullptr. > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 777d1a35e801..8cf1006c0840 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -102,7 +102,8 @@ private: > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream); > > CameraMetadata *requestTemplatePreview(); > > libcamera::PixelFormat toPixelFormat(int format); > > - std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number, > > + std::unique_ptr<CameraMetadata> getResultMetadata(libcamera::ControlList &metadata, > > + int frame_number, > > int64_t timestamp); > > > > unsigned int id_; > > > > -- > Regards > -- > Kieran
Hi Kieran On Fri, Oct 09, 2020 at 02:17:56PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 09/10/2020 14:19, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Fri, Oct 09, 2020 at 01:44:19PM +0100, Kieran Bingham wrote: > >> Hi Jacopo, > >> > >> On 09/10/2020 13:20, Jacopo Mondi wrote: > >>> Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property > >>> inspecting what the value returned by the pipeline handler > >>> through the properties::draft::PipelineMaxDepth libcamera property. > >>> > >>> Report the result metadata ANDROID_REQUEST_PIPELINE_DEPTH that > >>> reports the processing stage a frame went through by inspecting the > >>> controls::draft::PipelineDepth libcamera control reported in the > >>> completed Request. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> src/android/camera_device.cpp | 28 ++++++++++++++++++++++------ > >>> src/android/camera_device.h | 3 ++- > >>> 2 files changed, 24 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>> index 0a94c1ae17ac..bccec358ea13 100644 > >>> --- a/src/android/camera_device.cpp > >>> +++ b/src/android/camera_device.cpp > >>> @@ -12,6 +12,7 @@ > >>> #include <tuple> > >>> #include <vector> > >>> > >>> +#include <libcamera/control_ids.h> > >>> #include <libcamera/controls.h> > >>> #include <libcamera/formats.h> > >>> #include <libcamera/property_ids.h> > >>> @@ -576,6 +577,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > >>> return nullptr; > >>> } > >>> > >>> + const ControlList &cameraProperties = camera_->properties(); > >>> + > >>> /* Color correction static metadata. */ > >>> std::vector<uint8_t> aberrationModes = { > >>> ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF, > >>> @@ -867,9 +870,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > >>> staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT, > >>> &partialResultCount, 1); > >>> > >>> - uint8_t maxPipelineDepth = 2; > >>> - staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, > >>> - &maxPipelineDepth, 1); > >>> + if (cameraProperties.contains(properties::draft::PipelineMaxDepth)) { > >>> + uint8_t maxPipelineDepth = > >>> + cameraProperties.get<int32_t>(properties::draft::PipelineMaxDepth); > >>> + staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, > >>> + &maxPipelineDepth, 1); > >>> + } > >> > >> I assume if the pipeline handler doesn't specify a max depth, and one > >> doesn't get added here, that's not a problem for android. > > > > That's the right question to ask! > > > > By points: > > 1) Is this is problem for Android: it depends, Do you want to be > > compliant with the Android requirements for the reported Android HW > > level ? Yes, that might be a problem, depending on which > > property/control the pipeline handler does not report. > > > > 2) What policy do we want ? It boils down to the concept of > > "android-aware" pipeline handler. The Android HAL cannot, in my > > opinion, be fully Android-compliant without support in the pipeline > > handler. It cannot produce 'default' values for properties, controls > > and metadata without breaking compliance with CTS and other tests. > > The pipeline handler needs to report and handle the values required > > by Android, I don't see many ways around it :) > > To me, I see that we would handle this with the > "Pipeline Validation Tool" *1 Without digressing for too long, compliance with Android is not something we should try to verify ourselves imho. Versioning would be hell, just to name one issue that comes to mind. There's plenty of tools provided by Android for verify compliance. We should better focus on libcamera features :) > > We can simply add tests there for any android specific requirements. > If only they were 'simple' to express, version and update. It's just too complex imho. Look at the CTS CameraTest size in example > then a pipeline can be 'passed' as android capabable if it implements > and passes those tests, and - simply not otherwise. > > > *1 - ah yes the fabled tool that we really should start ... > - Lets discuss next week, and see if we can get at least a small one > started!, then it's easier to continue if it exists ;-) > > -- > Kieran > > > > > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> > >>> /* LIMITED does not support reprocessing. */ > >>> uint32_t maxNumInputStreams = 0; > >>> @@ -1480,7 +1486,8 @@ void CameraDevice::requestComplete(Request *request) > >>> * pipeline handlers) timestamp in the Request itself. > >>> */ > >>> FrameBuffer *buffer = buffers.begin()->second; > >>> - resultMetadata = getResultMetadata(descriptor->frameNumber, > >>> + resultMetadata = getResultMetadata(request->metadata(), > >>> + descriptor->frameNumber, > >>> buffer->metadata().timestamp); > >>> > >>> /* Handle any JPEG compression. */ > >>> @@ -1600,7 +1607,8 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) > >>> * Produce a set of fixed result metadata. > >>> */ > >>> std::unique_ptr<CameraMetadata> > >>> -CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > >>> +CameraDevice::getResultMetadata(ControlList &metadata, > >>> + [[maybe_unused]] int frame_number, > >>> int64_t timestamp) > >>> { > >>> /* > >>> @@ -1608,7 +1616,7 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > >>> * Currently: 18 entries, 62 bytes > >>> */ > >>> std::unique_ptr<CameraMetadata> resultMetadata = > >>> - std::make_unique<CameraMetadata>(18, 62); > >>> + std::make_unique<CameraMetadata>(19, 63); > >>> if (!resultMetadata->isValid()) { > >>> LOG(HAL, Error) << "Failed to allocate static metadata"; > >>> return nullptr; > >>> @@ -1658,6 +1666,14 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > >>> resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER, > >>> &scene_flicker, 1); > >>> > >>> + /* Add metadata tags reported by libcamera. */ > >>> + if (metadata.contains(controls::draft::PipelineDepth)) { > >>> + uint8_t pipeline_depth = > >>> + metadata.get<int32_t>(controls::draft::PipelineDepth); > >>> + resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, > >>> + &pipeline_depth, 1); > >>> + } > >>> + > >>> /* > >>> * Return the result metadata pack even is not valid: get() will return > >>> * nullptr. > >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > >>> index 777d1a35e801..8cf1006c0840 100644 > >>> --- a/src/android/camera_device.h > >>> +++ b/src/android/camera_device.h > >>> @@ -102,7 +102,8 @@ private: > >>> void notifyError(uint32_t frameNumber, camera3_stream_t *stream); > >>> CameraMetadata *requestTemplatePreview(); > >>> libcamera::PixelFormat toPixelFormat(int format); > >>> - std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number, > >>> + std::unique_ptr<CameraMetadata> getResultMetadata(libcamera::ControlList &metadata, > >>> + int frame_number, > >>> int64_t timestamp); > >>> > >>> unsigned int id_; > >>> > >> > >> -- > >> Regards > >> -- > >> Kieran > > -- > Regards > -- > Kieran
Hi Jacopo, On Fri, Oct 09, 2020 at 03:32:28PM +0200, Jacopo Mondi wrote: > On Fri, Oct 09, 2020 at 02:17:56PM +0100, Kieran Bingham wrote: > > On 09/10/2020 14:19, Jacopo Mondi wrote: > > > On Fri, Oct 09, 2020 at 01:44:19PM +0100, Kieran Bingham wrote: > > >> On 09/10/2020 13:20, Jacopo Mondi wrote: > > >>> Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property > > >>> inspecting what the value returned by the pipeline handler > > >>> through the properties::draft::PipelineMaxDepth libcamera property. > > >>> > > >>> Report the result metadata ANDROID_REQUEST_PIPELINE_DEPTH that > > >>> reports the processing stage a frame went through by inspecting the > > >>> controls::draft::PipelineDepth libcamera control reported in the > > >>> completed Request. > > >>> > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >>> --- > > >>> src/android/camera_device.cpp | 28 ++++++++++++++++++++++------ > > >>> src/android/camera_device.h | 3 ++- > > >>> 2 files changed, 24 insertions(+), 7 deletions(-) > > >>> > > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > >>> index 0a94c1ae17ac..bccec358ea13 100644 > > >>> --- a/src/android/camera_device.cpp > > >>> +++ b/src/android/camera_device.cpp > > >>> @@ -12,6 +12,7 @@ > > >>> #include <tuple> > > >>> #include <vector> > > >>> > > >>> +#include <libcamera/control_ids.h> > > >>> #include <libcamera/controls.h> > > >>> #include <libcamera/formats.h> > > >>> #include <libcamera/property_ids.h> > > >>> @@ -576,6 +577,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > >>> return nullptr; > > >>> } > > >>> > > >>> + const ControlList &cameraProperties = camera_->properties(); > > >>> + > > >>> /* Color correction static metadata. */ > > >>> std::vector<uint8_t> aberrationModes = { > > >>> ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF, > > >>> @@ -867,9 +870,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > >>> staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT, > > >>> &partialResultCount, 1); > > >>> > > >>> - uint8_t maxPipelineDepth = 2; > > >>> - staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, > > >>> - &maxPipelineDepth, 1); > > >>> + if (cameraProperties.contains(properties::draft::PipelineMaxDepth)) { > > >>> + uint8_t maxPipelineDepth = > > >>> + cameraProperties.get<int32_t>(properties::draft::PipelineMaxDepth); > > >>> + staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, > > >>> + &maxPipelineDepth, 1); > > >>> + } > > >> > > >> I assume if the pipeline handler doesn't specify a max depth, and one > > >> doesn't get added here, that's not a problem for android. > > > > > > That's the right question to ask! > > > > > > By points: > > > 1) Is this is problem for Android: it depends, Do you want to be > > > compliant with the Android requirements for the reported Android HW > > > level ? Yes, that might be a problem, depending on which > > > property/control the pipeline handler does not report. > > > > > > 2) What policy do we want ? It boils down to the concept of > > > "android-aware" pipeline handler. The Android HAL cannot, in my > > > opinion, be fully Android-compliant without support in the pipeline > > > handler. It cannot produce 'default' values for properties, controls > > > and metadata without breaking compliance with CTS and other tests. > > > The pipeline handler needs to report and handle the values required > > > by Android, I don't see many ways around it :) I don't think you'll be surprised that I disagree :-) First of all, there are features that libcamera doesn't provide today (quite a few), and that it will not provide in a foreseable future. When those features are not mandatory to support for Android (for instance the example that comes directly to my mind - and maybe not the best one - is that auto-focus isn't mandatory for fixed-lens cameras, so we could pretend they're all fixed-lens for now :-)), we can hardcode the related metadata in the HAL (for the same example, that would be hardcoding the list of supported auto-focus modes to OFF). I don't see this as an issue, and from an Android compliance point of view, the static metadata entries need to be present to report the featuer is absent, but that's it. Then, we have features we plan to support in a relatively near future, which are exposed using draft properties and controls in this series (on a side note I think some of the draft properties and controls fall in the previous category, so we don't necessarily need to introduce them right now). For those, we're taking a shortcut of making the IPU3 pipeline handler Android-aware, in order to test an end-to-end implementation. That's not the final goal, what we're aiming for is creating libcamera properties and controls that will support those features, to replace the draft ones. The pipeline handlers will then not be Android-aware, and the Android camera HAL will translate between the two. Finally, we have features that we already support, for which translation can already be implemented in the Android camera HAL (and it already is for some of those features, if not all of them). I don't see why a pipeline handler would need to be Android-aware in the long run. > > To me, I see that we would handle this with the > > "Pipeline Validation Tool" *1 > > Without digressing for too long, compliance with Android is not > something we should try to verify ourselves imho. Versioning would be > hell, just to name one issue that comes to mind. > > There's plenty of tools provided by Android for verify compliance. We > should better focus on libcamera features :) I fully agree, but we'll still need a pipeline validation tool that will be the libcamera equivalent of CTS. > > We can simply add tests there for any android specific requirements. > > If only they were 'simple' to express, version and update. It's just > too complex imho. Look at the CTS CameraTest size in example > > > then a pipeline can be 'passed' as android capabable if it implements > > and passes those tests, and - simply not otherwise. > > > > *1 - ah yes the fabled tool that we really should start ... > > - Lets discuss next week, and see if we can get at least a small one > > started!, then it's easier to continue if it exists ;-) > > > > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> > > >>> /* LIMITED does not support reprocessing. */ > > >>> uint32_t maxNumInputStreams = 0; > > >>> @@ -1480,7 +1486,8 @@ void CameraDevice::requestComplete(Request *request) > > >>> * pipeline handlers) timestamp in the Request itself. > > >>> */ > > >>> FrameBuffer *buffer = buffers.begin()->second; > > >>> - resultMetadata = getResultMetadata(descriptor->frameNumber, > > >>> + resultMetadata = getResultMetadata(request->metadata(), > > >>> + descriptor->frameNumber, > > >>> buffer->metadata().timestamp); > > >>> > > >>> /* Handle any JPEG compression. */ > > >>> @@ -1600,7 +1607,8 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) > > >>> * Produce a set of fixed result metadata. > > >>> */ > > >>> std::unique_ptr<CameraMetadata> > > >>> -CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > > >>> +CameraDevice::getResultMetadata(ControlList &metadata, > > >>> + [[maybe_unused]] int frame_number, > > >>> int64_t timestamp) > > >>> { > > >>> /* > > >>> @@ -1608,7 +1616,7 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > > >>> * Currently: 18 entries, 62 bytes > > >>> */ > > >>> std::unique_ptr<CameraMetadata> resultMetadata = > > >>> - std::make_unique<CameraMetadata>(18, 62); > > >>> + std::make_unique<CameraMetadata>(19, 63); > > >>> if (!resultMetadata->isValid()) { > > >>> LOG(HAL, Error) << "Failed to allocate static metadata"; > > >>> return nullptr; > > >>> @@ -1658,6 +1666,14 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, > > >>> resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER, > > >>> &scene_flicker, 1); > > >>> > > >>> + /* Add metadata tags reported by libcamera. */ > > >>> + if (metadata.contains(controls::draft::PipelineDepth)) { > > >>> + uint8_t pipeline_depth = > > >>> + metadata.get<int32_t>(controls::draft::PipelineDepth); > > >>> + resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, > > >>> + &pipeline_depth, 1); > > >>> + } > > >>> + > > >>> /* > > >>> * Return the result metadata pack even is not valid: get() will return > > >>> * nullptr. > > >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > >>> index 777d1a35e801..8cf1006c0840 100644 > > >>> --- a/src/android/camera_device.h > > >>> +++ b/src/android/camera_device.h > > >>> @@ -102,7 +102,8 @@ private: > > >>> void notifyError(uint32_t frameNumber, camera3_stream_t *stream); > > >>> CameraMetadata *requestTemplatePreview(); > > >>> libcamera::PixelFormat toPixelFormat(int format); > > >>> - std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number, > > >>> + std::unique_ptr<CameraMetadata> getResultMetadata(libcamera::ControlList &metadata, > > >>> + int frame_number, > > >>> int64_t timestamp); > > >>> > > >>> unsigned int id_; > > >>>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 0a94c1ae17ac..bccec358ea13 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -12,6 +12,7 @@ #include <tuple> #include <vector> +#include <libcamera/control_ids.h> #include <libcamera/controls.h> #include <libcamera/formats.h> #include <libcamera/property_ids.h> @@ -576,6 +577,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() return nullptr; } + const ControlList &cameraProperties = camera_->properties(); + /* Color correction static metadata. */ std::vector<uint8_t> aberrationModes = { ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF, @@ -867,9 +870,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT, &partialResultCount, 1); - uint8_t maxPipelineDepth = 2; - staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, - &maxPipelineDepth, 1); + if (cameraProperties.contains(properties::draft::PipelineMaxDepth)) { + uint8_t maxPipelineDepth = + cameraProperties.get<int32_t>(properties::draft::PipelineMaxDepth); + staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH, + &maxPipelineDepth, 1); + } /* LIMITED does not support reprocessing. */ uint32_t maxNumInputStreams = 0; @@ -1480,7 +1486,8 @@ void CameraDevice::requestComplete(Request *request) * pipeline handlers) timestamp in the Request itself. */ FrameBuffer *buffer = buffers.begin()->second; - resultMetadata = getResultMetadata(descriptor->frameNumber, + resultMetadata = getResultMetadata(request->metadata(), + descriptor->frameNumber, buffer->metadata().timestamp); /* Handle any JPEG compression. */ @@ -1600,7 +1607,8 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream) * Produce a set of fixed result metadata. */ std::unique_ptr<CameraMetadata> -CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, +CameraDevice::getResultMetadata(ControlList &metadata, + [[maybe_unused]] int frame_number, int64_t timestamp) { /* @@ -1608,7 +1616,7 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, * Currently: 18 entries, 62 bytes */ std::unique_ptr<CameraMetadata> resultMetadata = - std::make_unique<CameraMetadata>(18, 62); + std::make_unique<CameraMetadata>(19, 63); if (!resultMetadata->isValid()) { LOG(HAL, Error) << "Failed to allocate static metadata"; return nullptr; @@ -1658,6 +1666,14 @@ CameraDevice::getResultMetadata([[maybe_unused]] int frame_number, resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER, &scene_flicker, 1); + /* Add metadata tags reported by libcamera. */ + if (metadata.contains(controls::draft::PipelineDepth)) { + uint8_t pipeline_depth = + metadata.get<int32_t>(controls::draft::PipelineDepth); + resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, + &pipeline_depth, 1); + } + /* * Return the result metadata pack even is not valid: get() will return * nullptr. diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 777d1a35e801..8cf1006c0840 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -102,7 +102,8 @@ private: void notifyError(uint32_t frameNumber, camera3_stream_t *stream); CameraMetadata *requestTemplatePreview(); libcamera::PixelFormat toPixelFormat(int format); - std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number, + std::unique_ptr<CameraMetadata> getResultMetadata(libcamera::ControlList &metadata, + int frame_number, int64_t timestamp); unsigned int id_;
Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property inspecting what the value returned by the pipeline handler through the properties::draft::PipelineMaxDepth libcamera property. Report the result metadata ANDROID_REQUEST_PIPELINE_DEPTH that reports the processing stage a frame went through by inspecting the controls::draft::PipelineDepth libcamera control reported in the completed Request. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 28 ++++++++++++++++++++++------ src/android/camera_device.h | 3 ++- 2 files changed, 24 insertions(+), 7 deletions(-)