| Message ID | 20210715153241.63971-4-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On 7/15/21 9:02 PM, Jacopo Mondi wrote: > The validation of RAW stream support is performed in two different > places: > > - At initializeStreamConfigurations() time, by verifying that the > libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format > and ensuring the Camera successfully validates it > - As initializeStaticMetadata() time by generating a CameraConfiguration > for the Raw stream role and ensuring it is a Raw format with a 16 bit > depth > > The first check is used to build the list of supported Raw stream > resolutions. The latter is used to register the > ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability. > > As building the list of supported Raw streams doesn't serve any > purpose if the RAW capability is nor registered, centralize the Raw s/nor/not > format support verification at initializeStreamConfigurations() time by > ensuring the supported format is a Raw one with a 16 bit depth. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_capabilities.cpp | 23 ++++++++--------------- > src/android/camera_capabilities.h | 1 + > 2 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 15e54192adff..9e2714f132c4 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > camera_ = camera; > orientation_ = orientation; > facing_ = facing; > + rawStreamAvailable_ = false; > > /* Acquire the camera and initialize available stream configurations. */ > int ret = camera_->acquire(); > @@ -324,11 +325,14 @@ int CameraCapabilities::initializeStreamConfigurations() > > std::vector<Size> resolutions; > const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat); > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && > + info.bitsPerPixel == 16) { > + rawStreamAvailable_ = true; Nice, to have this in one place now. Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > resolutions = initializeRawResolutions(mappedFormat); > - else > + } else { > resolutions = initializeYUVResolutions(mappedFormat, > cameraResolutions); > + } > > for (const Size &res : resolutions) { > streamConfigurations_.push_back({ res, androidFormat }); > @@ -866,22 +870,11 @@ int CameraCapabilities::initializeStaticMetadata() > }; > > /* Report if camera supports RAW. */ > - bool rawStreamAvailable = false; > - std::unique_ptr<CameraConfiguration> cameraConfig = > - camera_->generateConfiguration({ StreamRole::Raw }); > - if (cameraConfig && !cameraConfig->empty()) { > - const PixelFormatInfo &info = > - PixelFormatInfo::info(cameraConfig->at(0).pixelFormat); > - /* Only advertise RAW support if RAW16 is possible. */ > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && > - info.bitsPerPixel == 16) { > - rawStreamAvailable = true; > + if (rawStreamAvailable_) > availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW); > - } > - } > > /* Number of { RAW, YUV, JPEG } supported output streams */ > - int32_t numOutStreams[] = { rawStreamAvailable, 2, 1 }; > + int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 }; > staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS, > numOutStreams); > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h > index e7aa46c0a689..42a976d3b482 100644 > --- a/src/android/camera_capabilities.h > +++ b/src/android/camera_capabilities.h > @@ -55,6 +55,7 @@ private: > > int facing_; > int orientation_; > + bool rawStreamAvailable_; > > std::vector<Camera3StreamConfiguration> streamConfigurations_; > std::map<int, libcamera::PixelFormat> formatsMap_;
Hi Jacopo, Thank you for the patch. On Thu, Jul 15, 2021 at 05:32:41PM +0200, Jacopo Mondi wrote: > The validation of RAW stream support is performed in two different > places: > > - At initializeStreamConfigurations() time, by verifying that the > libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format > and ensuring the Camera successfully validates it > - As initializeStaticMetadata() time by generating a CameraConfiguration > for the Raw stream role and ensuring it is a Raw format with a 16 bit > depth > > The first check is used to build the list of supported Raw stream > resolutions. The latter is used to register the > ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability. > > As building the list of supported Raw streams doesn't serve any > purpose if the RAW capability is nor registered, centralize the Raw > format support verification at initializeStreamConfigurations() time by > ensuring the supported format is a Raw one with a 16 bit depth. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_capabilities.cpp | 23 ++++++++--------------- > src/android/camera_capabilities.h | 1 + > 2 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 15e54192adff..9e2714f132c4 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > camera_ = camera; > orientation_ = orientation; > facing_ = facing; > + rawStreamAvailable_ = false; > > /* Acquire the camera and initialize available stream configurations. */ > int ret = camera_->acquire(); > @@ -324,11 +325,14 @@ int CameraCapabilities::initializeStreamConfigurations() > > std::vector<Size> resolutions; > const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat); > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && > + info.bitsPerPixel == 16) { > + rawStreamAvailable_ = true; > resolutions = initializeRawResolutions(mappedFormat); > - else > + } else { Shouldn't you skip here the raw formats that have other bpp than 16 ? And probably the RGB formats too ? > resolutions = initializeYUVResolutions(mappedFormat, > cameraResolutions); > + } > > for (const Size &res : resolutions) { > streamConfigurations_.push_back({ res, androidFormat }); > @@ -866,22 +870,11 @@ int CameraCapabilities::initializeStaticMetadata() > }; > > /* Report if camera supports RAW. */ > - bool rawStreamAvailable = false; > - std::unique_ptr<CameraConfiguration> cameraConfig = > - camera_->generateConfiguration({ StreamRole::Raw }); > - if (cameraConfig && !cameraConfig->empty()) { > - const PixelFormatInfo &info = > - PixelFormatInfo::info(cameraConfig->at(0).pixelFormat); > - /* Only advertise RAW support if RAW16 is possible. */ > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && > - info.bitsPerPixel == 16) { > - rawStreamAvailable = true; > + if (rawStreamAvailable_) > availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW); > - } > - } > > /* Number of { RAW, YUV, JPEG } supported output streams */ > - int32_t numOutStreams[] = { rawStreamAvailable, 2, 1 }; > + int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 }; > staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS, > numOutStreams); > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h > index e7aa46c0a689..42a976d3b482 100644 > --- a/src/android/camera_capabilities.h > +++ b/src/android/camera_capabilities.h > @@ -55,6 +55,7 @@ private: > > int facing_; > int orientation_; > + bool rawStreamAvailable_; > > std::vector<Camera3StreamConfiguration> streamConfigurations_; > std::map<int, libcamera::PixelFormat> formatsMap_;
Hi Laurent, On Fri, Jul 23, 2021 at 04:43:19PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Jul 15, 2021 at 05:32:41PM +0200, Jacopo Mondi wrote: > > The validation of RAW stream support is performed in two different > > places: > > > > - At initializeStreamConfigurations() time, by verifying that the > > libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format > > and ensuring the Camera successfully validates it > > - As initializeStaticMetadata() time by generating a CameraConfiguration > > for the Raw stream role and ensuring it is a Raw format with a 16 bit > > depth > > > > The first check is used to build the list of supported Raw stream > > resolutions. The latter is used to register the > > ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability. > > > > As building the list of supported Raw streams doesn't serve any > > purpose if the RAW capability is nor registered, centralize the Raw > > format support verification at initializeStreamConfigurations() time by > > ensuring the supported format is a Raw one with a 16 bit depth. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_capabilities.cpp | 23 ++++++++--------------- > > src/android/camera_capabilities.h | 1 + > > 2 files changed, 9 insertions(+), 15 deletions(-) > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > index 15e54192adff..9e2714f132c4 100644 > > --- a/src/android/camera_capabilities.cpp > > +++ b/src/android/camera_capabilities.cpp > > @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > > camera_ = camera; > > orientation_ = orientation; > > facing_ = facing; > > + rawStreamAvailable_ = false; > > > > /* Acquire the camera and initialize available stream configurations. */ > > int ret = camera_->acquire(); > > @@ -324,11 +325,14 @@ int CameraCapabilities::initializeStreamConfigurations() > > > > std::vector<Size> resolutions; > > const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat); > > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) > > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && > > + info.bitsPerPixel == 16) { > > + rawStreamAvailable_ = true; > > resolutions = initializeRawResolutions(mappedFormat); > > - else > > + } else { > > Shouldn't you skip here the raw formats that have other bpp than 16 ? Correct! Raw !16bpp formats should not be enumerated in the else branch > And probably the RGB formats too ? Should we ? The initialization walks the camera3FormatsMap where the formats mapping is defined. If for any reason (processed) RGB formats are listed there (in example to support IMPLEMENTATION_DEFINED) why would we ignore them ? Thanks j > > > resolutions = initializeYUVResolutions(mappedFormat, > > cameraResolutions); > > + } > > > > for (const Size &res : resolutions) { > > streamConfigurations_.push_back({ res, androidFormat }); > > @@ -866,22 +870,11 @@ int CameraCapabilities::initializeStaticMetadata() > > }; > > > > /* Report if camera supports RAW. */ > > - bool rawStreamAvailable = false; > > - std::unique_ptr<CameraConfiguration> cameraConfig = > > - camera_->generateConfiguration({ StreamRole::Raw }); > > - if (cameraConfig && !cameraConfig->empty()) { > > - const PixelFormatInfo &info = > > - PixelFormatInfo::info(cameraConfig->at(0).pixelFormat); > > - /* Only advertise RAW support if RAW16 is possible. */ > > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && > > - info.bitsPerPixel == 16) { > > - rawStreamAvailable = true; > > + if (rawStreamAvailable_) > > availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW); > > - } > > - } > > > > /* Number of { RAW, YUV, JPEG } supported output streams */ > > - int32_t numOutStreams[] = { rawStreamAvailable, 2, 1 }; > > + int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 }; > > staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS, > > numOutStreams); > > > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h > > index e7aa46c0a689..42a976d3b482 100644 > > --- a/src/android/camera_capabilities.h > > +++ b/src/android/camera_capabilities.h > > @@ -55,6 +55,7 @@ private: > > > > int facing_; > > int orientation_; > > + bool rawStreamAvailable_; > > > > std::vector<Camera3StreamConfiguration> streamConfigurations_; > > std::map<int, libcamera::PixelFormat> formatsMap_; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Jul 26, 2021 at 10:51:28AM +0200, Jacopo Mondi wrote: > On Fri, Jul 23, 2021 at 04:43:19PM +0300, Laurent Pinchart wrote: > > On Thu, Jul 15, 2021 at 05:32:41PM +0200, Jacopo Mondi wrote: > > > The validation of RAW stream support is performed in two different > > > places: > > > > > > - At initializeStreamConfigurations() time, by verifying that the > > > libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format > > > and ensuring the Camera successfully validates it > > > - As initializeStaticMetadata() time by generating a CameraConfiguration > > > for the Raw stream role and ensuring it is a Raw format with a 16 bit > > > depth > > > > > > The first check is used to build the list of supported Raw stream > > > resolutions. The latter is used to register the > > > ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability. > > > > > > As building the list of supported Raw streams doesn't serve any > > > purpose if the RAW capability is nor registered, centralize the Raw > > > format support verification at initializeStreamConfigurations() time by > > > ensuring the supported format is a Raw one with a 16 bit depth. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_capabilities.cpp | 23 ++++++++--------------- > > > src/android/camera_capabilities.h | 1 + > > > 2 files changed, 9 insertions(+), 15 deletions(-) > > > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > > index 15e54192adff..9e2714f132c4 100644 > > > --- a/src/android/camera_capabilities.cpp > > > +++ b/src/android/camera_capabilities.cpp > > > @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > > > camera_ = camera; > > > orientation_ = orientation; > > > facing_ = facing; > > > + rawStreamAvailable_ = false; > > > > > > /* Acquire the camera and initialize available stream configurations. */ > > > int ret = camera_->acquire(); > > > @@ -324,11 +325,14 @@ int CameraCapabilities::initializeStreamConfigurations() > > > > > > std::vector<Size> resolutions; > > > const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat); > > > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) > > > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && > > > + info.bitsPerPixel == 16) { > > > + rawStreamAvailable_ = true; > > > resolutions = initializeRawResolutions(mappedFormat); > > > - else > > > + } else { > > > > Shouldn't you skip here the raw formats that have other bpp than 16 ? > > Correct! Raw !16bpp formats should not be enumerated in the else > branch > > > And probably the RGB formats too ? > > Should we ? The initialization walks the camera3FormatsMap where the > formats mapping is defined. If for any reason (processed) RGB formats are > listed there (in example to support IMPLEMENTATION_DEFINED) why would > we ignore them ? Indeed. That's probably unlikely, but it may happen. However, calling initializeYUVResolutions() for an RGB format sounds weird. > > > resolutions = initializeYUVResolutions(mappedFormat, > > > cameraResolutions); > > > + } > > > > > > for (const Size &res : resolutions) { > > > streamConfigurations_.push_back({ res, androidFormat }); > > > @@ -866,22 +870,11 @@ int CameraCapabilities::initializeStaticMetadata() > > > }; > > > > > > /* Report if camera supports RAW. */ > > > - bool rawStreamAvailable = false; > > > - std::unique_ptr<CameraConfiguration> cameraConfig = > > > - camera_->generateConfiguration({ StreamRole::Raw }); > > > - if (cameraConfig && !cameraConfig->empty()) { > > > - const PixelFormatInfo &info = > > > - PixelFormatInfo::info(cameraConfig->at(0).pixelFormat); > > > - /* Only advertise RAW support if RAW16 is possible. */ > > > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && > > > - info.bitsPerPixel == 16) { > > > - rawStreamAvailable = true; > > > + if (rawStreamAvailable_) > > > availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW); > > > - } > > > - } > > > > > > /* Number of { RAW, YUV, JPEG } supported output streams */ > > > - int32_t numOutStreams[] = { rawStreamAvailable, 2, 1 }; > > > + int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 }; > > > staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS, > > > numOutStreams); > > > > > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h > > > index e7aa46c0a689..42a976d3b482 100644 > > > --- a/src/android/camera_capabilities.h > > > +++ b/src/android/camera_capabilities.h > > > @@ -55,6 +55,7 @@ private: > > > > > > int facing_; > > > int orientation_; > > > + bool rawStreamAvailable_; > > > > > > std::vector<Camera3StreamConfiguration> streamConfigurations_; > > > std::map<int, libcamera::PixelFormat> formatsMap_;
diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp index 15e54192adff..9e2714f132c4 100644 --- a/src/android/camera_capabilities.cpp +++ b/src/android/camera_capabilities.cpp @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, camera_ = camera; orientation_ = orientation; facing_ = facing; + rawStreamAvailable_ = false; /* Acquire the camera and initialize available stream configurations. */ int ret = camera_->acquire(); @@ -324,11 +325,14 @@ int CameraCapabilities::initializeStreamConfigurations() std::vector<Size> resolutions; const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat); - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && + info.bitsPerPixel == 16) { + rawStreamAvailable_ = true; resolutions = initializeRawResolutions(mappedFormat); - else + } else { resolutions = initializeYUVResolutions(mappedFormat, cameraResolutions); + } for (const Size &res : resolutions) { streamConfigurations_.push_back({ res, androidFormat }); @@ -866,22 +870,11 @@ int CameraCapabilities::initializeStaticMetadata() }; /* Report if camera supports RAW. */ - bool rawStreamAvailable = false; - std::unique_ptr<CameraConfiguration> cameraConfig = - camera_->generateConfiguration({ StreamRole::Raw }); - if (cameraConfig && !cameraConfig->empty()) { - const PixelFormatInfo &info = - PixelFormatInfo::info(cameraConfig->at(0).pixelFormat); - /* Only advertise RAW support if RAW16 is possible. */ - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW && - info.bitsPerPixel == 16) { - rawStreamAvailable = true; + if (rawStreamAvailable_) availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW); - } - } /* Number of { RAW, YUV, JPEG } supported output streams */ - int32_t numOutStreams[] = { rawStreamAvailable, 2, 1 }; + int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 }; staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS, numOutStreams); diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h index e7aa46c0a689..42a976d3b482 100644 --- a/src/android/camera_capabilities.h +++ b/src/android/camera_capabilities.h @@ -55,6 +55,7 @@ private: int facing_; int orientation_; + bool rawStreamAvailable_; std::vector<Camera3StreamConfiguration> streamConfigurations_; std::map<int, libcamera::PixelFormat> formatsMap_;
The validation of RAW stream support is performed in two different places: - At initializeStreamConfigurations() time, by verifying that the libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format and ensuring the Camera successfully validates it - As initializeStaticMetadata() time by generating a CameraConfiguration for the Raw stream role and ensuring it is a Raw format with a 16 bit depth The first check is used to build the list of supported Raw stream resolutions. The latter is used to register the ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability. As building the list of supported Raw streams doesn't serve any purpose if the RAW capability is nor registered, centralize the Raw format support verification at initializeStreamConfigurations() time by ensuring the supported format is a Raw one with a 16 bit depth. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_capabilities.cpp | 23 ++++++++--------------- src/android/camera_capabilities.h | 1 + 2 files changed, 9 insertions(+), 15 deletions(-)