[libcamera-devel] [PATCH v7 3/3] android: camera_device: Reorder configurations before requesting
Hirokazu Honda
hiroh at chromium.org
Sat Dec 12 01:50:36 CET 2020
On Sat, Dec 12, 2020 at 9:46 AM Laurent Pinchart
<laurent.pinchart at 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 at chromium.org>
> > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > Reviewed-by: Umang Jain <email at 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 at 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
More information about the libcamera-devel
mailing list