| Message ID | 20250831165243.36652-1-robert.mader@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Robert, thank you for the patch. Robert Mader <robert.mader@collabora.com> writes: > When using the softwareISP we allocate the output buffers ourselves, How does it differ from the other cases, namely for a converter and for raw streams? Should it be explained in a code comment in the code below? > usually from system memory. There's thus no strong reason to limit > choices for client. Situations where this might be useful include: > 1. Video encoding, e.g. encoding multiple buffers in parallel. > 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. > > Thus allow up to 16 buffers - arbitrarily decided - while keeping the > default to the previous 4. > > While on it, mark the config as adjusted if we did so. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 38 ++++++++++++++++++++++-- > 1 file changed, 36 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 9b24a0db9..da871c7fb 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -375,6 +375,9 @@ public: > const Transform &combinedTransform() const { return combinedTransform_; } > > private: > + static constexpr unsigned int kNumBuffersDefault = 4; > + static constexpr unsigned int kNumBuffersSwISPMax = 16; s/kNumBuffersSwISPMax/kNumBuffersSwIspMax/ to make it consistent with similar identifiers. > + > /* > * The SimpleCameraData instance is guaranteed to be valid as long as > * the corresponding Camera instance is valid. In order to borrow a > @@ -1246,6 +1249,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > cfg.size); > if (cfg.stride == 0) > return Invalid; > + > + if (data_->converter_) { > + if (cfg.bufferCount != kNumBuffersDefault) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersDefault; > + cfg.bufferCount = kNumBuffersDefault; > + status = Adjusted; > + } > + } else { > + if (cfg.bufferCount == 0) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersDefault; > + cfg.bufferCount = kNumBuffersDefault; > + status = Adjusted; > + } > + if (cfg.bufferCount > kNumBuffersSwISPMax) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersSwISPMax; > + cfg.bufferCount = kNumBuffersSwISPMax; > + status = Adjusted; > + } > + } > } else { > V4L2DeviceFormat format; > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > @@ -1257,9 +1285,15 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > cfg.stride = format.planes[0].bpl; > cfg.frameSize = format.planes[0].size; > - } > > - cfg.bufferCount = 4; > + if (cfg.bufferCount != kNumBuffersDefault) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersDefault; > + cfg.bufferCount = kNumBuffersDefault; > + status = Adjusted; > + } > + } > } > > return status;
On 01.09.25 11:35, Milan Zamazal wrote: > Hi Robert, > > thank you for the patch. > > Robert Mader<robert.mader@collabora.com> writes: > >> When using the softwareISP we allocate the output buffers ourselves, > How does it differ from the other cases, namely for a converter and for > raw streams? Should it be explained in a code comment in the code > below? In the other cases we allocate buffers via V4L2, while in the swISP case we use our own DmaBufAllocator (only allocating 3 "internal" buffers via V4L2), thus shouldn't have any hardware limitation apart from system memory. FTR., it might well make sense to allow more buffers in the other cases as well - I'm just not sure if some V4L2 drivers might have limitations. If we can assume that every well written driver handles 1-16 buffers - then I'd say lets support it. >> usually from system memory. There's thus no strong reason to limit >> choices for client. Situations where this might be useful include: >> 1. Video encoding, e.g. encoding multiple buffers in parallel. >> 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. >> >> Thus allow up to 16 buffers - arbitrarily decided - while keeping the >> default to the previous 4. >> >> While on it, mark the config as adjusted if we did so. >> >> Signed-off-by: Robert Mader<robert.mader@collabora.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 38 ++++++++++++++++++++++-- >> 1 file changed, 36 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 9b24a0db9..da871c7fb 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -375,6 +375,9 @@ public: >> const Transform &combinedTransform() const { return combinedTransform_; } >> >> private: >> + static constexpr unsigned int kNumBuffersDefault = 4; >> + static constexpr unsigned int kNumBuffersSwISPMax = 16; > s/kNumBuffersSwISPMax/kNumBuffersSwIspMax/ to make it consistent with > similar identifiers. Thanks, will change in v2! >> + >> /* >> * The SimpleCameraData instance is guaranteed to be valid as long as >> * the corresponding Camera instance is valid. In order to borrow a >> @@ -1246,6 +1249,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> cfg.size); >> if (cfg.stride == 0) >> return Invalid; >> + >> + if (data_->converter_) { >> + if (cfg.bufferCount != kNumBuffersDefault) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersDefault; >> + cfg.bufferCount = kNumBuffersDefault; >> + status = Adjusted; >> + } >> + } else { >> + if (cfg.bufferCount == 0) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersDefault; >> + cfg.bufferCount = kNumBuffersDefault; >> + status = Adjusted; >> + } >> + if (cfg.bufferCount > kNumBuffersSwISPMax) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersSwISPMax; >> + cfg.bufferCount = kNumBuffersSwISPMax; >> + status = Adjusted; >> + } >> + } >> } else { >> V4L2DeviceFormat format; >> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); >> @@ -1257,9 +1285,15 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> cfg.stride = format.planes[0].bpl; >> cfg.frameSize = format.planes[0].size; >> - } >> >> - cfg.bufferCount = 4; >> + if (cfg.bufferCount != kNumBuffersDefault) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersDefault; >> + cfg.bufferCount = kNumBuffersDefault; >> + status = Adjusted; >> + } >> + } >> } >> >> return status;
Hi Robert, On Sun, Aug 31, 2025 at 06:52:43PM +0200, Robert Mader wrote: > When using the softwareISP we allocate the output buffers ourselves, > usually from system memory. There's thus no strong reason to limit > choices for client. Situations where this might be useful include: > 1. Video encoding, e.g. encoding multiple buffers in parallel. > 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. These use cases should be able to pass in bufferCount according to their needs, no ? > > Thus allow up to 16 buffers - arbitrarily decided - while keeping the > default to the previous 4. > > While on it, mark the config as adjusted if we did so. Why not properly support the cfg.bufferCount instead? For e.g. there was a recent patch merged for the rkisp1 pipeline handler. commit 521177161a76949e85a76ddebe0694db7545ae4f Author: Stefan Klug <stefan.klug@ideasonboard.com> Date: Thu Jul 17 14:59:24 2025 +0200 pipeline: rkisp1: Properly handle the bufferCount set in the stream configuration The StreamConfiguration::bufferCount is reset to a hardcoded value of 4 in RkISP1Path::validate(). Keep the minimum value of 4 but do not reset it, if it was set to a larger value. This allows the user to set bufferCount to an arbitrary number of buffers which then can be allocated for example by the FrameBufferAllocator. If the bufferCount is set to a smaller value it gets reset to kRkISP1MinBufferCount again and the configuration is marked as adjusted. Do you think the simple pipeline handler will be able to support cfg.bufferCount or there will be some blockers ? > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 38 ++++++++++++++++++++++-- > 1 file changed, 36 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 9b24a0db9..da871c7fb 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -375,6 +375,9 @@ public: > const Transform &combinedTransform() const { return combinedTransform_; } > > private: > + static constexpr unsigned int kNumBuffersDefault = 4; > + static constexpr unsigned int kNumBuffersSwISPMax = 16; > + > /* > * The SimpleCameraData instance is guaranteed to be valid as long as > * the corresponding Camera instance is valid. In order to borrow a > @@ -1246,6 +1249,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > cfg.size); > if (cfg.stride == 0) > return Invalid; > + > + if (data_->converter_) { > + if (cfg.bufferCount != kNumBuffersDefault) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersDefault; > + cfg.bufferCount = kNumBuffersDefault; > + status = Adjusted; > + } > + } else { > + if (cfg.bufferCount == 0) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersDefault; > + cfg.bufferCount = kNumBuffersDefault; > + status = Adjusted; > + } > + if (cfg.bufferCount > kNumBuffersSwISPMax) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersSwISPMax; > + cfg.bufferCount = kNumBuffersSwISPMax; > + status = Adjusted; > + } > + } > } else { > V4L2DeviceFormat format; > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > @@ -1257,9 +1285,15 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > cfg.stride = format.planes[0].bpl; > cfg.frameSize = format.planes[0].size; > - } > > - cfg.bufferCount = 4; > + if (cfg.bufferCount != kNumBuffersDefault) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersDefault; > + cfg.bufferCount = kNumBuffersDefault; > + status = Adjusted; > + } > + } > } > > return status; > -- > 2.51.0 >
On 01.09.25 14:11, Umang Jain wrote: > Hi Robert, > > On Sun, Aug 31, 2025 at 06:52:43PM +0200, Robert Mader wrote: >> When using the softwareISP we allocate the output buffers ourselves, >> usually from system memory. There's thus no strong reason to limit >> choices for client. Situations where this might be useful include: >> 1. Video encoding, e.g. encoding multiple buffers in parallel. >> 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. > These use cases should be able to pass in bufferCount according to their > needs, no ? Err, exactly. That's what I'm trying to allow here - just limited to the swISP because I'm not sure about potential hardware limitations in other use-cases. >> Thus allow up to 16 buffers - arbitrarily decided - while keeping the >> default to the previous 4. >> >> While on it, mark the config as adjusted if we did so. > Why not properly support the cfg.bufferCount instead? > For e.g. there was a recent patch merged for the rkisp1 pipeline > handler. What do you mean with "properly support the cfg.bufferCount"? No upper limit? > commit 521177161a76949e85a76ddebe0694db7545ae4f > Author: Stefan Klug<stefan.klug@ideasonboard.com> > Date: Thu Jul 17 14:59:24 2025 +0200 > > pipeline: rkisp1: Properly handle the bufferCount set in the stream configuration > > The StreamConfiguration::bufferCount is reset to a hardcoded value of 4 > in RkISP1Path::validate(). Keep the minimum value of 4 but do not reset > it, if it was set to a larger value. This allows the user to set > bufferCount to an arbitrary number of buffers which then can be > allocated for example by the FrameBufferAllocator. If the bufferCount is > set to a smaller value it gets reset to kRkISP1MinBufferCount again and > the configuration is marked as adjusted. > > Do you think the simple pipeline handler will be able to support > cfg.bufferCount or there will be some blockers ? Yep, that patch motivated the change here. Previously the simple pipeline behaved like rkisp1 did before the quoted patch, i.e. hardcoding cfg.bufferCount to 4 (and not marking the config as adjusted). With this patch users get what they requested in cfg.bufferCount - if the swISP is used and with an arbitrary upper limit of 16. >> Signed-off-by: Robert Mader<robert.mader@collabora.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 38 ++++++++++++++++++++++-- >> 1 file changed, 36 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 9b24a0db9..da871c7fb 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -375,6 +375,9 @@ public: >> const Transform &combinedTransform() const { return combinedTransform_; } >> >> private: >> + static constexpr unsigned int kNumBuffersDefault = 4; >> + static constexpr unsigned int kNumBuffersSwISPMax = 16; >> + >> /* >> * The SimpleCameraData instance is guaranteed to be valid as long as >> * the corresponding Camera instance is valid. In order to borrow a >> @@ -1246,6 +1249,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> cfg.size); >> if (cfg.stride == 0) >> return Invalid; >> + >> + if (data_->converter_) { >> + if (cfg.bufferCount != kNumBuffersDefault) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersDefault; >> + cfg.bufferCount = kNumBuffersDefault; >> + status = Adjusted; >> + } >> + } else { >> + if (cfg.bufferCount == 0) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersDefault; >> + cfg.bufferCount = kNumBuffersDefault; >> + status = Adjusted; >> + } >> + if (cfg.bufferCount > kNumBuffersSwISPMax) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersSwISPMax; >> + cfg.bufferCount = kNumBuffersSwISPMax; >> + status = Adjusted; >> + } >> + } >> } else { >> V4L2DeviceFormat format; >> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); >> @@ -1257,9 +1285,15 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> cfg.stride = format.planes[0].bpl; >> cfg.frameSize = format.planes[0].size; >> - } >> >> - cfg.bufferCount = 4; >> + if (cfg.bufferCount != kNumBuffersDefault) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersDefault; >> + cfg.bufferCount = kNumBuffersDefault; >> + status = Adjusted; >> + } >> + } >> } >> >> return status; >> -- >> 2.51.0 >>
On Mon, Sep 01, 2025 at 02:24:03PM +0200, Robert Mader wrote: > On 01.09.25 14:11, Umang Jain wrote: > > Hi Robert, > > > > On Sun, Aug 31, 2025 at 06:52:43PM +0200, Robert Mader wrote: > > > When using the softwareISP we allocate the output buffers ourselves, > > > usually from system memory. There's thus no strong reason to limit > > > choices for client. Situations where this might be useful include: > > > 1. Video encoding, e.g. encoding multiple buffers in parallel. > > > 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. > > These use cases should be able to pass in bufferCount according to their > > needs, no ? > Err, exactly. That's what I'm trying to allow here - just limited to the > swISP because I'm not sure about potential hardware limitations in other > use-cases. > > > Thus allow up to 16 buffers - arbitrarily decided - while keeping the > > > default to the previous 4. > > > > > > While on it, mark the config as adjusted if we did so. > > Why not properly support the cfg.bufferCount instead? > > For e.g. there was a recent patch merged for the rkisp1 pipeline > > handler. > > What do you mean with "properly support the cfg.bufferCount"? No upper > limit? > > > commit 521177161a76949e85a76ddebe0694db7545ae4f > > Author: Stefan Klug<stefan.klug@ideasonboard.com> > > Date: Thu Jul 17 14:59:24 2025 +0200 > > > > pipeline: rkisp1: Properly handle the bufferCount set in the stream configuration > > > > The StreamConfiguration::bufferCount is reset to a hardcoded value of 4 > > in RkISP1Path::validate(). Keep the minimum value of 4 but do not reset > > it, if it was set to a larger value. This allows the user to set > > bufferCount to an arbitrary number of buffers which then can be > > allocated for example by the FrameBufferAllocator. If the bufferCount is > > set to a smaller value it gets reset to kRkISP1MinBufferCount again and > > the configuration is marked as adjusted. > > > > Do you think the simple pipeline handler will be able to support > > cfg.bufferCount or there will be some blockers ? > > Yep, that patch motivated the change here. Previously the simple pipeline > behaved like rkisp1 did before the quoted patch, i.e. hardcoding > cfg.bufferCount to 4 (and not marking the config as adjusted). With this > patch users get what they requested in cfg.bufferCount - if the swISP is > used and with an arbitrary upper limit of 16. Ah ok, so the logic is aligned. > > > > Signed-off-by: Robert Mader<robert.mader@collabora.com> > > > --- > > > src/libcamera/pipeline/simple/simple.cpp | 38 ++++++++++++++++++++++-- > > > 1 file changed, 36 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > > index 9b24a0db9..da871c7fb 100644 > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > @@ -375,6 +375,9 @@ public: > > > const Transform &combinedTransform() const { return combinedTransform_; } > > > private: > > > + static constexpr unsigned int kNumBuffersDefault = 4; > > > + static constexpr unsigned int kNumBuffersSwISPMax = 16; > > > + > > > /* > > > * The SimpleCameraData instance is guaranteed to be valid as long as > > > * the corresponding Camera instance is valid. In order to borrow a > > > @@ -1246,6 +1249,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > cfg.size); > > > if (cfg.stride == 0) > > > return Invalid; > > > + > > > + if (data_->converter_) { > > > + if (cfg.bufferCount != kNumBuffersDefault) { > > > + LOG(SimplePipeline, Debug) > > > + << "Adjusting bufferCount from " << cfg.bufferCount > > > + << " to " << kNumBuffersDefault; > > > + cfg.bufferCount = kNumBuffersDefault; > > > + status = Adjusted; > > > + } > > > + } else { > > > + if (cfg.bufferCount == 0) { > > > + LOG(SimplePipeline, Debug) > > > + << "Adjusting bufferCount from " << cfg.bufferCount > > > + << " to " << kNumBuffersDefault; > > > + cfg.bufferCount = kNumBuffersDefault; > > > + status = Adjusted; > > > + } > > > + if (cfg.bufferCount > kNumBuffersSwISPMax) { > > > + LOG(SimplePipeline, Debug) > > > + << "Adjusting bufferCount from " << cfg.bufferCount > > > + << " to " << kNumBuffersSwISPMax; > > > + cfg.bufferCount = kNumBuffersSwISPMax; > > > + status = Adjusted; > > > + } > > > + } Until here, this makes sense to me. > > > } else { > > > V4L2DeviceFormat format; > > > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > > > @@ -1257,9 +1285,15 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > cfg.stride = format.planes[0].bpl; > > > cfg.frameSize = format.planes[0].size; > > > - } > > > - cfg.bufferCount = 4; > > > + if (cfg.bufferCount != kNumBuffersDefault) { > > > + LOG(SimplePipeline, Debug) > > > + << "Adjusting bufferCount from " << cfg.bufferCount > > > + << " to " << kNumBuffersDefault; > > > + cfg.bufferCount = kNumBuffersDefault; > > > + status = Adjusted; > > > + } > > > + } However, I don't understand this. I see that cfg.bufferCount here is either 1) kNumBuffersDefault or 2) a value <= kNumBuffersSwISPMax. Won't it get reset in case of 2) ? > > > } > > > return status; > > > -- > > > 2.51.0 > > > > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718
On Mon, Sep 01, 2025 at 12:22:37PM +0200, Robert Mader wrote: > On 01.09.25 11:35, Milan Zamazal wrote: > > Hi Robert, > > > > thank you for the patch. > > > > Robert Mader<robert.mader@collabora.com> writes: > > > >> When using the softwareISP we allocate the output buffers ourselves, > > How does it differ from the other cases, namely for a converter and for > > raw streams? Should it be explained in a code comment in the code > > below? > > In the other cases we allocate buffers via V4L2, while in the swISP case > we use our own DmaBufAllocator (only allocating 3 "internal" buffers via > V4L2), thus shouldn't have any hardware limitation apart from system > memory. FTR., it might well make sense to allow more buffers in the > other cases as well - I'm just not sure if some V4L2 drivers might have > limitations. If we can assume that every well written driver handles > 1-16 buffers - then I'd say lets support it. For all practical purposes, the limit should be the same for V4L2-allocated buffers: the only limiting factor is system memory. Drivers are free to set an arbitrary limit on the number of buffers, but in practice only 3 drivers set such a limit (hantro, vicodec and vivid), and all of them set the limit to 64. The limit is otherwise set by the vb2 core to 32 buffers. > >> usually from system memory. There's thus no strong reason to limit > >> choices for client. Situations where this might be useful include: > >> 1. Video encoding, e.g. encoding multiple buffers in parallel. > >> 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. > >> > >> Thus allow up to 16 buffers - arbitrarily decided - while keeping the > >> default to the previous 4. > >> > >> While on it, mark the config as adjusted if we did so. > >> > >> Signed-off-by: Robert Mader<robert.mader@collabora.com> > >> --- > >> src/libcamera/pipeline/simple/simple.cpp | 38 ++++++++++++++++++++++-- > >> 1 file changed, 36 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index 9b24a0db9..da871c7fb 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -375,6 +375,9 @@ public: > >> const Transform &combinedTransform() const { return combinedTransform_; } > >> > >> private: > >> + static constexpr unsigned int kNumBuffersDefault = 4; > >> + static constexpr unsigned int kNumBuffersSwISPMax = 16; > > s/kNumBuffersSwISPMax/kNumBuffersSwIspMax/ to make it consistent with > > similar identifiers. > > Thanks, will change in v2! > > >> + > >> /* > >> * The SimpleCameraData instance is guaranteed to be valid as long as > >> * the corresponding Camera instance is valid. In order to borrow a > >> @@ -1246,6 +1249,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > >> cfg.size); > >> if (cfg.stride == 0) > >> return Invalid; > >> + > >> + if (data_->converter_) { > >> + if (cfg.bufferCount != kNumBuffersDefault) { > >> + LOG(SimplePipeline, Debug) > >> + << "Adjusting bufferCount from " << cfg.bufferCount > >> + << " to " << kNumBuffersDefault; > >> + cfg.bufferCount = kNumBuffersDefault; > >> + status = Adjusted; > >> + } > >> + } else { > >> + if (cfg.bufferCount == 0) { > >> + LOG(SimplePipeline, Debug) > >> + << "Adjusting bufferCount from " << cfg.bufferCount > >> + << " to " << kNumBuffersDefault; > >> + cfg.bufferCount = kNumBuffersDefault; > >> + status = Adjusted; > >> + } > >> + if (cfg.bufferCount > kNumBuffersSwISPMax) { > >> + LOG(SimplePipeline, Debug) > >> + << "Adjusting bufferCount from " << cfg.bufferCount > >> + << " to " << kNumBuffersSwISPMax; > >> + cfg.bufferCount = kNumBuffersSwISPMax; > >> + status = Adjusted; > >> + } > >> + } > >> } else { > >> V4L2DeviceFormat format; > >> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > >> @@ -1257,9 +1285,15 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > >> > >> cfg.stride = format.planes[0].bpl; > >> cfg.frameSize = format.planes[0].size; > >> - } > >> > >> - cfg.bufferCount = 4; > >> + if (cfg.bufferCount != kNumBuffersDefault) { > >> + LOG(SimplePipeline, Debug) > >> + << "Adjusting bufferCount from " << cfg.bufferCount > >> + << " to " << kNumBuffersDefault; > >> + cfg.bufferCount = kNumBuffersDefault; > >> + status = Adjusted; > >> + } > >> + } > >> } > >> > >> return status;
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 9b24a0db9..da871c7fb 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -375,6 +375,9 @@ public: const Transform &combinedTransform() const { return combinedTransform_; } private: + static constexpr unsigned int kNumBuffersDefault = 4; + static constexpr unsigned int kNumBuffersSwISPMax = 16; + /* * The SimpleCameraData instance is guaranteed to be valid as long as * the corresponding Camera instance is valid. In order to borrow a @@ -1246,6 +1249,31 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() cfg.size); if (cfg.stride == 0) return Invalid; + + if (data_->converter_) { + if (cfg.bufferCount != kNumBuffersDefault) { + LOG(SimplePipeline, Debug) + << "Adjusting bufferCount from " << cfg.bufferCount + << " to " << kNumBuffersDefault; + cfg.bufferCount = kNumBuffersDefault; + status = Adjusted; + } + } else { + if (cfg.bufferCount == 0) { + LOG(SimplePipeline, Debug) + << "Adjusting bufferCount from " << cfg.bufferCount + << " to " << kNumBuffersDefault; + cfg.bufferCount = kNumBuffersDefault; + status = Adjusted; + } + if (cfg.bufferCount > kNumBuffersSwISPMax) { + LOG(SimplePipeline, Debug) + << "Adjusting bufferCount from " << cfg.bufferCount + << " to " << kNumBuffersSwISPMax; + cfg.bufferCount = kNumBuffersSwISPMax; + status = Adjusted; + } + } } else { V4L2DeviceFormat format; format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); @@ -1257,9 +1285,15 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() cfg.stride = format.planes[0].bpl; cfg.frameSize = format.planes[0].size; - } - cfg.bufferCount = 4; + if (cfg.bufferCount != kNumBuffersDefault) { + LOG(SimplePipeline, Debug) + << "Adjusting bufferCount from " << cfg.bufferCount + << " to " << kNumBuffersDefault; + cfg.bufferCount = kNumBuffersDefault; + status = Adjusted; + } + } } return status;
When using the softwareISP we allocate the output buffers ourselves, usually from system memory. There's thus no strong reason to limit choices for client. Situations where this might be useful include: 1. Video encoding, e.g. encoding multiple buffers in parallel. 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. Thus allow up to 16 buffers - arbitrarily decided - while keeping the default to the previous 4. While on it, mark the config as adjusted if we did so. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- src/libcamera/pipeline/simple/simple.cpp | 38 ++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-)