| Message ID | 20261008211149.51765-3-robert.mader@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2026. 10. 08. 23:11 keltezéssel, Robert Mader írta: > While NV12 is the most common native format for hardware video encoders, > YUV420 is the equivalent for most software video encoders. Thus > supporting both is great for testing. > > Similarly having some RGB formats with and without alpha can be very > useful, especially as the swISP is currently limited to such formats. > > This is a polished version of a hack that was used to help debugging and > testing https://webrtc-review.googlesource.com/c/src/+/491680 - and so > helpful with that that I'd like to see it upstream. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > .../pipeline/virtual/frame_generator.h | 3 +- > .../virtual/image_frame_generator.cpp | 56 ++++++++++++++++--- > .../pipeline/virtual/image_frame_generator.h | 2 +- > .../virtual/test_pattern_generator.cpp | 46 +++++++++++++-- > .../pipeline/virtual/test_pattern_generator.h | 2 +- > src/libcamera/pipeline/virtual/virtual.cpp | 39 +++++++++---- > 6 files changed, 121 insertions(+), 27 deletions(-) > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h > index a0658c45cd82..70cd92907b63 100644 > --- a/src/libcamera/pipeline/virtual/frame_generator.h > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > @@ -9,6 +9,7 @@ > > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > +#include <libcamera/stream.h> > > namespace libcamera { > > @@ -19,7 +20,7 @@ public: > > virtual void configure(const Size &size) = 0; > > - virtual int generateFrame(const Size &size, > + virtual int generateFrame(const StreamConfiguration *cfg, const StreamConfiguration &cfg > const FrameBuffer *buffer) = 0; > > protected: > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > index 1853bddeca27..dcef90702e18 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -14,7 +14,9 @@ > > #include <libcamera/framebuffer.h> > > +#include <libcamera/formats.h> > #include "libcamera/internal/dma_buf_allocator.h" > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/mapped_framebuffer.h" > > #include "libyuv/convert.h" > @@ -124,8 +126,10 @@ void ImageFrameGenerator::configure(const Size &size) > } > } > > -int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer) > +int ImageFrameGenerator::generateFrame(const StreamConfiguration *cfg, const FrameBuffer *buffer) > { > + auto size = cfg->size; > + > ASSERT(!scaledFrameDatas_.empty()); > > /* > @@ -146,11 +150,49 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > frameIndex_ %= imageFrameDatas_.size(); > > /* Write the scaledY and scaledUV to the mapped frame buffer */ > - libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), size.width, > - scaledFrameDatas_[frameIndex_].UV.get(), size.width, > - planes[0].data(), size.width, > - planes[1].data(), size.width, > - size.width, size.height); > + int ret; > + const PixelFormatInfo &info = PixelFormatInfo::info(cfg->pixelFormat); I think `size` should be move around here. > + switch (cfg->pixelFormat) { > + case formats::NV12: > + ret = libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), > + size.width, > + scaledFrameDatas_[frameIndex_].UV.get(), > + size.width, > + planes[0].data(), > + info.stride(size.width, 0), > + planes[1].data(), > + info.stride(size.width, 1), > + size.width, size.height); > + break; > + case formats::YUV420: > + ret = libyuv::NV12ToI420(scaledFrameDatas_[frameIndex_].Y.get(), > + size.width, > + scaledFrameDatas_[frameIndex_].UV.get(), > + size.width, > + planes[0].data(), > + info.stride(size.width, 0), > + planes[1].data(), > + info.stride(size.width, 1), > + planes[2].data(), > + info.stride(size.width, 2), > + size.width, size.height); > + break; > + case formats::ABGR8888: > + case formats::XBGR8888: > + ret = libyuv::NV12ToARGB(scaledFrameDatas_[frameIndex_].Y.get(), > + size.width, > + scaledFrameDatas_[frameIndex_].UV.get(), > + size.width, > + planes[0].data(), > + info.stride(size.width, 0), > + size.width, size.height); > + break; > + default: > + LOG(Virtual, Error) << "Unsupported format"; > + return -1; > + } > + if (ret != 0) > + LOG(Virtual, Error) << "NV12 conversation failed with " << ret; conversion but I would just say "conversion failed: " << ret as "NV12 conversion", at least to me, implies a conversion *to* NV12 and not from. > > /* Proceed to the next image every 4 frames */ > /* \todo Consider setting the frameRepeat in the config file */ > @@ -158,7 +200,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > if (parameter_ % frameRepeat == 0) > frameIndex_++; > > - return 0; > + return ret; I wonder if it would be better to just use `bool`. > } > > /* > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > index 42a077ba803c..575900815d9e 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.h > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > @@ -37,7 +37,7 @@ private: > }; > > void configure(const Size &size) override; > - int generateFrame(const Size &size, const FrameBuffer *buffer) override; > + int generateFrame(const StreamConfiguration *cfg, const FrameBuffer *buffer) override; > > std::vector<ImageFrameData> imageFrameDatas_; > std::vector<ImageFrameData> scaledFrameDatas_; > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > index 7b53e8b32327..d8cdc1c91c24 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > @@ -11,7 +11,9 @@ > > #include <libcamera/base/log.h> > > +#include <libcamera/formats.h> > #include "libcamera/internal/dma_buf_allocator.h" > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/mapped_framebuffer.h" > > #include <libyuv/convert_from_argb.h> > @@ -42,7 +44,7 @@ LOG_DECLARE_CATEGORY(Virtual) > > static const unsigned int kARGBSize = 4; > > -int TestPatternGenerator::generateFrame(const Size &size, > +int TestPatternGenerator::generateFrame(const StreamConfiguration *cfg, > const FrameBuffer *buffer) > { > /* > @@ -58,16 +60,48 @@ int TestPatternGenerator::generateFrame(const Size &size, > MappedFrameBuffer::MapFlag::Write); > > const auto &planes = mappedFrameBuffer.planes(); > + const auto size = cfg->size; > > rotateLeft1Column<kARGBSize>(size, template_.get()); > > /* Convert the template_ to the frame buffer */ > - int ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize, > - planes[0].data(), size.width, > - planes[1].data(), size.width, > - size.width, size.height); > + int ret; > + const PixelFormatInfo &info = PixelFormatInfo::info(cfg->pixelFormat); > + switch (cfg->pixelFormat) { > + case formats::NV12: > + ret = libyuv::ARGBToNV12(template_.get(), > + size.width * kARGBSize, > + planes[0].data(), > + info.stride(size.width, 0), > + planes[1].data(), > + info.stride(size.width, 1), > + size.width, size.height); > + break; > + case formats::YUV420: > + ret = libyuv::ARGBToI420(template_.get(), > + size.width * kARGBSize, > + planes[0].data(), > + info.stride(size.width, 0), > + planes[1].data(), > + info.stride(size.width, 1), > + planes[2].data(), > + info.stride(size.width, 2), > + size.width, size.height); > + break; > + case formats::ABGR8888: > + case formats::XBGR8888: > + ret = libyuv::ARGBCopy(template_.get(), > + size.width * kARGBSize, > + planes[0].data(), > + info.stride(size.width, 0), > + size.width, size.height); > + break; > + default: > + LOG(Virtual, Error) << "Unsupported format"; > + return -1; > + } > if (ret != 0) > - LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret; > + LOG(Virtual, Error) << "ARGB conversation failed with " << ret; Most of the previous comments apply to this function as well. > > return ret; > } > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h > index 2a51bd31abdf..5022eaff3d30 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h > @@ -24,7 +24,7 @@ enum class TestPattern : char { > class TestPatternGenerator : public FrameGenerator > { > public: > - int generateFrame(const Size &size, const FrameBuffer *buffer) override; > + int generateFrame(const StreamConfiguration *cfg, const FrameBuffer *buffer) override; > > protected: > /* Buffer of test pattern template */ > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index e25dd85a7391..565b66477363 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -148,7 +148,7 @@ void VirtualCameraData::processRequest(Request *request) > found = true; > > if (streamConfig.frameGenerator->generateFrame( > - stream->configuration().size, buffer)) > + &stream->configuration(), buffer)) > fmd.status = FrameMetadata::Status::FrameError; > > bufferCompleted.emit(buffer); > @@ -202,16 +202,28 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > adjusted = true; > } Let's add constexpr PixelFormat outputFormats[] = { ... }; to the anon namespace in the file. And then it can be used here and in `generateConfiguration()`. > > - if (cfg.pixelFormat != formats::NV12) { > + if (cfg.pixelFormat != formats::NV12 && > + cfg.pixelFormat != formats::YUV420 && > + cfg.pixelFormat != formats::ABGR8888 && > + cfg.pixelFormat != formats::XBGR8888) { if (std::find(std::begin(outputFormats), std::end(outputFormats), cfg.pixelFormat) == std::end(outputFormats)) or an equivalent you like. > cfg.pixelFormat = formats::NV12; > status = Adjusted; > adjusted = true; > } > > - if (cfg.colorSpace != ColorSpace::Smpte170m) { > - cfg.colorSpace = ColorSpace::Smpte170m; > - status = Adjusted; > - adjusted = true; > + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRGB) { > + if (cfg.colorSpace != ColorSpace::Srgb) { > + cfg.colorSpace = ColorSpace::Srgb; > + status = Adjusted; > + adjusted = true; > + } > + } else if (info.colourEncoding == PixelFormatInfo::ColourEncodingYUV) { > + if (cfg.colorSpace != ColorSpace::Smpte170m) { > + cfg.colorSpace = ColorSpace::Smpte170m; > + status = Adjusted; > + adjusted = true; > + } > } std::optional<ColorSpace> colorSpace; switch (info.colourEncoding) { case ...: colorSpace = ...; default: ASSERT(false); } if (cfg.colorSpace != colorSpace) ... > > if (validateColorSpaces() == Adjusted) { > @@ -223,7 +235,6 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > LOG(Virtual, Info) > << "Stream configuration adjusted to " << cfg.toString(); > > - const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); > cfg.stride = info.stride(cfg.size.width, 0, 1); > cfg.frameSize = info.frameSize(cfg.size, 1); > > @@ -274,13 +285,19 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > return {}; > } > > + std::vector<PixelFormat> outputFormats = { formats::NV12, > + formats::YUV420, > + formats::ABGR8888, > + formats::XBGR8888 }; > + > std::map<PixelFormat, std::vector<SizeRange>> streamFormats; > - PixelFormat pixelFormat = formats::NV12; > - streamFormats[pixelFormat] = { { data->config_.minResolutionSize, > - data->config_.maxResolutionSize } }; > + for (auto pixelFormat : outputFormats) { > + streamFormats[pixelFormat] = { { data->config_.minResolutionSize, > + data->config_.maxResolutionSize } }; > + } > StreamFormats formats(streamFormats); > StreamConfiguration cfg(formats); > - cfg.pixelFormat = pixelFormat; > + cfg.pixelFormat = outputFormats.front(); > cfg.size = data->config_.maxResolutionSize; > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > cfg.colorSpace = ColorSpace::Smpte170m;
Hi, thanks for the review! On 09.10.26 10:43, Barnabás Pőcze wrote: > Hi > > 2026. 10. 08. 23:11 keltezéssel, Robert Mader írta: >> While NV12 is the most common native format for hardware video encoders, >> YUV420 is the equivalent for most software video encoders. Thus >> supporting both is great for testing. >> >> Similarly having some RGB formats with and without alpha can be very >> useful, especially as the swISP is currently limited to such formats. >> >> This is a polished version of a hack that was used to help debugging and >> testing https://webrtc-review.googlesource.com/c/src/+/491680 - and so >> helpful with that that I'd like to see it upstream. >> >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> --- >> .../pipeline/virtual/frame_generator.h | 3 +- >> .../virtual/image_frame_generator.cpp | 56 ++++++++++++++++--- >> .../pipeline/virtual/image_frame_generator.h | 2 +- >> .../virtual/test_pattern_generator.cpp | 46 +++++++++++++-- >> .../pipeline/virtual/test_pattern_generator.h | 2 +- >> src/libcamera/pipeline/virtual/virtual.cpp | 39 +++++++++---- >> 6 files changed, 121 insertions(+), 27 deletions(-) >> >> diff --git a/src/libcamera/pipeline/virtual/frame_generator.h >> b/src/libcamera/pipeline/virtual/frame_generator.h >> index a0658c45cd82..70cd92907b63 100644 >> --- a/src/libcamera/pipeline/virtual/frame_generator.h >> +++ b/src/libcamera/pipeline/virtual/frame_generator.h >> @@ -9,6 +9,7 @@ >> #include <libcamera/framebuffer.h> >> #include <libcamera/geometry.h> >> +#include <libcamera/stream.h> >> namespace libcamera { >> @@ -19,7 +20,7 @@ public: >> virtual void configure(const Size &size) = 0; >> - virtual int generateFrame(const Size &size, >> + virtual int generateFrame(const StreamConfiguration *cfg, > > const StreamConfiguration &cfg > > >> const FrameBuffer *buffer) = 0; >> protected: >> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp >> b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >> index 1853bddeca27..dcef90702e18 100644 >> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp >> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >> @@ -14,7 +14,9 @@ >> #include <libcamera/framebuffer.h> >> +#include <libcamera/formats.h> >> #include "libcamera/internal/dma_buf_allocator.h" >> +#include "libcamera/internal/formats.h" >> #include "libcamera/internal/mapped_framebuffer.h" >> #include "libyuv/convert.h" >> @@ -124,8 +126,10 @@ void ImageFrameGenerator::configure(const Size >> &size) >> } >> } >> -int ImageFrameGenerator::generateFrame(const Size &size, const >> FrameBuffer *buffer) >> +int ImageFrameGenerator::generateFrame(const StreamConfiguration >> *cfg, const FrameBuffer *buffer) >> { >> + auto size = cfg->size; >> + >> ASSERT(!scaledFrameDatas_.empty()); >> /* >> @@ -146,11 +150,49 @@ int ImageFrameGenerator::generateFrame(const >> Size &size, const FrameBuffer *buff >> frameIndex_ %= imageFrameDatas_.size(); >> /* Write the scaledY and scaledUV to the mapped frame buffer */ >> - libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), >> size.width, >> - scaledFrameDatas_[frameIndex_].UV.get(), size.width, >> - planes[0].data(), size.width, >> - planes[1].data(), size.width, >> - size.width, size.height); >> + int ret; >> + const PixelFormatInfo &info = >> PixelFormatInfo::info(cfg->pixelFormat); > > I think `size` should be move around here. > > > >> + switch (cfg->pixelFormat) { >> + case formats::NV12: >> + ret = libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), >> + size.width, >> + scaledFrameDatas_[frameIndex_].UV.get(), >> + size.width, >> + planes[0].data(), >> + info.stride(size.width, 0), >> + planes[1].data(), >> + info.stride(size.width, 1), >> + size.width, size.height); >> + break; >> + case formats::YUV420: >> + ret = >> libyuv::NV12ToI420(scaledFrameDatas_[frameIndex_].Y.get(), >> + size.width, >> + scaledFrameDatas_[frameIndex_].UV.get(), >> + size.width, >> + planes[0].data(), >> + info.stride(size.width, 0), >> + planes[1].data(), >> + info.stride(size.width, 1), >> + planes[2].data(), >> + info.stride(size.width, 2), >> + size.width, size.height); >> + break; >> + case formats::ABGR8888: >> + case formats::XBGR8888: >> + ret = >> libyuv::NV12ToARGB(scaledFrameDatas_[frameIndex_].Y.get(), >> + size.width, >> + scaledFrameDatas_[frameIndex_].UV.get(), >> + size.width, >> + planes[0].data(), >> + info.stride(size.width, 0), >> + size.width, size.height); >> + break; >> + default: >> + LOG(Virtual, Error) << "Unsupported format"; >> + return -1; >> + } >> + if (ret != 0) >> + LOG(Virtual, Error) << "NV12 conversation failed with " << ret; > > conversion > > but I would just say > > "conversion failed: " << ret > > as "NV12 conversion", at least to me, implies a conversion *to* NV12 > and not from. > > >> /* Proceed to the next image every 4 frames */ >> /* \todo Consider setting the frameRepeat in the config file */ >> @@ -158,7 +200,7 @@ int ImageFrameGenerator::generateFrame(const Size >> &size, const FrameBuffer *buff >> if (parameter_ % frameRepeat == 0) >> frameIndex_++; >> - return 0; >> + return ret; > > I wonder if it would be better to just use `bool`. I applied all suggestions but this one - it's not a big difference, but the libyuv funcs return int, so I consider it slightly nicer to get the actual code. It shouldn't happen under normal circumstances anyway. Happy to change it if you feel strongly about it, though. Regards > > >> } >> /* >> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h >> b/src/libcamera/pipeline/virtual/image_frame_generator.h >> index 42a077ba803c..575900815d9e 100644 >> --- a/src/libcamera/pipeline/virtual/image_frame_generator.h >> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h >> @@ -37,7 +37,7 @@ private: >> }; >> void configure(const Size &size) override; >> - int generateFrame(const Size &size, const FrameBuffer *buffer) >> override; >> + int generateFrame(const StreamConfiguration *cfg, const >> FrameBuffer *buffer) override; >> std::vector<ImageFrameData> imageFrameDatas_; >> std::vector<ImageFrameData> scaledFrameDatas_; >> diff --git >> a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >> b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >> index 7b53e8b32327..d8cdc1c91c24 100644 >> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >> @@ -11,7 +11,9 @@ >> #include <libcamera/base/log.h> >> +#include <libcamera/formats.h> >> #include "libcamera/internal/dma_buf_allocator.h" >> +#include "libcamera/internal/formats.h" >> #include "libcamera/internal/mapped_framebuffer.h" >> #include <libyuv/convert_from_argb.h> >> @@ -42,7 +44,7 @@ LOG_DECLARE_CATEGORY(Virtual) >> static const unsigned int kARGBSize = 4; >> -int TestPatternGenerator::generateFrame(const Size &size, >> +int TestPatternGenerator::generateFrame(const StreamConfiguration *cfg, >> const FrameBuffer *buffer) >> { >> /* >> @@ -58,16 +60,48 @@ int TestPatternGenerator::generateFrame(const >> Size &size, >> MappedFrameBuffer::MapFlag::Write); >> const auto &planes = mappedFrameBuffer.planes(); >> + const auto size = cfg->size; >> rotateLeft1Column<kARGBSize>(size, template_.get()); >> /* Convert the template_ to the frame buffer */ >> - int ret = libyuv::ARGBToNV12(template_.get(), size.width * >> kARGBSize, >> - planes[0].data(), size.width, >> - planes[1].data(), size.width, >> - size.width, size.height); >> + int ret; >> + const PixelFormatInfo &info = >> PixelFormatInfo::info(cfg->pixelFormat); >> + switch (cfg->pixelFormat) { >> + case formats::NV12: >> + ret = libyuv::ARGBToNV12(template_.get(), >> + size.width * kARGBSize, >> + planes[0].data(), >> + info.stride(size.width, 0), >> + planes[1].data(), >> + info.stride(size.width, 1), >> + size.width, size.height); >> + break; >> + case formats::YUV420: >> + ret = libyuv::ARGBToI420(template_.get(), >> + size.width * kARGBSize, >> + planes[0].data(), >> + info.stride(size.width, 0), >> + planes[1].data(), >> + info.stride(size.width, 1), >> + planes[2].data(), >> + info.stride(size.width, 2), >> + size.width, size.height); >> + break; >> + case formats::ABGR8888: >> + case formats::XBGR8888: >> + ret = libyuv::ARGBCopy(template_.get(), >> + size.width * kARGBSize, >> + planes[0].data(), >> + info.stride(size.width, 0), >> + size.width, size.height); >> + break; >> + default: >> + LOG(Virtual, Error) << "Unsupported format"; >> + return -1; >> + } >> if (ret != 0) >> - LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret; >> + LOG(Virtual, Error) << "ARGB conversation failed with " << ret; > > Most of the previous comments apply to this function as well. > > >> return ret; >> } >> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h >> b/src/libcamera/pipeline/virtual/test_pattern_generator.h >> index 2a51bd31abdf..5022eaff3d30 100644 >> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h >> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h >> @@ -24,7 +24,7 @@ enum class TestPattern : char { >> class TestPatternGenerator : public FrameGenerator >> { >> public: >> - int generateFrame(const Size &size, const FrameBuffer *buffer) >> override; >> + int generateFrame(const StreamConfiguration *cfg, const >> FrameBuffer *buffer) override; >> protected: >> /* Buffer of test pattern template */ >> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp >> b/src/libcamera/pipeline/virtual/virtual.cpp >> index e25dd85a7391..565b66477363 100644 >> --- a/src/libcamera/pipeline/virtual/virtual.cpp >> +++ b/src/libcamera/pipeline/virtual/virtual.cpp >> @@ -148,7 +148,7 @@ void VirtualCameraData::processRequest(Request >> *request) >> found = true; >> if (streamConfig.frameGenerator->generateFrame( >> - stream->configuration().size, buffer)) >> + &stream->configuration(), buffer)) >> fmd.status = FrameMetadata::Status::FrameError; >> bufferCompleted.emit(buffer); >> @@ -202,16 +202,28 @@ CameraConfiguration::Status >> VirtualCameraConfiguration::validate() >> adjusted = true; >> } > > > Let's add > > constexpr PixelFormat outputFormats[] = { ... }; > > to the anon namespace in the file. And then it can be used here > and in `generateConfiguration()`. > > >> - if (cfg.pixelFormat != formats::NV12) { >> + if (cfg.pixelFormat != formats::NV12 && >> + cfg.pixelFormat != formats::YUV420 && >> + cfg.pixelFormat != formats::ABGR8888 && >> + cfg.pixelFormat != formats::XBGR8888) { > > > if (std::find(std::begin(outputFormats), std::end(outputFormats), > cfg.pixelFormat) == std::end(outputFormats)) > > or an equivalent you like. > > >> cfg.pixelFormat = formats::NV12; >> status = Adjusted; >> adjusted = true; >> } >> - if (cfg.colorSpace != ColorSpace::Smpte170m) { >> - cfg.colorSpace = ColorSpace::Smpte170m; >> - status = Adjusted; >> - adjusted = true; >> + const PixelFormatInfo &info = >> PixelFormatInfo::info(cfg.pixelFormat); >> + if (info.colourEncoding == >> PixelFormatInfo::ColourEncodingRGB) { >> + if (cfg.colorSpace != ColorSpace::Srgb) { >> + cfg.colorSpace = ColorSpace::Srgb; >> + status = Adjusted; >> + adjusted = true; >> + } >> + } else if (info.colourEncoding == >> PixelFormatInfo::ColourEncodingYUV) { >> + if (cfg.colorSpace != ColorSpace::Smpte170m) { >> + cfg.colorSpace = ColorSpace::Smpte170m; >> + status = Adjusted; >> + adjusted = true; >> + } >> } > > std::optional<ColorSpace> colorSpace; > > switch (info.colourEncoding) { > case ...: > colorSpace = ...; > default: > ASSERT(false); > } > > if (cfg.colorSpace != colorSpace) > ... > > >> if (validateColorSpaces() == Adjusted) { >> @@ -223,7 +235,6 @@ CameraConfiguration::Status >> VirtualCameraConfiguration::validate() >> LOG(Virtual, Info) >> << "Stream configuration adjusted to " << >> cfg.toString(); >> - const PixelFormatInfo &info = >> PixelFormatInfo::info(cfg.pixelFormat); >> cfg.stride = info.stride(cfg.size.width, 0, 1); >> cfg.frameSize = info.frameSize(cfg.size, 1); >> @@ -274,13 +285,19 @@ >> PipelineHandlerVirtual::generateConfiguration(Camera *camera, >> return {}; >> } >> + std::vector<PixelFormat> outputFormats = { formats::NV12, >> + formats::YUV420, >> + formats::ABGR8888, >> + formats::XBGR8888 }; >> + >> std::map<PixelFormat, std::vector<SizeRange>> streamFormats; >> - PixelFormat pixelFormat = formats::NV12; >> - streamFormats[pixelFormat] = { { >> data->config_.minResolutionSize, >> - data->config_.maxResolutionSize } }; >> + for (auto pixelFormat : outputFormats) { >> + streamFormats[pixelFormat] = { { >> data->config_.minResolutionSize, >> + data->config_.maxResolutionSize } }; >> + } >> StreamFormats formats(streamFormats); >> StreamConfiguration cfg(formats); >> - cfg.pixelFormat = pixelFormat; >> + cfg.pixelFormat = outputFormats.front(); >> cfg.size = data->config_.maxResolutionSize; >> cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; >> cfg.colorSpace = ColorSpace::Smpte170m; >
diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h index a0658c45cd82..70cd92907b63 100644 --- a/src/libcamera/pipeline/virtual/frame_generator.h +++ b/src/libcamera/pipeline/virtual/frame_generator.h @@ -9,6 +9,7 @@ #include <libcamera/framebuffer.h> #include <libcamera/geometry.h> +#include <libcamera/stream.h> namespace libcamera { @@ -19,7 +20,7 @@ public: virtual void configure(const Size &size) = 0; - virtual int generateFrame(const Size &size, + virtual int generateFrame(const StreamConfiguration *cfg, const FrameBuffer *buffer) = 0; protected: diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp index 1853bddeca27..dcef90702e18 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp @@ -14,7 +14,9 @@ #include <libcamera/framebuffer.h> +#include <libcamera/formats.h> #include "libcamera/internal/dma_buf_allocator.h" +#include "libcamera/internal/formats.h" #include "libcamera/internal/mapped_framebuffer.h" #include "libyuv/convert.h" @@ -124,8 +126,10 @@ void ImageFrameGenerator::configure(const Size &size) } } -int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer) +int ImageFrameGenerator::generateFrame(const StreamConfiguration *cfg, const FrameBuffer *buffer) { + auto size = cfg->size; + ASSERT(!scaledFrameDatas_.empty()); /* @@ -146,11 +150,49 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff frameIndex_ %= imageFrameDatas_.size(); /* Write the scaledY and scaledUV to the mapped frame buffer */ - libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), size.width, - scaledFrameDatas_[frameIndex_].UV.get(), size.width, - planes[0].data(), size.width, - planes[1].data(), size.width, - size.width, size.height); + int ret; + const PixelFormatInfo &info = PixelFormatInfo::info(cfg->pixelFormat); + switch (cfg->pixelFormat) { + case formats::NV12: + ret = libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), + size.width, + scaledFrameDatas_[frameIndex_].UV.get(), + size.width, + planes[0].data(), + info.stride(size.width, 0), + planes[1].data(), + info.stride(size.width, 1), + size.width, size.height); + break; + case formats::YUV420: + ret = libyuv::NV12ToI420(scaledFrameDatas_[frameIndex_].Y.get(), + size.width, + scaledFrameDatas_[frameIndex_].UV.get(), + size.width, + planes[0].data(), + info.stride(size.width, 0), + planes[1].data(), + info.stride(size.width, 1), + planes[2].data(), + info.stride(size.width, 2), + size.width, size.height); + break; + case formats::ABGR8888: + case formats::XBGR8888: + ret = libyuv::NV12ToARGB(scaledFrameDatas_[frameIndex_].Y.get(), + size.width, + scaledFrameDatas_[frameIndex_].UV.get(), + size.width, + planes[0].data(), + info.stride(size.width, 0), + size.width, size.height); + break; + default: + LOG(Virtual, Error) << "Unsupported format"; + return -1; + } + if (ret != 0) + LOG(Virtual, Error) << "NV12 conversation failed with " << ret; /* Proceed to the next image every 4 frames */ /* \todo Consider setting the frameRepeat in the config file */ @@ -158,7 +200,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff if (parameter_ % frameRepeat == 0) frameIndex_++; - return 0; + return ret; } /* diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h index 42a077ba803c..575900815d9e 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.h +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h @@ -37,7 +37,7 @@ private: }; void configure(const Size &size) override; - int generateFrame(const Size &size, const FrameBuffer *buffer) override; + int generateFrame(const StreamConfiguration *cfg, const FrameBuffer *buffer) override; std::vector<ImageFrameData> imageFrameDatas_; std::vector<ImageFrameData> scaledFrameDatas_; diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp index 7b53e8b32327..d8cdc1c91c24 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp @@ -11,7 +11,9 @@ #include <libcamera/base/log.h> +#include <libcamera/formats.h> #include "libcamera/internal/dma_buf_allocator.h" +#include "libcamera/internal/formats.h" #include "libcamera/internal/mapped_framebuffer.h" #include <libyuv/convert_from_argb.h> @@ -42,7 +44,7 @@ LOG_DECLARE_CATEGORY(Virtual) static const unsigned int kARGBSize = 4; -int TestPatternGenerator::generateFrame(const Size &size, +int TestPatternGenerator::generateFrame(const StreamConfiguration *cfg, const FrameBuffer *buffer) { /* @@ -58,16 +60,48 @@ int TestPatternGenerator::generateFrame(const Size &size, MappedFrameBuffer::MapFlag::Write); const auto &planes = mappedFrameBuffer.planes(); + const auto size = cfg->size; rotateLeft1Column<kARGBSize>(size, template_.get()); /* Convert the template_ to the frame buffer */ - int ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize, - planes[0].data(), size.width, - planes[1].data(), size.width, - size.width, size.height); + int ret; + const PixelFormatInfo &info = PixelFormatInfo::info(cfg->pixelFormat); + switch (cfg->pixelFormat) { + case formats::NV12: + ret = libyuv::ARGBToNV12(template_.get(), + size.width * kARGBSize, + planes[0].data(), + info.stride(size.width, 0), + planes[1].data(), + info.stride(size.width, 1), + size.width, size.height); + break; + case formats::YUV420: + ret = libyuv::ARGBToI420(template_.get(), + size.width * kARGBSize, + planes[0].data(), + info.stride(size.width, 0), + planes[1].data(), + info.stride(size.width, 1), + planes[2].data(), + info.stride(size.width, 2), + size.width, size.height); + break; + case formats::ABGR8888: + case formats::XBGR8888: + ret = libyuv::ARGBCopy(template_.get(), + size.width * kARGBSize, + planes[0].data(), + info.stride(size.width, 0), + size.width, size.height); + break; + default: + LOG(Virtual, Error) << "Unsupported format"; + return -1; + } if (ret != 0) - LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret; + LOG(Virtual, Error) << "ARGB conversation failed with " << ret; return ret; } diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h index 2a51bd31abdf..5022eaff3d30 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h @@ -24,7 +24,7 @@ enum class TestPattern : char { class TestPatternGenerator : public FrameGenerator { public: - int generateFrame(const Size &size, const FrameBuffer *buffer) override; + int generateFrame(const StreamConfiguration *cfg, const FrameBuffer *buffer) override; protected: /* Buffer of test pattern template */ diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp index e25dd85a7391..565b66477363 100644 --- a/src/libcamera/pipeline/virtual/virtual.cpp +++ b/src/libcamera/pipeline/virtual/virtual.cpp @@ -148,7 +148,7 @@ void VirtualCameraData::processRequest(Request *request) found = true; if (streamConfig.frameGenerator->generateFrame( - stream->configuration().size, buffer)) + &stream->configuration(), buffer)) fmd.status = FrameMetadata::Status::FrameError; bufferCompleted.emit(buffer); @@ -202,16 +202,28 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() adjusted = true; } - if (cfg.pixelFormat != formats::NV12) { + if (cfg.pixelFormat != formats::NV12 && + cfg.pixelFormat != formats::YUV420 && + cfg.pixelFormat != formats::ABGR8888 && + cfg.pixelFormat != formats::XBGR8888) { cfg.pixelFormat = formats::NV12; status = Adjusted; adjusted = true; } - if (cfg.colorSpace != ColorSpace::Smpte170m) { - cfg.colorSpace = ColorSpace::Smpte170m; - status = Adjusted; - adjusted = true; + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRGB) { + if (cfg.colorSpace != ColorSpace::Srgb) { + cfg.colorSpace = ColorSpace::Srgb; + status = Adjusted; + adjusted = true; + } + } else if (info.colourEncoding == PixelFormatInfo::ColourEncodingYUV) { + if (cfg.colorSpace != ColorSpace::Smpte170m) { + cfg.colorSpace = ColorSpace::Smpte170m; + status = Adjusted; + adjusted = true; + } } if (validateColorSpaces() == Adjusted) { @@ -223,7 +235,6 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() LOG(Virtual, Info) << "Stream configuration adjusted to " << cfg.toString(); - const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); cfg.stride = info.stride(cfg.size.width, 0, 1); cfg.frameSize = info.frameSize(cfg.size, 1); @@ -274,13 +285,19 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, return {}; } + std::vector<PixelFormat> outputFormats = { formats::NV12, + formats::YUV420, + formats::ABGR8888, + formats::XBGR8888 }; + std::map<PixelFormat, std::vector<SizeRange>> streamFormats; - PixelFormat pixelFormat = formats::NV12; - streamFormats[pixelFormat] = { { data->config_.minResolutionSize, - data->config_.maxResolutionSize } }; + for (auto pixelFormat : outputFormats) { + streamFormats[pixelFormat] = { { data->config_.minResolutionSize, + data->config_.maxResolutionSize } }; + } StreamFormats formats(streamFormats); StreamConfiguration cfg(formats); - cfg.pixelFormat = pixelFormat; + cfg.pixelFormat = outputFormats.front(); cfg.size = data->config_.maxResolutionSize; cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; cfg.colorSpace = ColorSpace::Smpte170m;
While NV12 is the most common native format for hardware video encoders, YUV420 is the equivalent for most software video encoders. Thus supporting both is great for testing. Similarly having some RGB formats with and without alpha can be very useful, especially as the swISP is currently limited to such formats. This is a polished version of a hack that was used to help debugging and testing https://webrtc-review.googlesource.com/c/src/+/491680 - and so helpful with that that I'd like to see it upstream. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- .../pipeline/virtual/frame_generator.h | 3 +- .../virtual/image_frame_generator.cpp | 56 ++++++++++++++++--- .../pipeline/virtual/image_frame_generator.h | 2 +- .../virtual/test_pattern_generator.cpp | 46 +++++++++++++-- .../pipeline/virtual/test_pattern_generator.h | 2 +- src/libcamera/pipeline/virtual/virtual.cpp | 39 +++++++++---- 6 files changed, 121 insertions(+), 27 deletions(-)