| Message ID | 20200228032913.497826-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Fri, Feb 28, 2020 at 04:29:11AM +0100, Niklas Söderlund wrote: > Add a name to each pixel format and extend PixelFormat to be constructed > from a name and to retrieve the name for printing. Make use of the new > functionality to demonstrate it. > > - Update the cam utility to read and print the pixel format name instead > of the fourcc integer number. > > - Update log messages to print the name instead of the fourcc integer > number. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/pixelformats.h | 3 ++ > src/cam/main.cpp | 9 ++--- > src/libcamera/pipeline/uvcvideo.cpp | 4 +- > src/libcamera/pixelformats.cpp | 58 ++++++++++++++++++++--------- > src/libcamera/stream.cpp | 2 +- > 5 files changed, 51 insertions(+), 25 deletions(-) > > diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h > index f0951e983192d5e8..8cea3a90ef2cc7ae 100644 > --- a/include/libcamera/pixelformats.h > +++ b/include/libcamera/pixelformats.h > @@ -24,6 +24,7 @@ public: > PixelFormat(); > PixelFormat(const PixelFormat &other); > explicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers); > + explicit PixelFormat(const std::string &name); Shouldn't this take modifiers too ? > PixelFormat &operator=(const PixelFormat &other); > > @@ -31,6 +32,7 @@ public: > bool operator!=(const PixelFormat &other) const; > bool operator<(const PixelFormat &other) const; > > + const std::string &name() const; > uint32_t v4l2() const; > uint32_t fourcc() const; > const std::set<uint32_t> &modifiers() const; > @@ -47,6 +49,7 @@ protected: > private: > const PixelFormatEntry *fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const; > const PixelFormatEntry *fromV4L2(uint32_t v4l2_fourcc) const; > + const PixelFormatEntry *fromName(const std::string &name) const; > > const PixelFormatEntry *format_; > }; > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index c8ef79daea37d8b6..0a08e362294fc9ee 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -159,7 +159,7 @@ int CamApp::parseOptions(int argc, char *argv[]) > ArgumentRequired); > streamKeyValue.addOption("height", OptionInteger, "Height in pixels", > ArgumentRequired); > - streamKeyValue.addOption("pixelformat", OptionInteger, "Pixel format", > + streamKeyValue.addOption("pixelformat", OptionString, "Pixel format", > ArgumentRequired); > > OptionsParser parser; > @@ -247,9 +247,9 @@ int CamApp::prepareConfig() > if (opt.isSet("height")) > cfg.size.height = opt["height"]; > > - /* TODO: Translate 4CC string to ID. */ > if (opt.isSet("pixelformat")) > - cfg.pixelFormat = PixelFormat(opt["pixelformat"], {}); > + cfg.pixelFormat = > + PixelFormat(opt["pixelformat"].toString()); > } > } > > @@ -282,8 +282,7 @@ int CamApp::infoConfiguration() > > const StreamFormats &formats = cfg.formats(); > for (PixelFormat pixelformat : formats.pixelformats()) { > - std::cout << " * Pixelformat: 0x" << std::hex > - << std::setw(8) << pixelformat.fourcc() << " " > + std::cout << " * Pixelformat: " << pixelformat.name() << " " > << formats.range(pixelformat).toString() > << std::endl; Would it be useful to print both the name and the hex fourcc value ? When debugging we often see the numerical value, being able to compare it with printed messages could be interesting. We could add a toString() to PixelFormat to combine the two: std::string PixeFormat::toString() { std::stringstream ss; ss << name() << " (" << utils::hex(fourcc(), 8) << ")"; return ss.str(); } This could be extended to also take modifiers into account. We don't necessarily need to print both everywhere, but in the cam tool it may be useful. > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index 5f3e52f691aaeae4..75fbfe1eb9145424 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -115,8 +115,8 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() > if (iter == pixelFormats.end()) { > cfg.pixelFormat = pixelFormats.front(); > LOG(UVC, Debug) > - << "Adjusting pixel format from " << pixelFormat.fourcc() > - << " to " << cfg.pixelFormat.fourcc(); > + << "Adjusting pixel format from " << pixelFormat.name() > + << " to " << cfg.pixelFormat.name(); > status = Adjusted; > } > > diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp > index b2aacbc39b9ca16a..70f41d86a23baceb 100644 > --- a/src/libcamera/pixelformats.cpp > +++ b/src/libcamera/pixelformats.cpp > @@ -35,6 +35,7 @@ LOG_DEFINE_CATEGORY(PixelFormats) > */ > > struct PixelFormatEntry { > + std::string name; Would const char * be a bit more efficient ? > uint32_t v4l2; > uint32_t drm; > std::set<uint32_t> modifiers; > @@ -42,27 +43,27 @@ struct PixelFormatEntry { > > static const std::vector<PixelFormatEntry> pixelFormats = { > /* Invalid format, important to be first in list. */ > - { .v4l2 = 0, .drm = DRM_FORMAT_INVALID, .modifiers = {} }, > + { .name = "INVALID", .v4l2 = 0, .drm = DRM_FORMAT_INVALID, .modifiers = {} }, > /* RGB formats. */ > - { .v4l2 = V4L2_PIX_FMT_RGB24, .drm = DRM_FORMAT_BGR888, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_BGR24, .drm = DRM_FORMAT_RGB888, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_ARGB32, .drm = DRM_FORMAT_BGRA8888, .modifiers = {} }, > + { .name = "RGR888", .v4l2 = V4L2_PIX_FMT_RGB24, .drm = DRM_FORMAT_BGR888, .modifiers = {} }, > + { .name = "RGB888", .v4l2 = V4L2_PIX_FMT_BGR24, .drm = DRM_FORMAT_RGB888, .modifiers = {} }, > + { .name = "BGRA8888", .v4l2 = V4L2_PIX_FMT_ARGB32, .drm = DRM_FORMAT_BGRA8888, .modifiers = {} }, > /* YUV packed formats. */ > - { .v4l2 = V4L2_PIX_FMT_YUYV, .drm = DRM_FORMAT_YUYV, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_YVYU, .drm = DRM_FORMAT_YVYU, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_UYVY, .drm = DRM_FORMAT_UYVY, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_VYUY, .drm = DRM_FORMAT_VYUY, .modifiers = {} }, > + { .name = "YUYV", .v4l2 = V4L2_PIX_FMT_YUYV, .drm = DRM_FORMAT_YUYV, .modifiers = {} }, > + { .name = "YVYU", .v4l2 = V4L2_PIX_FMT_YVYU, .drm = DRM_FORMAT_YVYU, .modifiers = {} }, > + { .name = "UYVY", .v4l2 = V4L2_PIX_FMT_UYVY, .drm = DRM_FORMAT_UYVY, .modifiers = {} }, > + { .name = "VYUY", .v4l2 = V4L2_PIX_FMT_VYUY, .drm = DRM_FORMAT_VYUY, .modifiers = {} }, > /* YUY planar formats. */ > - { .v4l2 = V4L2_PIX_FMT_NV16, .drm = DRM_FORMAT_NV16, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_NV16M, .drm = DRM_FORMAT_NV16, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_NV61, .drm = DRM_FORMAT_NV61, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_NV61M, .drm = DRM_FORMAT_NV61, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_NV12, .drm = DRM_FORMAT_NV12, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_NV12M, .drm = DRM_FORMAT_NV12, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_NV21, .drm = DRM_FORMAT_NV21, .modifiers = {} }, > - { .v4l2 = V4L2_PIX_FMT_NV21M, .drm = DRM_FORMAT_NV21, .modifiers = {} }, > + { .name = "NV16", .v4l2 = V4L2_PIX_FMT_NV16, .drm = DRM_FORMAT_NV16, .modifiers = {} }, > + { .name = "NV16M", .v4l2 = V4L2_PIX_FMT_NV16M, .drm = DRM_FORMAT_NV16, .modifiers = {} }, > + { .name = "NV61", .v4l2 = V4L2_PIX_FMT_NV61, .drm = DRM_FORMAT_NV61, .modifiers = {} }, > + { .name = "NV61M", .v4l2 = V4L2_PIX_FMT_NV61M, .drm = DRM_FORMAT_NV61, .modifiers = {} }, > + { .name = "NV12", .v4l2 = V4L2_PIX_FMT_NV12, .drm = DRM_FORMAT_NV12, .modifiers = {} }, > + { .name = "NV12M", .v4l2 = V4L2_PIX_FMT_NV12M, .drm = DRM_FORMAT_NV12, .modifiers = {} }, > + { .name = "NV21", .v4l2 = V4L2_PIX_FMT_NV21, .drm = DRM_FORMAT_NV21, .modifiers = {} }, > + { .name = "NV21M", .v4l2 = V4L2_PIX_FMT_NV21M, .drm = DRM_FORMAT_NV21, .modifiers = {} }, > /* Compressed formats. */ > - { .v4l2 = V4L2_PIX_FMT_MJPEG, .drm = DRM_FORMAT_MJPEG, .modifiers = {} }, > + { .name = "MJPEG", .v4l2 = V4L2_PIX_FMT_MJPEG, .drm = DRM_FORMAT_MJPEG, .modifiers = {} }, > }; > > PixelFormat::PixelFormat() > @@ -85,6 +86,11 @@ PixelFormat::PixelFormat(uint32_t v4l2_fourcc) > { > } > > +PixelFormat::PixelFormat(const std::string &name) > + : format_(fromName(name)) > +{ > +} > + > PixelFormat &PixelFormat::operator=(const PixelFormat &other) > { > format_ = other.format_; > @@ -107,6 +113,11 @@ bool PixelFormat::operator<(const PixelFormat &other) const > return format_ > other.format_; > } > > +const std::string &PixelFormat::name() const > +{ > + return format_->name; > +} > + > uint32_t PixelFormat::v4l2() const > { > return format_->v4l2; > @@ -149,4 +160,17 @@ const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const > return &pixelFormats[0]; > } > > +const PixelFormatEntry *PixelFormat::fromName(const std::string &name) const > +{ > + for (const PixelFormatEntry &entry : pixelFormats) for (const PixelFormatEntry &entry : pixelFormats) { > + if (entry.name == name) > + return &entry; } > + > + LOG(PixelFormats, Error) > + << "Unsupported name for pixel format " > + << name; > + > + return &pixelFormats[0]; I think nullptr would be better, and I'm not sure I would log a message here. I'll review 3/6, we can discuss the issue there. > +} > + > } /* namespace libcamera */ > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index dbce550ca8d0b7b1..e82ffd8f3b211925 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -348,7 +348,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats) > std::string StreamConfiguration::toString() const > { > std::stringstream ss; > - ss << size.toString() << "-" << utils::hex(pixelFormat.fourcc()); > + ss << size.toString() << "-" << pixelFormat.name(); > return ss.str(); > } >
diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h index f0951e983192d5e8..8cea3a90ef2cc7ae 100644 --- a/include/libcamera/pixelformats.h +++ b/include/libcamera/pixelformats.h @@ -24,6 +24,7 @@ public: PixelFormat(); PixelFormat(const PixelFormat &other); explicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers); + explicit PixelFormat(const std::string &name); PixelFormat &operator=(const PixelFormat &other); @@ -31,6 +32,7 @@ public: bool operator!=(const PixelFormat &other) const; bool operator<(const PixelFormat &other) const; + const std::string &name() const; uint32_t v4l2() const; uint32_t fourcc() const; const std::set<uint32_t> &modifiers() const; @@ -47,6 +49,7 @@ protected: private: const PixelFormatEntry *fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const; const PixelFormatEntry *fromV4L2(uint32_t v4l2_fourcc) const; + const PixelFormatEntry *fromName(const std::string &name) const; const PixelFormatEntry *format_; }; diff --git a/src/cam/main.cpp b/src/cam/main.cpp index c8ef79daea37d8b6..0a08e362294fc9ee 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -159,7 +159,7 @@ int CamApp::parseOptions(int argc, char *argv[]) ArgumentRequired); streamKeyValue.addOption("height", OptionInteger, "Height in pixels", ArgumentRequired); - streamKeyValue.addOption("pixelformat", OptionInteger, "Pixel format", + streamKeyValue.addOption("pixelformat", OptionString, "Pixel format", ArgumentRequired); OptionsParser parser; @@ -247,9 +247,9 @@ int CamApp::prepareConfig() if (opt.isSet("height")) cfg.size.height = opt["height"]; - /* TODO: Translate 4CC string to ID. */ if (opt.isSet("pixelformat")) - cfg.pixelFormat = PixelFormat(opt["pixelformat"], {}); + cfg.pixelFormat = + PixelFormat(opt["pixelformat"].toString()); } } @@ -282,8 +282,7 @@ int CamApp::infoConfiguration() const StreamFormats &formats = cfg.formats(); for (PixelFormat pixelformat : formats.pixelformats()) { - std::cout << " * Pixelformat: 0x" << std::hex - << std::setw(8) << pixelformat.fourcc() << " " + std::cout << " * Pixelformat: " << pixelformat.name() << " " << formats.range(pixelformat).toString() << std::endl; diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index 5f3e52f691aaeae4..75fbfe1eb9145424 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -115,8 +115,8 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() if (iter == pixelFormats.end()) { cfg.pixelFormat = pixelFormats.front(); LOG(UVC, Debug) - << "Adjusting pixel format from " << pixelFormat.fourcc() - << " to " << cfg.pixelFormat.fourcc(); + << "Adjusting pixel format from " << pixelFormat.name() + << " to " << cfg.pixelFormat.name(); status = Adjusted; } diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp index b2aacbc39b9ca16a..70f41d86a23baceb 100644 --- a/src/libcamera/pixelformats.cpp +++ b/src/libcamera/pixelformats.cpp @@ -35,6 +35,7 @@ LOG_DEFINE_CATEGORY(PixelFormats) */ struct PixelFormatEntry { + std::string name; uint32_t v4l2; uint32_t drm; std::set<uint32_t> modifiers; @@ -42,27 +43,27 @@ struct PixelFormatEntry { static const std::vector<PixelFormatEntry> pixelFormats = { /* Invalid format, important to be first in list. */ - { .v4l2 = 0, .drm = DRM_FORMAT_INVALID, .modifiers = {} }, + { .name = "INVALID", .v4l2 = 0, .drm = DRM_FORMAT_INVALID, .modifiers = {} }, /* RGB formats. */ - { .v4l2 = V4L2_PIX_FMT_RGB24, .drm = DRM_FORMAT_BGR888, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_BGR24, .drm = DRM_FORMAT_RGB888, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_ARGB32, .drm = DRM_FORMAT_BGRA8888, .modifiers = {} }, + { .name = "RGR888", .v4l2 = V4L2_PIX_FMT_RGB24, .drm = DRM_FORMAT_BGR888, .modifiers = {} }, + { .name = "RGB888", .v4l2 = V4L2_PIX_FMT_BGR24, .drm = DRM_FORMAT_RGB888, .modifiers = {} }, + { .name = "BGRA8888", .v4l2 = V4L2_PIX_FMT_ARGB32, .drm = DRM_FORMAT_BGRA8888, .modifiers = {} }, /* YUV packed formats. */ - { .v4l2 = V4L2_PIX_FMT_YUYV, .drm = DRM_FORMAT_YUYV, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_YVYU, .drm = DRM_FORMAT_YVYU, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_UYVY, .drm = DRM_FORMAT_UYVY, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_VYUY, .drm = DRM_FORMAT_VYUY, .modifiers = {} }, + { .name = "YUYV", .v4l2 = V4L2_PIX_FMT_YUYV, .drm = DRM_FORMAT_YUYV, .modifiers = {} }, + { .name = "YVYU", .v4l2 = V4L2_PIX_FMT_YVYU, .drm = DRM_FORMAT_YVYU, .modifiers = {} }, + { .name = "UYVY", .v4l2 = V4L2_PIX_FMT_UYVY, .drm = DRM_FORMAT_UYVY, .modifiers = {} }, + { .name = "VYUY", .v4l2 = V4L2_PIX_FMT_VYUY, .drm = DRM_FORMAT_VYUY, .modifiers = {} }, /* YUY planar formats. */ - { .v4l2 = V4L2_PIX_FMT_NV16, .drm = DRM_FORMAT_NV16, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_NV16M, .drm = DRM_FORMAT_NV16, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_NV61, .drm = DRM_FORMAT_NV61, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_NV61M, .drm = DRM_FORMAT_NV61, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_NV12, .drm = DRM_FORMAT_NV12, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_NV12M, .drm = DRM_FORMAT_NV12, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_NV21, .drm = DRM_FORMAT_NV21, .modifiers = {} }, - { .v4l2 = V4L2_PIX_FMT_NV21M, .drm = DRM_FORMAT_NV21, .modifiers = {} }, + { .name = "NV16", .v4l2 = V4L2_PIX_FMT_NV16, .drm = DRM_FORMAT_NV16, .modifiers = {} }, + { .name = "NV16M", .v4l2 = V4L2_PIX_FMT_NV16M, .drm = DRM_FORMAT_NV16, .modifiers = {} }, + { .name = "NV61", .v4l2 = V4L2_PIX_FMT_NV61, .drm = DRM_FORMAT_NV61, .modifiers = {} }, + { .name = "NV61M", .v4l2 = V4L2_PIX_FMT_NV61M, .drm = DRM_FORMAT_NV61, .modifiers = {} }, + { .name = "NV12", .v4l2 = V4L2_PIX_FMT_NV12, .drm = DRM_FORMAT_NV12, .modifiers = {} }, + { .name = "NV12M", .v4l2 = V4L2_PIX_FMT_NV12M, .drm = DRM_FORMAT_NV12, .modifiers = {} }, + { .name = "NV21", .v4l2 = V4L2_PIX_FMT_NV21, .drm = DRM_FORMAT_NV21, .modifiers = {} }, + { .name = "NV21M", .v4l2 = V4L2_PIX_FMT_NV21M, .drm = DRM_FORMAT_NV21, .modifiers = {} }, /* Compressed formats. */ - { .v4l2 = V4L2_PIX_FMT_MJPEG, .drm = DRM_FORMAT_MJPEG, .modifiers = {} }, + { .name = "MJPEG", .v4l2 = V4L2_PIX_FMT_MJPEG, .drm = DRM_FORMAT_MJPEG, .modifiers = {} }, }; PixelFormat::PixelFormat() @@ -85,6 +86,11 @@ PixelFormat::PixelFormat(uint32_t v4l2_fourcc) { } +PixelFormat::PixelFormat(const std::string &name) + : format_(fromName(name)) +{ +} + PixelFormat &PixelFormat::operator=(const PixelFormat &other) { format_ = other.format_; @@ -107,6 +113,11 @@ bool PixelFormat::operator<(const PixelFormat &other) const return format_ > other.format_; } +const std::string &PixelFormat::name() const +{ + return format_->name; +} + uint32_t PixelFormat::v4l2() const { return format_->v4l2; @@ -149,4 +160,17 @@ const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const return &pixelFormats[0]; } +const PixelFormatEntry *PixelFormat::fromName(const std::string &name) const +{ + for (const PixelFormatEntry &entry : pixelFormats) + if (entry.name == name) + return &entry; + + LOG(PixelFormats, Error) + << "Unsupported name for pixel format " + << name; + + return &pixelFormats[0]; +} + } /* namespace libcamera */ diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp index dbce550ca8d0b7b1..e82ffd8f3b211925 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -348,7 +348,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats) std::string StreamConfiguration::toString() const { std::stringstream ss; - ss << size.toString() << "-" << utils::hex(pixelFormat.fourcc()); + ss << size.toString() << "-" << pixelFormat.name(); return ss.str(); }
Add a name to each pixel format and extend PixelFormat to be constructed from a name and to retrieve the name for printing. Make use of the new functionality to demonstrate it. - Update the cam utility to read and print the pixel format name instead of the fourcc integer number. - Update log messages to print the name instead of the fourcc integer number. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/pixelformats.h | 3 ++ src/cam/main.cpp | 9 ++--- src/libcamera/pipeline/uvcvideo.cpp | 4 +- src/libcamera/pixelformats.cpp | 58 ++++++++++++++++++++--------- src/libcamera/stream.cpp | 2 +- 5 files changed, 51 insertions(+), 25 deletions(-)