| Message ID | 20261009194007.23175-5-robert.mader@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Robert Mader (2026-10-09 20:40:07) > This patch was originally developed to help debugging and testing > https://webrtc-review.googlesource.com/c/src/+/491680, however I see > some benefits in always doing it: > 1. It is common for hardware ISPs to have some stride alignment - even > though usually somewhat lower. Using the given alignment should thus > help ensure that clients implement stride handling correctly, > preventing cases like the one linked above where a major implemention > went without that - unnocited - for quite a long time. > 2. It makes it more likely that output buffers can be imported by GPUs > and display engines. > 2. It paves the way for GPU-accelerated frame generators (similar to > DebayerEGL), which may become desirable going forward in order to > provide high resolution / high framerate / high bit-depth / HDR > content. I'm looking forward to the second 2! > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > .../pipeline/virtual/frame_generator.h | 1 + > .../virtual/image_frame_generator.cpp | 23 ++++++++++--------- > .../virtual/test_pattern_generator.cpp | 18 ++++++++++----- > src/libcamera/pipeline/virtual/virtual.cpp | 9 +++++--- > 4 files changed, 31 insertions(+), 20 deletions(-) > > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h > index fd34a91a5e50..d575c28a3ce0 100644 > --- a/src/libcamera/pipeline/virtual/frame_generator.h > +++ b/src/libcamera/pipeline/virtual/frame_generator.h > @@ -23,6 +23,7 @@ public: > virtual int generateFrame(const StreamConfiguration &cfg, > const FrameBuffer *buffer) = 0; > > + static constexpr unsigned int kStrideAlignment = 256; I guess this would make anything the application asks for somewhat redundant, unless they ask for 512 ;-) > protected: > FrameGenerator() {} > }; > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > index 5c56db96d8e2..be5633050a11 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -110,11 +110,6 @@ void ImageFrameGenerator::configure(const Size &size) > std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2); > auto &src = imageFrameDatas_[i]; > > - /* > - * \todo Some platforms might enforce stride due to GPU. > - * The width needs to be a multiple of the stride to work > - * properly for now. > - */ > libyuv::NV12Scale(src.Y.get(), src.size.width, > src.UV.get(), src.size.width, > src.size.width, src.size.height, > @@ -158,9 +153,11 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const Fra > scaledFrameDatas_[frameIndex_].UV.get(), > size.width, > planes[0].data(), > - info.stride(size.width, 0), > + info.stride(size.width, 0, > + kStrideAlignment), > planes[1].data(), > - info.stride(size.width, 1), > + info.stride(size.width, 1, > + kStrideAlignment), Should/could the stride be set from the StreamConfiguration? Once validated - I guess in the current state it would be the same as kStrideAlignment, but if we ever make it configurable - then it should match the StreamConfiguration ? But if there's a valid reason to keep a constant here then so be it, so either way: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> -- Kieran > size.width, size.height); > break; > case formats::YUV420: > @@ -169,11 +166,14 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const Fra > scaledFrameDatas_[frameIndex_].UV.get(), > size.width, > planes[0].data(), > - info.stride(size.width, 0), > + info.stride(size.width, 0, > + kStrideAlignment), > planes[1].data(), > - info.stride(size.width, 1), > + info.stride(size.width, 1, > + kStrideAlignment), > planes[2].data(), > - info.stride(size.width, 2), > + info.stride(size.width, 2, > + kStrideAlignment), > size.width, size.height); > break; > case formats::ABGR8888: > @@ -183,7 +183,8 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const Fra > scaledFrameDatas_[frameIndex_].UV.get(), > size.width, > planes[0].data(), > - info.stride(size.width, 0), > + info.stride(size.width, 0, > + kStrideAlignment), > size.width, size.height); > break; > default: > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > index 4cde32176661..9ed5f6e74f84 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > @@ -72,20 +72,25 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg, > ret = libyuv::ARGBToNV12(template_.get(), > size.width * kARGBSize, > planes[0].data(), > - info.stride(size.width, 0), > + info.stride(size.width, 0, > + kStrideAlignment), > planes[1].data(), > - info.stride(size.width, 1), > + info.stride(size.width, 1, > + kStrideAlignment), > size.width, size.height); > break; > case formats::YUV420: > ret = libyuv::ARGBToI420(template_.get(), > size.width * kARGBSize, > planes[0].data(), > - info.stride(size.width, 0), > + info.stride(size.width, 0, > + kStrideAlignment), > planes[1].data(), > - info.stride(size.width, 1), > + info.stride(size.width, 1, > + kStrideAlignment), > planes[2].data(), > - info.stride(size.width, 2), > + info.stride(size.width, 2, > + kStrideAlignment), > size.width, size.height); > break; > case formats::ABGR8888: > @@ -93,7 +98,8 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg, > ret = libyuv::ARGBCopy(template_.get(), > size.width * kARGBSize, > planes[0].data(), > - info.stride(size.width, 0), > + info.stride(size.width, 0, > + kStrideAlignment), > size.width, size.height); > break; > default: > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 6e97981b0f40..a9c1a5a85fc1 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -240,8 +240,10 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > adjusted = true; > } > > - auto stride = info.stride(cfg.size.width, 0); > - auto frameSize = info.frameSize(cfg.size); > + auto stride = info.stride(cfg.size.width, 0, > + FrameGenerator::kStrideAlignment); > + auto frameSize = info.frameSize(cfg.size, > + FrameGenerator::kStrideAlignment); > if ((cfg.stride && cfg.stride != stride) || > (cfg.frameSize && cfg.frameSize != frameSize)) { > status = Adjusted; > @@ -346,7 +348,8 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera, > > std::vector<unsigned int> planeSizes; > for (size_t i = 0; i < info.numPlanes(); ++i) > - planeSizes.push_back(info.planeSize(config.size, i)); > + planeSizes.push_back(info.planeSize(config.size, i, > + FrameGenerator::kStrideAlignment)); > > return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers); > } > -- > 2.56.0 >
Hi 2026. 10. 09. 21:40 keltezéssel, Robert Mader írta: > This patch was originally developed to help debugging and testing > https://webrtc-review.googlesource.com/c/src/+/491680, however I see > some benefits in always doing it: > 1. It is common for hardware ISPs to have some stride alignment - even > though usually somewhat lower. Using the given alignment should thus > help ensure that clients implement stride handling correctly, > preventing cases like the one linked above where a major implemention implementation > went without that - unnocited - for quite a long time. unnoticed > 2. It makes it more likely that output buffers can be imported by GPUs > and display engines. > 2. It paves the way for GPU-accelerated frame generators (similar to > DebayerEGL), which may become desirable going forward in order to > provide high resolution / high framerate / high bit-depth / HDR > content. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- Does this not need a resolution to https://gitlab.freedesktop.org/camera/libcamera/-/work_items/360 ? > .../pipeline/virtual/frame_generator.h | 1 + > .../virtual/image_frame_generator.cpp | 23 ++++++++++--------- > .../virtual/test_pattern_generator.cpp | 18 ++++++++++----- > src/libcamera/pipeline/virtual/virtual.cpp | 9 +++++--- > 4 files changed, 31 insertions(+), 20 deletions(-) > > [...]
diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h index fd34a91a5e50..d575c28a3ce0 100644 --- a/src/libcamera/pipeline/virtual/frame_generator.h +++ b/src/libcamera/pipeline/virtual/frame_generator.h @@ -23,6 +23,7 @@ public: virtual int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) = 0; + static constexpr unsigned int kStrideAlignment = 256; protected: FrameGenerator() {} }; diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp index 5c56db96d8e2..be5633050a11 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp @@ -110,11 +110,6 @@ void ImageFrameGenerator::configure(const Size &size) std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2); auto &src = imageFrameDatas_[i]; - /* - * \todo Some platforms might enforce stride due to GPU. - * The width needs to be a multiple of the stride to work - * properly for now. - */ libyuv::NV12Scale(src.Y.get(), src.size.width, src.UV.get(), src.size.width, src.size.width, src.size.height, @@ -158,9 +153,11 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const Fra scaledFrameDatas_[frameIndex_].UV.get(), size.width, planes[0].data(), - info.stride(size.width, 0), + info.stride(size.width, 0, + kStrideAlignment), planes[1].data(), - info.stride(size.width, 1), + info.stride(size.width, 1, + kStrideAlignment), size.width, size.height); break; case formats::YUV420: @@ -169,11 +166,14 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const Fra scaledFrameDatas_[frameIndex_].UV.get(), size.width, planes[0].data(), - info.stride(size.width, 0), + info.stride(size.width, 0, + kStrideAlignment), planes[1].data(), - info.stride(size.width, 1), + info.stride(size.width, 1, + kStrideAlignment), planes[2].data(), - info.stride(size.width, 2), + info.stride(size.width, 2, + kStrideAlignment), size.width, size.height); break; case formats::ABGR8888: @@ -183,7 +183,8 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const Fra scaledFrameDatas_[frameIndex_].UV.get(), size.width, planes[0].data(), - info.stride(size.width, 0), + info.stride(size.width, 0, + kStrideAlignment), size.width, size.height); break; default: diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp index 4cde32176661..9ed5f6e74f84 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp @@ -72,20 +72,25 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg, ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize, planes[0].data(), - info.stride(size.width, 0), + info.stride(size.width, 0, + kStrideAlignment), planes[1].data(), - info.stride(size.width, 1), + info.stride(size.width, 1, + kStrideAlignment), size.width, size.height); break; case formats::YUV420: ret = libyuv::ARGBToI420(template_.get(), size.width * kARGBSize, planes[0].data(), - info.stride(size.width, 0), + info.stride(size.width, 0, + kStrideAlignment), planes[1].data(), - info.stride(size.width, 1), + info.stride(size.width, 1, + kStrideAlignment), planes[2].data(), - info.stride(size.width, 2), + info.stride(size.width, 2, + kStrideAlignment), size.width, size.height); break; case formats::ABGR8888: @@ -93,7 +98,8 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg, ret = libyuv::ARGBCopy(template_.get(), size.width * kARGBSize, planes[0].data(), - info.stride(size.width, 0), + info.stride(size.width, 0, + kStrideAlignment), size.width, size.height); break; default: diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp index 6e97981b0f40..a9c1a5a85fc1 100644 --- a/src/libcamera/pipeline/virtual/virtual.cpp +++ b/src/libcamera/pipeline/virtual/virtual.cpp @@ -240,8 +240,10 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() adjusted = true; } - auto stride = info.stride(cfg.size.width, 0); - auto frameSize = info.frameSize(cfg.size); + auto stride = info.stride(cfg.size.width, 0, + FrameGenerator::kStrideAlignment); + auto frameSize = info.frameSize(cfg.size, + FrameGenerator::kStrideAlignment); if ((cfg.stride && cfg.stride != stride) || (cfg.frameSize && cfg.frameSize != frameSize)) { status = Adjusted; @@ -346,7 +348,8 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera, std::vector<unsigned int> planeSizes; for (size_t i = 0; i < info.numPlanes(); ++i) - planeSizes.push_back(info.planeSize(config.size, i)); + planeSizes.push_back(info.planeSize(config.size, i, + FrameGenerator::kStrideAlignment)); return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers); }
This patch was originally developed to help debugging and testing https://webrtc-review.googlesource.com/c/src/+/491680, however I see some benefits in always doing it: 1. It is common for hardware ISPs to have some stride alignment - even though usually somewhat lower. Using the given alignment should thus help ensure that clients implement stride handling correctly, preventing cases like the one linked above where a major implemention went without that - unnocited - for quite a long time. 2. It makes it more likely that output buffers can be imported by GPUs and display engines. 2. It paves the way for GPU-accelerated frame generators (similar to DebayerEGL), which may become desirable going forward in order to provide high resolution / high framerate / high bit-depth / HDR content. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- .../pipeline/virtual/frame_generator.h | 1 + .../virtual/image_frame_generator.cpp | 23 ++++++++++--------- .../virtual/test_pattern_generator.cpp | 18 ++++++++++----- src/libcamera/pipeline/virtual/virtual.cpp | 9 +++++--- 4 files changed, 31 insertions(+), 20 deletions(-)