[PATCH 2/2] apps: cam: Print an error when outputting DNG and DNG support is missing
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 26 16:25:07 CEST 2024
On Thu, Sep 26, 2024 at 03:59:42PM +0200, Jacopo Mondi wrote:
> Hi Laurent
>
> On Wed, Sep 25, 2024 at 06:21:34PM GMT, Laurent Pinchart wrote:
> > When DNG support is missing, the cam application ignores the .dng suffix
> > of the file pattern and writes raw binary data instead, without
> > notifying the user. This leads to confusion. Fix it by printing an error
> > message.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/apps/cam/camera_session.cpp | 15 ++++++---
> > src/apps/cam/file_sink.cpp | 60 +++++++++++++++++++++++----------
> > src/apps/cam/file_sink.h | 18 ++++++++--
> > 3 files changed, 68 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > index 097dc479241a..227df9e922ea 100644
> > --- a/src/apps/cam/camera_session.cpp
> > +++ b/src/apps/cam/camera_session.cpp
> > @@ -230,11 +230,16 @@ int CameraSession::start()
> > #endif
> >
> > if (options_.isSet(OptFile)) {
> > - if (!options_[OptFile].toString().empty())
> > - sink_ = std::make_unique<FileSink>(camera_.get(), streamNames_,
> > - options_[OptFile]);
> > - else
> > - sink_ = std::make_unique<FileSink>(camera_.get(), streamNames_);
> > + std::unique_ptr<FileSink> sink =
> > + std::make_unique<FileSink>(camera_.get(), streamNames_);
> > +
> > + if (!options_[OptFile].toString().empty()) {
> > + ret = sink->setFilePattern(options_[OptFile]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + sink_ = std::move(sink);
> > }
> >
> > if (sink_) {
> > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp
> > index 3e000d2fd9c6..76e21db9bf9a 100644
> > --- a/src/apps/cam/file_sink.cpp
> > +++ b/src/apps/cam/file_sink.cpp
> > @@ -5,6 +5,7 @@
> > * File Sink
> > */
> >
> > +#include <array>
> > #include <assert.h>
> > #include <fcntl.h>
> > #include <iomanip>
> > @@ -12,6 +13,7 @@
> > #include <sstream>
> > #include <string.h>
> > #include <unistd.h>
> > +#include <utility>
> >
> > #include <libcamera/camera.h>
> >
> > @@ -24,13 +26,13 @@
> > using namespace libcamera;
> >
> > FileSink::FileSink([[maybe_unused]] const libcamera::Camera *camera,
> > - const std::map<const libcamera::Stream *, std::string> &streamNames,
> > - const std::string &pattern)
> > + const std::map<const libcamera::Stream *, std::string> &streamNames)
> > :
> > #ifdef HAVE_TIFF
> > camera_(camera),
> > #endif
>
> I wonder if we actually need this or we could initialize the camera_
> member unconditionally (and drop the above [[maybe_unused]])
I thought about that too but decided not to change it as it's a separate
issue.
> > - streamNames_(streamNames), pattern_(pattern)
> > + pattern_(kDefaultFilePattern), fileType_(FileType::Binary),
> > + streamNames_(streamNames)
> > {
> > }
> >
> > @@ -38,6 +40,41 @@ FileSink::~FileSink()
> > {
> > }
> >
> > +int FileSink::setFilePattern(const std::string &pattern)
> > +{
> > + static const std::array<std::pair<std::string, FileType>, 2> types{{
> > + { ".dng", FileType::Dng },
> > + { ".ppm", FileType::Ppm },
> > + }};
> > +
> > + pattern_ = pattern;
> > +
> > + if (pattern_.empty() || pattern_.back() == '/')
> > + pattern_ += kDefaultFilePattern;
> > +
> > + fileType_ = FileType::Binary;
> > +
> > + for (const auto &type : types) {
> > + if (pattern_.size() < type.first.size())
> > + continue;
> > +
> > + if (pattern_.find(type.first, pattern_.size() - type.first.size()) !=
>
> Or
> if (pattern_.rfind(type.first) != std::string::npos)
We want to check if pattern_ ends with type.first, not if type.first is
found somewhere inside pattern_. C++20 has a nice
std::string::ends_with()
(https://en.cppreference.com/w/cpp/string/basic_string/ends_with), but
C++17 doesn't :-(
> The rest looks good to me
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> > + std::string::npos) {
> > + fileType_ = type.second;
> > + break;
> > + }
> > + }
> > +
> > +#ifndef HAVE_TIFF
> > + if (fileType_ == FileType::Dng) {
> > + std::cerr << "DNG support not available" << std::endl;
> > + return -EINVAL;
> > + }
> > +#endif /* HAVE_TIFF */
> > +
> > + return 0;
> > +}
> > +
> > int FileSink::configure(const libcamera::CameraConfiguration &config)
> > {
> > int ret = FrameSink::configure(config);
> > @@ -67,21 +104,10 @@ bool FileSink::processRequest(Request *request)
> > void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
> > [[maybe_unused]] const ControlList &metadata)
> > {
> > - std::string filename;
> > + std::string filename = pattern_;
> > size_t pos;
> > int fd, ret = 0;
> >
> > - if (!pattern_.empty())
> > - filename = pattern_;
> > -
> > -#ifdef HAVE_TIFF
> > - bool dng = filename.find(".dng", filename.size() - 4) != std::string::npos;
> > -#endif /* HAVE_TIFF */
> > - bool ppm = filename.find(".ppm", filename.size() - 4) != std::string::npos;
> > -
> > - if (filename.empty() || filename.back() == '/')
> > - filename += "frame-#.bin";
> > -
> > pos = filename.find_first_of('#');
> > if (pos != std::string::npos) {
> > std::stringstream ss;
> > @@ -93,7 +119,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
> > Image *image = mappedBuffers_[buffer].get();
> >
> > #ifdef HAVE_TIFF
> > - if (dng) {
> > + if (fileType_ == FileType::Dng) {
> > ret = DNGWriter::write(filename.c_str(), camera_,
> > stream->configuration(), metadata,
> > buffer, image->data(0).data());
> > @@ -104,7 +130,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
> > return;
> > }
> > #endif /* HAVE_TIFF */
> > - if (ppm) {
> > + if (fileType_ == FileType::Ppm) {
> > ret = PPMWriter::write(filename.c_str(), stream->configuration(),
> > image->data(0));
> > if (ret < 0)
> > diff --git a/src/apps/cam/file_sink.h b/src/apps/cam/file_sink.h
> > index 9d560783af09..71b7fe0feab5 100644
> > --- a/src/apps/cam/file_sink.h
> > +++ b/src/apps/cam/file_sink.h
> > @@ -21,10 +21,11 @@ class FileSink : public FrameSink
> > {
> > public:
> > FileSink(const libcamera::Camera *camera,
> > - const std::map<const libcamera::Stream *, std::string> &streamNames,
> > - const std::string &pattern = "");
> > + const std::map<const libcamera::Stream *, std::string> &streamNames);
> > ~FileSink();
> >
> > + int setFilePattern(const std::string &pattern);
> > +
> > int configure(const libcamera::CameraConfiguration &config) override;
> >
> > void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > @@ -32,6 +33,14 @@ public:
> > bool processRequest(libcamera::Request *request) override;
> >
> > private:
> > + static constexpr const char *kDefaultFilePattern = "frame-#.bin";
> > +
> > + enum class FileType {
> > + Binary,
> > + Dng,
> > + Ppm,
> > + };
> > +
> > void writeBuffer(const libcamera::Stream *stream,
> > libcamera::FrameBuffer *buffer,
> > const libcamera::ControlList &metadata);
> > @@ -39,7 +48,10 @@ private:
> > #ifdef HAVE_TIFF
> > const libcamera::Camera *camera_;
> > #endif
> > - std::map<const libcamera::Stream *, std::string> streamNames_;
> > +
> > std::string pattern_;
> > + FileType fileType_;
> > +
> > + std::map<const libcamera::Stream *, std::string> streamNames_;
> > std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;
> > };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list