[PATCH v13 6/7] libcamera: virtual: Read config and register cameras based on the config

[Cheng-Hao Yang]

Hi Barnabás,

On Fri, Sep 27, 2024 at 4:25 AM Barnabás Pőcze <pobrn at protonmail.com> wrote:
>
> Hi
>
>
> 2024. szeptember 26., csütörtök 18:29 keltezéssel, Harvey Yang <chenghaoyang at chromium.org> írta:
>
> > This patch introduces the configuration file for Virtual Pipeline
> > Handler. The config file is written in yaml, and the format is
> > documented in `README.md`.
> >
> > The config file will define the camera with IDs, supported formats and
> > image sources, etc. In the default config file, only Test Patterns are
> > used. Developers can use real images loading if desired.
> >
> > Signed-off-by: Konami Shu <konamiz at google.com>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > Co-developed-by: Yunke Cao <yunkec at chromium.org>
> > Co-developed-by: Tomasz Figa <tfiga at chromium.org>
> > ---
> > [...]
> > +int Parser::parseFrameGenerator(const YamlObject &cameraConfigData, VirtualCameraData *data)
> > +{
> > +     const std::string testPatternKey = "test_pattern";
> > +     const std::string framesKey = "frames";
> > +     if (cameraConfigData.contains(testPatternKey)) {
> > +             if (cameraConfigData.contains(framesKey)) {
> > +                     LOG(Virtual, Error) << "A camera should use either "
> > +                                         << testPatternKey << " or " << framesKey;
> > +                     return -EINVAL;
> > +             }
> > +
> > +             auto testPattern = cameraConfigData[testPatternKey].get<std::string>("");
> > +
> > +             if (testPattern == "bars") {
> > +                     data->config_.frame = TestPattern::ColorBars;
> > +             } else if (testPattern == "lines") {
> > +                     data->config_.frame = TestPattern::DiagonalLines;
> > +             } else {
> > +                     LOG(Virtual, Debug) << "Test pattern: " << testPattern
> > +                                         << " is not supported";
> > +                     return -EINVAL;
> > +             }
> > +
> > +             return 0;
> > +     }
> > +
> > +     const YamlObject &frames = cameraConfigData[framesKey];
> > +
> > +     /* When there is no frames provided in the config file, use color bar test pattern */
> > +     if (!frames) {
> > +             data->config_.frame = TestPattern::ColorBars;
> > +             return 0;
> > +     }
> > +
> > +     if (!frames.isDictionary()) {
> > +             LOG(Virtual, Error) << "'frames' is not a dictionary.";
> > +             return -EINVAL;
> > +     }
> > +
> > +     auto path = frames["path"].get<std::string>();
> > +
> > +     if (!path) {
> > +             LOG(Virtual, Error) << "Test pattern or path should be specified.";
> > +             return -EINVAL;
> > +     } else if (auto ext = std::filesystem::path(*path).extension();
> > +                ext == ".jpg" || ext == ".jpeg") {
> > +             data->config_.frame = ImageFrames{ *path, std::nullopt };
> > +     } else if (std::filesystem::is_directory(std::filesystem::symlink_status(*path))) {
> > +             using std::filesystem::directory_iterator;
> > +             unsigned int numOfFiles = std::distance(directory_iterator(*path), directory_iterator{});
> > +             if (numOfFiles == 0) {
> > +                     LOG(Virtual, Error) << "Empty directory";
> > +                     return -EINVAL;
> > +             }
> > +             data->config_.frame = ImageFrames{ *path, numOfFiles };
> > +     } else {
> > +             LOG(Virtual, Error) << "Frame: " << *path << " is not supported";
> > +             return -EINVAL;
> > +     }
> > [...]
>
> I wanted to reflect a bit on this part. I think the `ImageFrames` type can be made
> more easily understandable as follows:
>
>   struct ImageFrames {
>     std::vector<std::filesystem::path> files;
>   };
>
> and then the above part would become something like this:
>
>   if (!path) {
>     LOG(Virtual, Error) << "Test pattern or path should be specified.";
>     return -EINVAL;
>   }
>
>   std::vector<std::filesystem::path> files;
>
>   switch (std::filesystem::symlink_status(*path).type()) {
>   case std::filesystem::file_type::regular:
>     files.push_back(*path);
>     break;
>   case std::filesystem::file_type::directory:
>     for (const auto& dentry : std::filesystem::directory_iterator { *path })
>       if (dentry.is_regular_file())
>         files.push_back(dentry.path());
>     std::sort(files.begin(), files.end(), [](const auto& a, const auto& b) {
>       return ::strverscmp(a.c_str(), b.c_str()) < 0;
>     });
>     if (files.empty()) {
>       LOG(Virtual, Error) << "Directory has no files: " << *path;
>       return -EINVAL;
>     }
>     break;
>   default:
>     LOG(Virtual, Error) << "Frame: " << *path << " is not supported";
>     return -EINVAL;
>   }
>
>   data->config_.frame = ImageFrames { std::move(files) };
>
> So there would simply be a list of files. I think this is simple, and does not
> need any special code for the "single file" scenario when using an `ImageFrames`
> object.
>
> Note, that the files in the dictionary are sorted using `strverscmp()`, so
> frame-99 should be placed before frame-100, as one would expect.

Yeah this makes sense to me. IIUC, It also allows other file names, instead
of using numbers as the index though.

>
> Also notice that there is no extension checking anymore, as I don't believe
> that to be too useful since it does not guarantee anything, and only jpeg files
> are supported, so whether or not the file can be read will be found out when
> the program tries to load them.

Yeah that's true. Mentioning it in README.md should be enough.

>
> Any thoughts?

Mostly adopted.

Thanks!
Harvey

>
>
> Regards,
> Barnabás Pőcze