| Message ID | 20201211095336.40500-3-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote: > This reorders Camera3Configs before executing > CameraConfiguration::validate() to make it easier for the Camera > to satisfy the Android framework request. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Umang Jain <email@uajain.com> > --- > src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++- > 1 file changed, 108 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 7e8b2818..0c58c1c5 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -27,6 +27,8 @@ > > using namespace libcamera; > > +LOG_DECLARE_CATEGORY(HAL) > + > namespace { > > /* > @@ -144,9 +146,112 @@ struct Camera3StreamConfig { > std::vector<Camera3Stream> streams; > StreamConfiguration config; > }; > -} /* namespace */ > > -LOG_DECLARE_CATEGORY(HAL) > +/* > + * Reorder the configurations so that libcamera::Camera can accept them as much > + * as possible. The sort rule is as follows. > + * 1.) The configuration for NV12 request whose resolution is the largest. > + * 2.) The configuration for JPEG request. > + * 3.) Others. Larger resolutions and different formats are put earlier. > + */ > +void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs, > + const camera3_stream_t *jpegStream) > +{ > + const Camera3StreamConfig *jpegConfig = nullptr; > + > + std::map<PixelFormat, std::vector<const Camera3StreamConfig *>> formatToConfigs; > + for (const auto &streamConfig : unsortedConfigs) { > + if (jpegStream && !jpegConfig) { > + const auto &streams = streamConfig.streams; > + if (std::find_if(streams.begin(), streams.end(), > + [jpegStream](const auto &stream) { > + return stream.stream == jpegStream; > + }) != streams.end()) { > + jpegConfig = &streamConfig; > + continue; > + } > + } > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig); > + } > + if (jpegStream && !jpegConfig) > + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for JPEG"; > + > + for (auto &[format, streamConfigs] : formatToConfigs) { This breaks compilation on gcc 7 :-( ../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’: ../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable] for (auto &[format, streamConfigs] : formatToConfigs) { gcc 7 doesn't seem to support [[maybe_unused]] for structured bindings, so the only option that seem to work is for (auto &fmt : formatToConfigs) { auto &streamConfigs = fmt.second; > + /* Sorted by resolution. Smaller is put first. */ > + std::sort(streamConfigs.begin(), streamConfigs.end(), > + [](const auto *streamConfigA, const auto *streamConfigB) { > + const Size &sizeA = streamConfigA->config.size; > + const Size &sizeB = streamConfigB->config.size; > + return sizeA < sizeB; > + }); > + } > + > + std::vector<Camera3StreamConfig> sortedConfigs; > + sortedConfigs.reserve(unsortedConfigs.size()); > + > + /* > + * NV12 is the most prioritized format. Put the configuration with NV12 > + * and the largest resolution first. > + */ > + const auto nv12It = formatToConfigs.find(formats::NV12); > + if (nv12It != formatToConfigs.end()) { > + auto &nv12Configs = nv12It->second; > + const auto &nv12Largest = nv12Configs.back(); The use of auto makes it more difficult to read the code, as one has to look up the variables types. auto has its uses, as some types are just too long to type out explicitly (like for nv12It for instance, which would be std::map<PixelFormat, std::vector<const Camera3StreamConfig *>>::iterator), but for nv12Largest typing out const Camera3StreamConfig *nv12Largest = nv12Configs.back(); isn't too bad, and improves readability I think. This is nothing that has to be addressed now, but could we keep this in mind for the future ? > + > + /* > + * If JPEG will be created from NV12 and the size is larger than > + * the largest NV12 configurations, then put the NV12 > + * configuration for JPEG first. > + */ > + if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) { > + const Size &nv12SizeForJpeg = jpegConfig->config.size; > + const Size &nv12LargestSize = nv12Largest->config.size; > + > + if (nv12LargestSize < nv12SizeForJpeg) { > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > + sortedConfigs.push_back(std::move(*jpegConfig)); > + jpegConfig = nullptr; > + } > + } > + > + LOG(HAL, Debug) << "Insert " << nv12Largest->config.toString(); > + sortedConfigs.push_back(*nv12Largest); > + nv12Configs.pop_back(); > + > + if (nv12Configs.empty()) > + formatToConfigs.erase(nv12It); > + } > + > + /* If the configuration for JPEG is there, then put it. */ > + if (jpegConfig) { > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > + sortedConfigs.push_back(std::move(*jpegConfig)); > + jpegConfig = nullptr; > + } > + > + /* > + * Put configurations with different formats and larger resolutions > + * earlier. > + */ > + while (!formatToConfigs.empty()) { > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > + auto &configs = it->second; > + LOG(HAL, Debug) << "Insert " << configs.back()->config.toString(); > + sortedConfigs.push_back(*configs.back()); > + configs.pop_back(); > + > + if (configs.empty()) > + it = formatToConfigs.erase(it); > + else > + it++; > + } > + } > + > + ASSERT(sortedConfigs.size() == unsortedConfigs.size()); > + > + unsortedConfigs = sortedConfigs; > +} I've added a blank line here. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> And pushed with the compilation fix and the blank line. Thanks for bearing with us and the lengthy review. I'll do my best to review your future patches faster. > +} /* namespace */ > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > int flags) > @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > streamConfigs[index].streams.push_back({ jpegStream, type }); > } > > + sortCamera3StreamConfigs(streamConfigs, jpegStream); > for (const auto &streamConfig : streamConfigs) { > config_->addConfiguration(streamConfig.config); >
Hi Laurent, Thanks for reviewing. I submitted new patches addressing your comments. Regards, -Hiro On Fri, Dec 11, 2020 at 10:12 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote: > > This reorders Camera3Configs before executing > > CameraConfiguration::validate() to make it easier for the Camera > > to satisfy the Android framework request. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Umang Jain <email@uajain.com> > > --- > > src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++- > > 1 file changed, 108 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 7e8b2818..0c58c1c5 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -27,6 +27,8 @@ > > > > using namespace libcamera; > > > > +LOG_DECLARE_CATEGORY(HAL) > > + > > namespace { > > > > /* > > @@ -144,9 +146,112 @@ struct Camera3StreamConfig { > > std::vector<Camera3Stream> streams; > > StreamConfiguration config; > > }; > > -} /* namespace */ > > > > -LOG_DECLARE_CATEGORY(HAL) > > +/* > > + * Reorder the configurations so that libcamera::Camera can accept them as much > > + * as possible. The sort rule is as follows. > > + * 1.) The configuration for NV12 request whose resolution is the largest. > > + * 2.) The configuration for JPEG request. > > + * 3.) Others. Larger resolutions and different formats are put earlier. > > + */ > > +void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs, > > + const camera3_stream_t *jpegStream) > > +{ > > + const Camera3StreamConfig *jpegConfig = nullptr; > > + > > + std::map<PixelFormat, std::vector<const Camera3StreamConfig *>> formatToConfigs; > > + for (const auto &streamConfig : unsortedConfigs) { > > + if (jpegStream && !jpegConfig) { > > + const auto &streams = streamConfig.streams; > > + if (std::find_if(streams.begin(), streams.end(), > > + [jpegStream](const auto &stream) { > > + return stream.stream == jpegStream; > > + }) != streams.end()) { > > + jpegConfig = &streamConfig; > > + continue; > > + } > > + } > > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig); > > + } > > + if (jpegStream && !jpegConfig) > > + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for JPEG"; > > + > > + for (auto &[format, streamConfigs] : formatToConfigs) { > > This breaks compilation on gcc 7 :-( > > ../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’: > ../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable] > for (auto &[format, streamConfigs] : formatToConfigs) { > > gcc 7 doesn't seem to support [[maybe_unused]] for structured bindings, > so the only option that seem to work is > > for (auto &fmt : formatToConfigs) { > auto &streamConfigs = fmt.second; > > > + /* Sorted by resolution. Smaller is put first. */ > > + std::sort(streamConfigs.begin(), streamConfigs.end(), > > + [](const auto *streamConfigA, const auto *streamConfigB) { > > + const Size &sizeA = streamConfigA->config.size; > > + const Size &sizeB = streamConfigB->config.size; > > + return sizeA < sizeB; > > + }); > > + } > > + > > + std::vector<Camera3StreamConfig> sortedConfigs; > > + sortedConfigs.reserve(unsortedConfigs.size()); > > + > > + /* > > + * NV12 is the most prioritized format. Put the configuration with NV12 > > + * and the largest resolution first. > > + */ > > + const auto nv12It = formatToConfigs.find(formats::NV12); > > + if (nv12It != formatToConfigs.end()) { > > + auto &nv12Configs = nv12It->second; > > + const auto &nv12Largest = nv12Configs.back(); > > The use of auto makes it more difficult to read the code, as one has to > look up the variables types. auto has its uses, as some types are just > too long to type out explicitly (like for nv12It for instance, which > would be std::map<PixelFormat, std::vector<const Camera3StreamConfig *>>::iterator), > but for nv12Largest typing out > > const Camera3StreamConfig *nv12Largest = nv12Configs.back(); > > isn't too bad, and improves readability I think. This is nothing that > has to be addressed now, but could we keep this in mind for the future ? > > > + > > + /* > > + * If JPEG will be created from NV12 and the size is larger than > > + * the largest NV12 configurations, then put the NV12 > > + * configuration for JPEG first. > > + */ > > + if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) { > > + const Size &nv12SizeForJpeg = jpegConfig->config.size; > > + const Size &nv12LargestSize = nv12Largest->config.size; > > + > > + if (nv12LargestSize < nv12SizeForJpeg) { > > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > > + sortedConfigs.push_back(std::move(*jpegConfig)); > > + jpegConfig = nullptr; > > + } > > + } > > + > > + LOG(HAL, Debug) << "Insert " << nv12Largest->config.toString(); > > + sortedConfigs.push_back(*nv12Largest); > > + nv12Configs.pop_back(); > > + > > + if (nv12Configs.empty()) > > + formatToConfigs.erase(nv12It); > > + } > > + > > + /* If the configuration for JPEG is there, then put it. */ > > + if (jpegConfig) { > > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > > + sortedConfigs.push_back(std::move(*jpegConfig)); > > + jpegConfig = nullptr; > > + } > > + > > + /* > > + * Put configurations with different formats and larger resolutions > > + * earlier. > > + */ > > + while (!formatToConfigs.empty()) { > > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > > + auto &configs = it->second; > > + LOG(HAL, Debug) << "Insert " << configs.back()->config.toString(); > > + sortedConfigs.push_back(*configs.back()); > > + configs.pop_back(); > > + > > + if (configs.empty()) > > + it = formatToConfigs.erase(it); > > + else > > + it++; > > + } > > + } > > + > > + ASSERT(sortedConfigs.size() == unsortedConfigs.size()); > > + > > + unsortedConfigs = sortedConfigs; > > +} > > I've added a blank line here. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > And pushed with the compilation fix and the blank line. Thanks for > bearing with us and the lengthy review. I'll do my best to review your > future patches faster. > > > +} /* namespace */ > > > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > int flags) > > @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > streamConfigs[index].streams.push_back({ jpegStream, type }); > > } > > > > + sortCamera3StreamConfigs(streamConfigs, jpegStream); > > for (const auto &streamConfig : streamConfigs) { > > config_->addConfiguration(streamConfig.config); > > > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Sat, Dec 12, 2020 at 09:42:28AM +0900, Hirokazu Honda wrote: > Hi Laurent, > > Thanks for reviewing. > I submitted new patches addressing your comments. Thank you, but I've already addressed the small issues and pushed the series. I apologize if that wasn't clear from my last e-mail. There's one change missing in the code I've pushed though, compared to your v8: diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 38689bdc40b1..0b68a92764ba 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -199,7 +198,7 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs, const auto nv12It = formatToConfigs.find(formats::NV12); if (nv12It != formatToConfigs.end()) { auto &nv12Configs = nv12It->second; - const auto &nv12Largest = nv12Configs.back(); + const Camera3StreamConfig *nv12Largest = nv12Configs.back(); /* * If JPEG will be created from NV12 and the size is larger than Would you like to send a patch for that, or would you like me to handle it ? > On Fri, Dec 11, 2020 at 10:12 PM Laurent Pinchart wrote: > > > > Hi Hiro, > > > > Thank you for the patch. > > > > On Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote: > > > This reorders Camera3Configs before executing > > > CameraConfiguration::validate() to make it easier for the Camera > > > to satisfy the Android framework request. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Reviewed-by: Umang Jain <email@uajain.com> > > > --- > > > src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++- > > > 1 file changed, 108 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 7e8b2818..0c58c1c5 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -27,6 +27,8 @@ > > > > > > using namespace libcamera; > > > > > > +LOG_DECLARE_CATEGORY(HAL) > > > + > > > namespace { > > > > > > /* > > > @@ -144,9 +146,112 @@ struct Camera3StreamConfig { > > > std::vector<Camera3Stream> streams; > > > StreamConfiguration config; > > > }; > > > -} /* namespace */ > > > > > > -LOG_DECLARE_CATEGORY(HAL) > > > +/* > > > + * Reorder the configurations so that libcamera::Camera can accept them as much > > > + * as possible. The sort rule is as follows. > > > + * 1.) The configuration for NV12 request whose resolution is the largest. > > > + * 2.) The configuration for JPEG request. > > > + * 3.) Others. Larger resolutions and different formats are put earlier. > > > + */ > > > +void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs, > > > + const camera3_stream_t *jpegStream) > > > +{ > > > + const Camera3StreamConfig *jpegConfig = nullptr; > > > + > > > + std::map<PixelFormat, std::vector<const Camera3StreamConfig *>> formatToConfigs; > > > + for (const auto &streamConfig : unsortedConfigs) { > > > + if (jpegStream && !jpegConfig) { > > > + const auto &streams = streamConfig.streams; > > > + if (std::find_if(streams.begin(), streams.end(), > > > + [jpegStream](const auto &stream) { > > > + return stream.stream == jpegStream; > > > + }) != streams.end()) { > > > + jpegConfig = &streamConfig; > > > + continue; > > > + } > > > + } > > > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig); > > > + } > > > + if (jpegStream && !jpegConfig) > > > + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for JPEG"; > > > + > > > + for (auto &[format, streamConfigs] : formatToConfigs) { > > > > This breaks compilation on gcc 7 :-( > > > > ../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’: > > ../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable] > > for (auto &[format, streamConfigs] : formatToConfigs) { > > > > gcc 7 doesn't seem to support [[maybe_unused]] for structured bindings, > > so the only option that seem to work is > > > > for (auto &fmt : formatToConfigs) { > > auto &streamConfigs = fmt.second; > > > > > + /* Sorted by resolution. Smaller is put first. */ > > > + std::sort(streamConfigs.begin(), streamConfigs.end(), > > > + [](const auto *streamConfigA, const auto *streamConfigB) { > > > + const Size &sizeA = streamConfigA->config.size; > > > + const Size &sizeB = streamConfigB->config.size; > > > + return sizeA < sizeB; > > > + }); > > > + } > > > + > > > + std::vector<Camera3StreamConfig> sortedConfigs; > > > + sortedConfigs.reserve(unsortedConfigs.size()); > > > + > > > + /* > > > + * NV12 is the most prioritized format. Put the configuration with NV12 > > > + * and the largest resolution first. > > > + */ > > > + const auto nv12It = formatToConfigs.find(formats::NV12); > > > + if (nv12It != formatToConfigs.end()) { > > > + auto &nv12Configs = nv12It->second; > > > + const auto &nv12Largest = nv12Configs.back(); > > > > The use of auto makes it more difficult to read the code, as one has to > > look up the variables types. auto has its uses, as some types are just > > too long to type out explicitly (like for nv12It for instance, which > > would be std::map<PixelFormat, std::vector<const Camera3StreamConfig *>>::iterator), > > but for nv12Largest typing out > > > > const Camera3StreamConfig *nv12Largest = nv12Configs.back(); > > > > isn't too bad, and improves readability I think. This is nothing that > > has to be addressed now, but could we keep this in mind for the future ? > > > > > + > > > + /* > > > + * If JPEG will be created from NV12 and the size is larger than > > > + * the largest NV12 configurations, then put the NV12 > > > + * configuration for JPEG first. > > > + */ > > > + if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) { > > > + const Size &nv12SizeForJpeg = jpegConfig->config.size; > > > + const Size &nv12LargestSize = nv12Largest->config.size; > > > + > > > + if (nv12LargestSize < nv12SizeForJpeg) { > > > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > > > + sortedConfigs.push_back(std::move(*jpegConfig)); > > > + jpegConfig = nullptr; > > > + } > > > + } > > > + > > > + LOG(HAL, Debug) << "Insert " << nv12Largest->config.toString(); > > > + sortedConfigs.push_back(*nv12Largest); > > > + nv12Configs.pop_back(); > > > + > > > + if (nv12Configs.empty()) > > > + formatToConfigs.erase(nv12It); > > > + } > > > + > > > + /* If the configuration for JPEG is there, then put it. */ > > > + if (jpegConfig) { > > > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > > > + sortedConfigs.push_back(std::move(*jpegConfig)); > > > + jpegConfig = nullptr; > > > + } > > > + > > > + /* > > > + * Put configurations with different formats and larger resolutions > > > + * earlier. > > > + */ > > > + while (!formatToConfigs.empty()) { > > > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > > > + auto &configs = it->second; > > > + LOG(HAL, Debug) << "Insert " << configs.back()->config.toString(); > > > + sortedConfigs.push_back(*configs.back()); > > > + configs.pop_back(); > > > + > > > + if (configs.empty()) > > > + it = formatToConfigs.erase(it); > > > + else > > > + it++; > > > + } > > > + } > > > + > > > + ASSERT(sortedConfigs.size() == unsortedConfigs.size()); > > > + > > > + unsortedConfigs = sortedConfigs; > > > +} > > > > I've added a blank line here. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > And pushed with the compilation fix and the blank line. Thanks for > > bearing with us and the lengthy review. I'll do my best to review your > > future patches faster. > > > > > +} /* namespace */ > > > > > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > > int flags) > > > @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > streamConfigs[index].streams.push_back({ jpegStream, type }); > > > } > > > > > > + sortCamera3StreamConfigs(streamConfigs, jpegStream); > > > for (const auto &streamConfig : streamConfigs) { > > > config_->addConfiguration(streamConfig.config); > > >
On Sat, Dec 12, 2020 at 9:46 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Sat, Dec 12, 2020 at 09:42:28AM +0900, Hirokazu Honda wrote: > > Hi Laurent, > > > > Thanks for reviewing. > > I submitted new patches addressing your comments. > > Thank you, but I've already addressed the small issues and pushed the > series. I apologize if that wasn't clear from my last e-mail. > I thought so but I didn't see the patches after fetching the master now. Looking https://git.linuxtv.org/libcamera.git/log/, you're correct. I think my local git setting was wrong. > There's one change missing in the code I've pushed though, compared to > your v8: > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 38689bdc40b1..0b68a92764ba 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -199,7 +198,7 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs, > const auto nv12It = formatToConfigs.find(formats::NV12); > if (nv12It != formatToConfigs.end()) { > auto &nv12Configs = nv12It->second; > - const auto &nv12Largest = nv12Configs.back(); > + const Camera3StreamConfig *nv12Largest = nv12Configs.back(); > > /* > * If JPEG will be created from NV12 and the size is larger than > > Would you like to send a patch for that, or would you like me to handle > it ? I don't mind it. I would not push it. Regards, -Hiro > > > On Fri, Dec 11, 2020 at 10:12 PM Laurent Pinchart wrote: > > > > > > Hi Hiro, > > > > > > Thank you for the patch. > > > > > > On Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote: > > > > This reorders Camera3Configs before executing > > > > CameraConfiguration::validate() to make it easier for the Camera > > > > to satisfy the Android framework request. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Reviewed-by: Umang Jain <email@uajain.com> > > > > --- > > > > src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++- > > > > 1 file changed, 108 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index 7e8b2818..0c58c1c5 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -27,6 +27,8 @@ > > > > > > > > using namespace libcamera; > > > > > > > > +LOG_DECLARE_CATEGORY(HAL) > > > > + > > > > namespace { > > > > > > > > /* > > > > @@ -144,9 +146,112 @@ struct Camera3StreamConfig { > > > > std::vector<Camera3Stream> streams; > > > > StreamConfiguration config; > > > > }; > > > > -} /* namespace */ > > > > > > > > -LOG_DECLARE_CATEGORY(HAL) > > > > +/* > > > > + * Reorder the configurations so that libcamera::Camera can accept them as much > > > > + * as possible. The sort rule is as follows. > > > > + * 1.) The configuration for NV12 request whose resolution is the largest. > > > > + * 2.) The configuration for JPEG request. > > > > + * 3.) Others. Larger resolutions and different formats are put earlier. > > > > + */ > > > > +void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs, > > > > + const camera3_stream_t *jpegStream) > > > > +{ > > > > + const Camera3StreamConfig *jpegConfig = nullptr; > > > > + > > > > + std::map<PixelFormat, std::vector<const Camera3StreamConfig *>> formatToConfigs; > > > > + for (const auto &streamConfig : unsortedConfigs) { > > > > + if (jpegStream && !jpegConfig) { > > > > + const auto &streams = streamConfig.streams; > > > > + if (std::find_if(streams.begin(), streams.end(), > > > > + [jpegStream](const auto &stream) { > > > > + return stream.stream == jpegStream; > > > > + }) != streams.end()) { > > > > + jpegConfig = &streamConfig; > > > > + continue; > > > > + } > > > > + } > > > > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig); > > > > + } > > > > + if (jpegStream && !jpegConfig) > > > > + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for JPEG"; > > > > + > > > > + for (auto &[format, streamConfigs] : formatToConfigs) { > > > > > > This breaks compilation on gcc 7 :-( > > > > > > ../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’: > > > ../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable] > > > for (auto &[format, streamConfigs] : formatToConfigs) { > > > > > > gcc 7 doesn't seem to support [[maybe_unused]] for structured bindings, > > > so the only option that seem to work is > > > > > > for (auto &fmt : formatToConfigs) { > > > auto &streamConfigs = fmt.second; > > > > > > > + /* Sorted by resolution. Smaller is put first. */ > > > > + std::sort(streamConfigs.begin(), streamConfigs.end(), > > > > + [](const auto *streamConfigA, const auto *streamConfigB) { > > > > + const Size &sizeA = streamConfigA->config.size; > > > > + const Size &sizeB = streamConfigB->config.size; > > > > + return sizeA < sizeB; > > > > + }); > > > > + } > > > > + > > > > + std::vector<Camera3StreamConfig> sortedConfigs; > > > > + sortedConfigs.reserve(unsortedConfigs.size()); > > > > + > > > > + /* > > > > + * NV12 is the most prioritized format. Put the configuration with NV12 > > > > + * and the largest resolution first. > > > > + */ > > > > + const auto nv12It = formatToConfigs.find(formats::NV12); > > > > + if (nv12It != formatToConfigs.end()) { > > > > + auto &nv12Configs = nv12It->second; > > > > + const auto &nv12Largest = nv12Configs.back(); > > > > > > The use of auto makes it more difficult to read the code, as one has to > > > look up the variables types. auto has its uses, as some types are just > > > too long to type out explicitly (like for nv12It for instance, which > > > would be std::map<PixelFormat, std::vector<const Camera3StreamConfig *>>::iterator), > > > but for nv12Largest typing out > > > > > > const Camera3StreamConfig *nv12Largest = nv12Configs.back(); > > > > > > isn't too bad, and improves readability I think. This is nothing that > > > has to be addressed now, but could we keep this in mind for the future ? > > > > > > > + > > > > + /* > > > > + * If JPEG will be created from NV12 and the size is larger than > > > > + * the largest NV12 configurations, then put the NV12 > > > > + * configuration for JPEG first. > > > > + */ > > > > + if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) { > > > > + const Size &nv12SizeForJpeg = jpegConfig->config.size; > > > > + const Size &nv12LargestSize = nv12Largest->config.size; > > > > + > > > > + if (nv12LargestSize < nv12SizeForJpeg) { > > > > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > > > > + sortedConfigs.push_back(std::move(*jpegConfig)); > > > > + jpegConfig = nullptr; > > > > + } > > > > + } > > > > + > > > > + LOG(HAL, Debug) << "Insert " << nv12Largest->config.toString(); > > > > + sortedConfigs.push_back(*nv12Largest); > > > > + nv12Configs.pop_back(); > > > > + > > > > + if (nv12Configs.empty()) > > > > + formatToConfigs.erase(nv12It); > > > > + } > > > > + > > > > + /* If the configuration for JPEG is there, then put it. */ > > > > + if (jpegConfig) { > > > > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > > > > + sortedConfigs.push_back(std::move(*jpegConfig)); > > > > + jpegConfig = nullptr; > > > > + } > > > > + > > > > + /* > > > > + * Put configurations with different formats and larger resolutions > > > > + * earlier. > > > > + */ > > > > + while (!formatToConfigs.empty()) { > > > > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > > > > + auto &configs = it->second; > > > > + LOG(HAL, Debug) << "Insert " << configs.back()->config.toString(); > > > > + sortedConfigs.push_back(*configs.back()); > > > > + configs.pop_back(); > > > > + > > > > + if (configs.empty()) > > > > + it = formatToConfigs.erase(it); > > > > + else > > > > + it++; > > > > + } > > > > + } > > > > + > > > > + ASSERT(sortedConfigs.size() == unsortedConfigs.size()); > > > > + > > > > + unsortedConfigs = sortedConfigs; > > > > +} > > > > > > I've added a blank line here. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > And pushed with the compilation fix and the blank line. Thanks for > > > bearing with us and the lengthy review. I'll do my best to review your > > > future patches faster. > > > > > > > +} /* namespace */ > > > > > > > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > > > int flags) > > > > @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > streamConfigs[index].streams.push_back({ jpegStream, type }); > > > > } > > > > > > > > + sortCamera3StreamConfigs(streamConfigs, jpegStream); > > > > for (const auto &streamConfig : streamConfigs) { > > > > config_->addConfiguration(streamConfig.config); > > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 7e8b2818..0c58c1c5 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -27,6 +27,8 @@ using namespace libcamera; +LOG_DECLARE_CATEGORY(HAL) + namespace { /* @@ -144,9 +146,112 @@ struct Camera3StreamConfig { std::vector<Camera3Stream> streams; StreamConfiguration config; }; -} /* namespace */ -LOG_DECLARE_CATEGORY(HAL) +/* + * Reorder the configurations so that libcamera::Camera can accept them as much + * as possible. The sort rule is as follows. + * 1.) The configuration for NV12 request whose resolution is the largest. + * 2.) The configuration for JPEG request. + * 3.) Others. Larger resolutions and different formats are put earlier. + */ +void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs, + const camera3_stream_t *jpegStream) +{ + const Camera3StreamConfig *jpegConfig = nullptr; + + std::map<PixelFormat, std::vector<const Camera3StreamConfig *>> formatToConfigs; + for (const auto &streamConfig : unsortedConfigs) { + if (jpegStream && !jpegConfig) { + const auto &streams = streamConfig.streams; + if (std::find_if(streams.begin(), streams.end(), + [jpegStream](const auto &stream) { + return stream.stream == jpegStream; + }) != streams.end()) { + jpegConfig = &streamConfig; + continue; + } + } + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig); + } + if (jpegStream && !jpegConfig) + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for JPEG"; + + for (auto &[format, streamConfigs] : formatToConfigs) { + /* Sorted by resolution. Smaller is put first. */ + std::sort(streamConfigs.begin(), streamConfigs.end(), + [](const auto *streamConfigA, const auto *streamConfigB) { + const Size &sizeA = streamConfigA->config.size; + const Size &sizeB = streamConfigB->config.size; + return sizeA < sizeB; + }); + } + + std::vector<Camera3StreamConfig> sortedConfigs; + sortedConfigs.reserve(unsortedConfigs.size()); + + /* + * NV12 is the most prioritized format. Put the configuration with NV12 + * and the largest resolution first. + */ + const auto nv12It = formatToConfigs.find(formats::NV12); + if (nv12It != formatToConfigs.end()) { + auto &nv12Configs = nv12It->second; + const auto &nv12Largest = nv12Configs.back(); + + /* + * If JPEG will be created from NV12 and the size is larger than + * the largest NV12 configurations, then put the NV12 + * configuration for JPEG first. + */ + if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) { + const Size &nv12SizeForJpeg = jpegConfig->config.size; + const Size &nv12LargestSize = nv12Largest->config.size; + + if (nv12LargestSize < nv12SizeForJpeg) { + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); + sortedConfigs.push_back(std::move(*jpegConfig)); + jpegConfig = nullptr; + } + } + + LOG(HAL, Debug) << "Insert " << nv12Largest->config.toString(); + sortedConfigs.push_back(*nv12Largest); + nv12Configs.pop_back(); + + if (nv12Configs.empty()) + formatToConfigs.erase(nv12It); + } + + /* If the configuration for JPEG is there, then put it. */ + if (jpegConfig) { + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); + sortedConfigs.push_back(std::move(*jpegConfig)); + jpegConfig = nullptr; + } + + /* + * Put configurations with different formats and larger resolutions + * earlier. + */ + while (!formatToConfigs.empty()) { + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { + auto &configs = it->second; + LOG(HAL, Debug) << "Insert " << configs.back()->config.toString(); + sortedConfigs.push_back(*configs.back()); + configs.pop_back(); + + if (configs.empty()) + it = formatToConfigs.erase(it); + else + it++; + } + } + + ASSERT(sortedConfigs.size() == unsortedConfigs.size()); + + unsortedConfigs = sortedConfigs; +} +} /* namespace */ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags) @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) streamConfigs[index].streams.push_back({ jpegStream, type }); } + sortCamera3StreamConfigs(streamConfigs, jpegStream); for (const auto &streamConfig : streamConfigs) { config_->addConfiguration(streamConfig.config);