| Message ID | 20201209055410.3232987-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On 09/12/2020 05:54, 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> > --- > src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++- > 1 file changed, 107 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b7bf3d88..c9e0ec98 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -9,6 +9,7 @@ > #include "camera_ops.h" > #include "post_processor.h" > > +#include <optional> > #include <sys/mman.h> > #include <tuple> > #include <vector> > @@ -27,6 +28,8 @@ > > using namespace libcamera; > > +LOG_DECLARE_CATEGORY(HAL) > + > namespace { > > /* > @@ -140,9 +143,109 @@ struct Camera3StreamConfig { > std::vector<CameraStream::Type> types; > 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. > + */ > +std::vector<Camera3StreamConfig> sortCamera3StreamConfigs( > + std::vector<Camera3StreamConfig> unsortedConfigs, > + const camera3_stream_t *jpegStream) > +{ > + const size_t unsortedSize = unsortedConfigs.size(); > + std::optional<Camera3StreamConfig> jpegConfig = std::nullopt; > + > + if (jpegStream) { > + for (size_t i = 0; i < unsortedSize; ++i) { > + const auto &streams = unsortedConfigs[i].streams; > + if (std::find(streams.begin(), streams.end(), > + jpegStream) != streams.end()) { > + jpegConfig = std::move(unsortedConfigs[i]); > + unsortedConfigs.erase(unsortedConfigs.begin() + i); > + break; > + } > + } > + if (!jpegConfig) > + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for Jpeg"; > + } > + > + std::map<uint32_t, std::vector<Camera3StreamConfig>> formatToConfigs; > + for (const auto &streamConfig : unsortedConfigs) { > + const StreamConfiguration &config = streamConfig.config; > + formatToConfigs[config.pixelFormat].push_back(streamConfig); > + > + } > + 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(unsortedSize); > + /* > + * 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 Size &nv12LargestSize = nv12Configs.back().config.size; > + /* > + * 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; > + > + if (nv12LargestSize < nv12SizeForJpeg) { > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > + sortedConfigs.push_back(std::move(*jpegConfig)); > + jpegConfig = std::nullopt; > + } > + } > + LOG(HAL, Debug) << "Insert " << nv12Configs.back().config.toString(); > + sortedConfigs.push_back(std::move(nv12Configs.back())); > + nv12Configs.pop_back(); > + } > + > + /* 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 = std::nullopt; > + } > + > + /* > + * Put configurations with different formats and larger resolutions > + * earlier. > + */ > + while (!formatToConfigs.empty()) { > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > + auto& configs = it->second; > + if (configs.empty()) { > + it = formatToConfigs.erase(it); > + continue; > + } > + LOG(HAL, Debug) << "Insert " << configs.back().config.toString(); > + sortedConfigs.push_back(std::move(configs.back())); > + configs.pop_back(); > + it++; > + } > + } > + assert(sortedConfigs.size() == unsortedSize); Should we assert(unsortedConfigs == 0) too? But I don't think it's a blocker or required. The sorting seems like a good idea otherwise. > + > + return sortedConfigs; > +} > +} /* namespace */ > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > int flags) > @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > streamConfigs[index].types.push_back(type); > } > > + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs), > + jpegStream); It's hard to see here, but I assume we now sort before we infer any indexes, so the index remains valid. As long as that statement is true, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > for (const auto &streamConfig : streamConfigs) { > config_->addConfiguration(streamConfig.config); > for (size_t i = 0; i < streamConfig.streams.size(); ++i) { > -- > 2.29.2.576.ga3fc446d84-goog > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
One more thought, On 09/12/2020 11:10, Kieran Bingham wrote: > Hi Hiro, > > On 09/12/2020 05:54, 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> >> --- >> src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++- >> 1 file changed, 107 insertions(+), 2 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index b7bf3d88..c9e0ec98 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -9,6 +9,7 @@ >> #include "camera_ops.h" >> #include "post_processor.h" >> >> +#include <optional> >> #include <sys/mman.h> >> #include <tuple> >> #include <vector> >> @@ -27,6 +28,8 @@ >> >> using namespace libcamera; >> >> +LOG_DECLARE_CATEGORY(HAL) >> + >> namespace { >> >> /* >> @@ -140,9 +143,109 @@ struct Camera3StreamConfig { >> std::vector<CameraStream::Type> types; >> 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. >> + */ >> +std::vector<Camera3StreamConfig> sortCamera3StreamConfigs( >> + std::vector<Camera3StreamConfig> unsortedConfigs, >> + const camera3_stream_t *jpegStream) >> +{ >> + const size_t unsortedSize = unsortedConfigs.size(); >> + std::optional<Camera3StreamConfig> jpegConfig = std::nullopt; >> + >> + if (jpegStream) { >> + for (size_t i = 0; i < unsortedSize; ++i) { >> + const auto &streams = unsortedConfigs[i].streams; >> + if (std::find(streams.begin(), streams.end(), >> + jpegStream) != streams.end()) { >> + jpegConfig = std::move(unsortedConfigs[i]); >> + unsortedConfigs.erase(unsortedConfigs.begin() + i); >> + break; >> + } >> + } >> + if (!jpegConfig) >> + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for Jpeg"; >> + } >> + >> + std::map<uint32_t, std::vector<Camera3StreamConfig>> formatToConfigs; >> + for (const auto &streamConfig : unsortedConfigs) { >> + const StreamConfiguration &config = streamConfig.config; >> + formatToConfigs[config.pixelFormat].push_back(streamConfig); >> + >> + } >> + 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(unsortedSize); >> + /* >> + * 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 Size &nv12LargestSize = nv12Configs.back().config.size; >> + /* >> + * 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; >> + >> + if (nv12LargestSize < nv12SizeForJpeg) { >> + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); >> + sortedConfigs.push_back(std::move(*jpegConfig)); >> + jpegConfig = std::nullopt; >> + } >> + } >> + LOG(HAL, Debug) << "Insert " << nv12Configs.back().config.toString(); >> + sortedConfigs.push_back(std::move(nv12Configs.back())); >> + nv12Configs.pop_back(); >> + } >> + >> + /* 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 = std::nullopt; >> + } >> + >> + /* >> + * Put configurations with different formats and larger resolutions >> + * earlier. >> + */ >> + while (!formatToConfigs.empty()) { >> + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { >> + auto& configs = it->second; >> + if (configs.empty()) { >> + it = formatToConfigs.erase(it); >> + continue; >> + } >> + LOG(HAL, Debug) << "Insert " << configs.back().config.toString(); >> + sortedConfigs.push_back(std::move(configs.back())); >> + configs.pop_back(); >> + it++; >> + } >> + } >> + assert(sortedConfigs.size() == unsortedSize); > > Should we assert(unsortedConfigs == 0) too? We use a LOG(Fatal) above, which is an assert with a print... We could either use that, or we have our own ASSERT macro defined, which we use throughout libcamera. But that's an easy change to capitalise to the macro instead, and could perhaps be done when applying. -- Kieran > But I don't think it's a blocker or required. > > The sorting seems like a good idea otherwise. > > >> + >> + return sortedConfigs; >> +} >> +} /* namespace */ >> >> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, >> int flags) >> @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) >> streamConfigs[index].types.push_back(type); >> } >> >> + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs), >> + jpegStream); > > It's hard to see here, but I assume we now sort before we infer any > indexes, so the index remains valid. > > As long as that statement is true, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> for (const auto &streamConfig : streamConfigs) { >> config_->addConfiguration(streamConfig.config); >> for (size_t i = 0; i < streamConfig.streams.size(); ++i) { >> -- >> 2.29.2.576.ga3fc446d84-goog >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >> >
Hi Kieran, On Wed, Dec 9, 2020 at 8:15 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > One more thought, > > On 09/12/2020 11:10, Kieran Bingham wrote: > > Hi Hiro, > > > > On 09/12/2020 05:54, 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> > >> --- > >> src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++- > >> 1 file changed, 107 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index b7bf3d88..c9e0ec98 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -9,6 +9,7 @@ > >> #include "camera_ops.h" > >> #include "post_processor.h" > >> > >> +#include <optional> > >> #include <sys/mman.h> > >> #include <tuple> > >> #include <vector> > >> @@ -27,6 +28,8 @@ > >> > >> using namespace libcamera; > >> > >> +LOG_DECLARE_CATEGORY(HAL) > >> + > >> namespace { > >> > >> /* > >> @@ -140,9 +143,109 @@ struct Camera3StreamConfig { > >> std::vector<CameraStream::Type> types; > >> 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. > >> + */ > >> +std::vector<Camera3StreamConfig> sortCamera3StreamConfigs( > >> + std::vector<Camera3StreamConfig> unsortedConfigs, > >> + const camera3_stream_t *jpegStream) > >> +{ > >> + const size_t unsortedSize = unsortedConfigs.size(); > >> + std::optional<Camera3StreamConfig> jpegConfig = std::nullopt; > >> + > >> + if (jpegStream) { > >> + for (size_t i = 0; i < unsortedSize; ++i) { > >> + const auto &streams = unsortedConfigs[i].streams; > >> + if (std::find(streams.begin(), streams.end(), > >> + jpegStream) != streams.end()) { > >> + jpegConfig = std::move(unsortedConfigs[i]); > >> + unsortedConfigs.erase(unsortedConfigs.begin() + i); > >> + break; > >> + } > >> + } > >> + if (!jpegConfig) > >> + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for Jpeg"; > >> + } > >> + > >> + std::map<uint32_t, std::vector<Camera3StreamConfig>> formatToConfigs; > >> + for (const auto &streamConfig : unsortedConfigs) { > >> + const StreamConfiguration &config = streamConfig.config; > >> + formatToConfigs[config.pixelFormat].push_back(streamConfig); > >> + > >> + } > >> + 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(unsortedSize); > >> + /* > >> + * 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 Size &nv12LargestSize = nv12Configs.back().config.size; > >> + /* > >> + * 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; > >> + > >> + if (nv12LargestSize < nv12SizeForJpeg) { > >> + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > >> + sortedConfigs.push_back(std::move(*jpegConfig)); > >> + jpegConfig = std::nullopt; > >> + } > >> + } > >> + LOG(HAL, Debug) << "Insert " << nv12Configs.back().config.toString(); > >> + sortedConfigs.push_back(std::move(nv12Configs.back())); > >> + nv12Configs.pop_back(); > >> + } > >> + > >> + /* 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 = std::nullopt; > >> + } > >> + > >> + /* > >> + * Put configurations with different formats and larger resolutions > >> + * earlier. > >> + */ > >> + while (!formatToConfigs.empty()) { > >> + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > >> + auto& configs = it->second; > >> + if (configs.empty()) { > >> + it = formatToConfigs.erase(it); > >> + continue; > >> + } > >> + LOG(HAL, Debug) << "Insert " << configs.back().config.toString(); > >> + sortedConfigs.push_back(std::move(configs.back())); > >> + configs.pop_back(); > >> + it++; > >> + } > >> + } > >> + assert(sortedConfigs.size() == unsortedSize); > > > > Should we assert(unsortedConfigs == 0) too? > > We use a LOG(Fatal) above, which is an assert with a print... > > We could either use that, or we have our own ASSERT macro defined, which > we use throughout libcamera. > > But that's an easy change to capitalise to the macro instead, and could > perhaps be done when applying. > Sure thing. Please feel free to modify the parts in applying. Regards, -Hiro > -- > Kieran > > > > > But I don't think it's a blocker or required. > > > > The sorting seems like a good idea otherwise. > > > > > >> + > >> + return sortedConfigs; > >> +} > >> +} /* namespace */ > >> > >> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > >> int flags) > >> @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > >> streamConfigs[index].types.push_back(type); > >> } > >> > >> + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs), > >> + jpegStream); > > > > It's hard to see here, but I assume we now sort before we infer any > > indexes, so the index remains valid. > > > > As long as that statement is true, > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > >> for (const auto &streamConfig : streamConfigs) { > >> config_->addConfiguration(streamConfig.config); > >> for (size_t i = 0; i < streamConfig.streams.size(); ++i) { > >> -- > >> 2.29.2.576.ga3fc446d84-goog > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > >> > > > > -- > Regards > -- > Kieran
HI Hiro, After much careful reading of the previous iterations, On 12/9/20 11:24 AM, 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: Umang Jain <email@uajain.com> > --- > src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++- > 1 file changed, 107 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b7bf3d88..c9e0ec98 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -9,6 +9,7 @@ > #include "camera_ops.h" > #include "post_processor.h" > > +#include <optional> > #include <sys/mman.h> > #include <tuple> > #include <vector> > @@ -27,6 +28,8 @@ > > using namespace libcamera; > > +LOG_DECLARE_CATEGORY(HAL) > + > namespace { > > /* > @@ -140,9 +143,109 @@ struct Camera3StreamConfig { > std::vector<CameraStream::Type> types; > 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. > + */ > +std::vector<Camera3StreamConfig> sortCamera3StreamConfigs( > + std::vector<Camera3StreamConfig> unsortedConfigs, > + const camera3_stream_t *jpegStream) > +{ > + const size_t unsortedSize = unsortedConfigs.size(); > + std::optional<Camera3StreamConfig> jpegConfig = std::nullopt; > + > + if (jpegStream) { > + for (size_t i = 0; i < unsortedSize; ++i) { > + const auto &streams = unsortedConfigs[i].streams; > + if (std::find(streams.begin(), streams.end(), > + jpegStream) != streams.end()) { > + jpegConfig = std::move(unsortedConfigs[i]); > + unsortedConfigs.erase(unsortedConfigs.begin() + i); > + break; > + } > + } > + if (!jpegConfig) > + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for Jpeg"; > + } > + > + std::map<uint32_t, std::vector<Camera3StreamConfig>> formatToConfigs; > + for (const auto &streamConfig : unsortedConfigs) { > + const StreamConfiguration &config = streamConfig.config; > + formatToConfigs[config.pixelFormat].push_back(streamConfig); > + > + } > + 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(unsortedSize); > + /* > + * 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 Size &nv12LargestSize = nv12Configs.back().config.size; > + /* > + * 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; > + > + if (nv12LargestSize < nv12SizeForJpeg) { > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > + sortedConfigs.push_back(std::move(*jpegConfig)); > + jpegConfig = std::nullopt; > + } > + } > + LOG(HAL, Debug) << "Insert " << nv12Configs.back().config.toString(); > + sortedConfigs.push_back(std::move(nv12Configs.back())); > + nv12Configs.pop_back(); > + } > + > + /* 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 = std::nullopt; > + } > + > + /* > + * Put configurations with different formats and larger resolutions > + * earlier. > + */ > + while (!formatToConfigs.empty()) { > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > + auto& configs = it->second; > + if (configs.empty()) { > + it = formatToConfigs.erase(it); > + continue; > + } > + LOG(HAL, Debug) << "Insert " << configs.back().config.toString(); > + sortedConfigs.push_back(std::move(configs.back())); > + configs.pop_back(); > + it++; > + } > + } > + assert(sortedConfigs.size() == unsortedSize); > + > + return sortedConfigs; > +} > +} /* namespace */ > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > int flags) > @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > streamConfigs[index].types.push_back(type); > } > > + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs), > + jpegStream); > for (const auto &streamConfig : streamConfigs) { > config_->addConfiguration(streamConfig.config); > for (size_t i = 0; i < streamConfig.streams.size(); ++i) { > -- > 2.29.2.576.ga3fc446d84-goog > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, Thank you for the patch. On Wed, Dec 09, 2020 at 05:54:10AM +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> > --- > src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++- > 1 file changed, 107 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b7bf3d88..c9e0ec98 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -9,6 +9,7 @@ > #include "camera_ops.h" > #include "post_processor.h" > > +#include <optional> > #include <sys/mman.h> > #include <tuple> > #include <vector> > @@ -27,6 +28,8 @@ > > using namespace libcamera; > > +LOG_DECLARE_CATEGORY(HAL) > + > namespace { > > /* > @@ -140,9 +143,109 @@ struct Camera3StreamConfig { > std::vector<CameraStream::Type> types; > 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. > + */ > +std::vector<Camera3StreamConfig> sortCamera3StreamConfigs( > + std::vector<Camera3StreamConfig> unsortedConfigs, > + const camera3_stream_t *jpegStream) How about making this a private member function ? > +{ > + const size_t unsortedSize = unsortedConfigs.size(); > + std::optional<Camera3StreamConfig> jpegConfig = std::nullopt; std::nullopt is the default value, so you can write std::optional<Camera3StreamConfig> jpegConfig; > + > + if (jpegStream) { > + for (size_t i = 0; i < unsortedSize; ++i) { > + const auto &streams = unsortedConfigs[i].streams; > + if (std::find(streams.begin(), streams.end(), > + jpegStream) != streams.end()) { > + jpegConfig = std::move(unsortedConfigs[i]); > + unsortedConfigs.erase(unsortedConfigs.begin() + i); > + break; > + } > + } > + if (!jpegConfig) > + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for Jpeg"; > + } > + > + std::map<uint32_t, std::vector<Camera3StreamConfig>> formatToConfigs; Should the key be a PixelFormat instead of a uint32_t ? > + for (const auto &streamConfig : unsortedConfigs) { > + const StreamConfiguration &config = streamConfig.config; > + formatToConfigs[config.pixelFormat].push_back(streamConfig); > + > + } I wonder if this could be simplified to const Camera3StreamConfig *jpegConfig = nullptr; std::map<PixelFormat, std::vector<Camera3StreamConfig>> formatToConfigs; for (const auto &streamConfig : unsortedConfigs) { if (jpegStream && !jpegConfig) { const auto &streams = streamConfig.streams; if (std::find(streams.begin(), streams.end(), jpegStream) != streams.end()) { jpegConfig = &streamConfig; continue; } } const StreamConfiguration &config = streamConfig.config; formatToConfigs[config.pixelFormat].push_back(streamConfig); } if (jpegStream && !jpegConfig) LOG(HAL, Fatal) << "No Camera3StreamConfig is found for JPEG"; The advantage is that unsortedStream would now be const. formatToConfigs could then be a map of PixelFormat to vector of pointers to Camera3StreamConfig, avoiding a copy. The only copy would be done when creating the entries in sortedConfigs. > + 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(unsortedSize); A blank line would be nice here. > + /* > + * 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 Size &nv12LargestSize = nv12Configs.back().config.size; Here too. > + /* > + * 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; > + > + if (nv12LargestSize < nv12SizeForJpeg) { > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > + sortedConfigs.push_back(std::move(*jpegConfig)); > + jpegConfig = std::nullopt; > + } > + } And here too. > + LOG(HAL, Debug) << "Insert " << nv12Configs.back().config.toString(); > + sortedConfigs.push_back(std::move(nv12Configs.back())); > + nv12Configs.pop_back(); > + } > + > + /* 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 = std::nullopt; > + } > + > + /* > + * Put configurations with different formats and larger resolutions > + * earlier. > + */ > + while (!formatToConfigs.empty()) { > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > + auto& configs = it->second; auto &configs = it->second; > + if (configs.empty()) { > + it = formatToConfigs.erase(it); > + continue; > + } This could be moved to the end, ... > + LOG(HAL, Debug) << "Insert " << configs.back().config.toString(); > + sortedConfigs.push_back(std::move(configs.back())); > + configs.pop_back(); > + it++; ... it would become configs.pop_back(); if (configs.empty()) it = formatToConfigs.erase(it); else it++; > + } > + } > + assert(sortedConfigs.size() == unsortedSize); > + > + return sortedConfigs; > +} > +} /* namespace */ > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > int flags) > @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > streamConfigs[index].types.push_back(type); > } > > + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs), > + jpegStream); This is a bit of a weird function signature. Could we sort the vector in-place ? sortCamera3StreamConfigs(streamConfigs, jpegStream); It doesn't mean that the implementation must be in-place, sortCamera3StreamConfigs() can make an internal copy. As I wanted to test my suggestions, I've ended up reworking the code to make sure it would compile. I've pushed the result to https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=android/hiro in case it could be useful. > for (const auto &streamConfig : streamConfigs) { > config_->addConfiguration(streamConfig.config); > for (size_t i = 0; i < streamConfig.streams.size(); ++i) {
Hi, Kieran, Laurent and Umang Thanks for reviewing! On Fri, Dec 11, 2020 at 7:25 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Wed, Dec 09, 2020 at 05:54:10AM +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> > > --- > > src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++- > > 1 file changed, 107 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index b7bf3d88..c9e0ec98 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -9,6 +9,7 @@ > > #include "camera_ops.h" > > #include "post_processor.h" > > > > +#include <optional> > > #include <sys/mman.h> > > #include <tuple> > > #include <vector> > > @@ -27,6 +28,8 @@ > > > > using namespace libcamera; > > > > +LOG_DECLARE_CATEGORY(HAL) > > + > > namespace { > > > > /* > > @@ -140,9 +143,109 @@ struct Camera3StreamConfig { > > std::vector<CameraStream::Type> types; > > 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. > > + */ > > +std::vector<Camera3StreamConfig> sortCamera3StreamConfigs( > > + std::vector<Camera3StreamConfig> unsortedConfigs, > > + const camera3_stream_t *jpegStream) > > How about making this a private member function ? > I would keep here so that we don't expose Camera3StreamConfig in camera_device.h. Regards, -Hiro > > +{ > > + const size_t unsortedSize = unsortedConfigs.size(); > > + std::optional<Camera3StreamConfig> jpegConfig = std::nullopt; > > std::nullopt is the default value, so you can write > > std::optional<Camera3StreamConfig> jpegConfig; > > > + > > + if (jpegStream) { > > + for (size_t i = 0; i < unsortedSize; ++i) { > > + const auto &streams = unsortedConfigs[i].streams; > > + if (std::find(streams.begin(), streams.end(), > > + jpegStream) != streams.end()) { > > + jpegConfig = std::move(unsortedConfigs[i]); > > + unsortedConfigs.erase(unsortedConfigs.begin() + i); > > + break; > > + } > > + } > > + if (!jpegConfig) > > + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for Jpeg"; > > + } > > + > > + std::map<uint32_t, std::vector<Camera3StreamConfig>> formatToConfigs; > > Should the key be a PixelFormat instead of a uint32_t ? > > > + for (const auto &streamConfig : unsortedConfigs) { > > + const StreamConfiguration &config = streamConfig.config; > > + formatToConfigs[config.pixelFormat].push_back(streamConfig); > > + > > + } > > I wonder if this could be simplified to > > const Camera3StreamConfig *jpegConfig = nullptr; > > std::map<PixelFormat, std::vector<Camera3StreamConfig>> formatToConfigs; > for (const auto &streamConfig : unsortedConfigs) { > if (jpegStream && !jpegConfig) { > const auto &streams = streamConfig.streams; > if (std::find(streams.begin(), streams.end(), > jpegStream) != streams.end()) { > jpegConfig = &streamConfig; > continue; > } > } > > const StreamConfiguration &config = streamConfig.config; > formatToConfigs[config.pixelFormat].push_back(streamConfig); > } > > if (jpegStream && !jpegConfig) > LOG(HAL, Fatal) << "No Camera3StreamConfig is found for JPEG"; > > The advantage is that unsortedStream would now be const. formatToConfigs > could then be a map of PixelFormat to vector of pointers to > Camera3StreamConfig, avoiding a copy. The only copy would be done when > creating the entries in sortedConfigs. > > > + 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(unsortedSize); > > A blank line would be nice here. > > > + /* > > + * 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 Size &nv12LargestSize = nv12Configs.back().config.size; > > Here too. > > > + /* > > + * 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; > > + > > + if (nv12LargestSize < nv12SizeForJpeg) { > > + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); > > + sortedConfigs.push_back(std::move(*jpegConfig)); > > + jpegConfig = std::nullopt; > > + } > > + } > > And here too. > > > + LOG(HAL, Debug) << "Insert " << nv12Configs.back().config.toString(); > > + sortedConfigs.push_back(std::move(nv12Configs.back())); > > + nv12Configs.pop_back(); > > + } > > + > > + /* 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 = std::nullopt; > > + } > > + > > + /* > > + * Put configurations with different formats and larger resolutions > > + * earlier. > > + */ > > + while (!formatToConfigs.empty()) { > > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { > > + auto& configs = it->second; > > auto &configs = it->second; > > > + if (configs.empty()) { > > + it = formatToConfigs.erase(it); > > + continue; > > + } > > This could be moved to the end, ... > > > + LOG(HAL, Debug) << "Insert " << configs.back().config.toString(); > > + sortedConfigs.push_back(std::move(configs.back())); > > + configs.pop_back(); > > + it++; > > ... it would become > > configs.pop_back(); > if (configs.empty()) > it = formatToConfigs.erase(it); > else > it++; > > > + } > > + } > > + assert(sortedConfigs.size() == unsortedSize); > > + > > + return sortedConfigs; > > +} > > +} /* namespace */ > > > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > int flags) > > @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > streamConfigs[index].types.push_back(type); > > } > > > > + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs), > > + jpegStream); > > This is a bit of a weird function signature. Could we sort the vector > in-place ? > > sortCamera3StreamConfigs(streamConfigs, jpegStream); > > It doesn't mean that the implementation must be in-place, > sortCamera3StreamConfigs() can make an internal copy. > > As I wanted to test my suggestions, I've ended up reworking the code to > make sure it would compile. I've pushed the result to > https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=android/hiro > in case it could be useful. > > > for (const auto &streamConfig : streamConfigs) { > > config_->addConfiguration(streamConfig.config); > > for (size_t i = 0; i < streamConfig.streams.size(); ++i) { > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index b7bf3d88..c9e0ec98 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -9,6 +9,7 @@ #include "camera_ops.h" #include "post_processor.h" +#include <optional> #include <sys/mman.h> #include <tuple> #include <vector> @@ -27,6 +28,8 @@ using namespace libcamera; +LOG_DECLARE_CATEGORY(HAL) + namespace { /* @@ -140,9 +143,109 @@ struct Camera3StreamConfig { std::vector<CameraStream::Type> types; 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. + */ +std::vector<Camera3StreamConfig> sortCamera3StreamConfigs( + std::vector<Camera3StreamConfig> unsortedConfigs, + const camera3_stream_t *jpegStream) +{ + const size_t unsortedSize = unsortedConfigs.size(); + std::optional<Camera3StreamConfig> jpegConfig = std::nullopt; + + if (jpegStream) { + for (size_t i = 0; i < unsortedSize; ++i) { + const auto &streams = unsortedConfigs[i].streams; + if (std::find(streams.begin(), streams.end(), + jpegStream) != streams.end()) { + jpegConfig = std::move(unsortedConfigs[i]); + unsortedConfigs.erase(unsortedConfigs.begin() + i); + break; + } + } + if (!jpegConfig) + LOG(HAL, Fatal) << "No Camera3StreamConfig is found for Jpeg"; + } + + std::map<uint32_t, std::vector<Camera3StreamConfig>> formatToConfigs; + for (const auto &streamConfig : unsortedConfigs) { + const StreamConfiguration &config = streamConfig.config; + formatToConfigs[config.pixelFormat].push_back(streamConfig); + + } + 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(unsortedSize); + /* + * 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 Size &nv12LargestSize = nv12Configs.back().config.size; + /* + * 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; + + if (nv12LargestSize < nv12SizeForJpeg) { + LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString(); + sortedConfigs.push_back(std::move(*jpegConfig)); + jpegConfig = std::nullopt; + } + } + LOG(HAL, Debug) << "Insert " << nv12Configs.back().config.toString(); + sortedConfigs.push_back(std::move(nv12Configs.back())); + nv12Configs.pop_back(); + } + + /* 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 = std::nullopt; + } + + /* + * Put configurations with different formats and larger resolutions + * earlier. + */ + while (!formatToConfigs.empty()) { + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) { + auto& configs = it->second; + if (configs.empty()) { + it = formatToConfigs.erase(it); + continue; + } + LOG(HAL, Debug) << "Insert " << configs.back().config.toString(); + sortedConfigs.push_back(std::move(configs.back())); + configs.pop_back(); + it++; + } + } + assert(sortedConfigs.size() == unsortedSize); + + return sortedConfigs; +} +} /* namespace */ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags) @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) streamConfigs[index].types.push_back(type); } + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs), + jpegStream); for (const auto &streamConfig : streamConfigs) { config_->addConfiguration(streamConfig.config); for (size_t i = 0; i < streamConfig.streams.size(); ++i) {