| Message ID | 20250711175345.90318-3-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Fri, Jul 11, 2025 at 07:53:35PM +0200, Milan Zamazal wrote: > When software ISP is enabled, we want to be able to provide a raw stream > in addition to the processed stream. For this purpose, we need two > streams. If only the processed stream is requested, it doesn't harm to > allocate two. > > This is a hack for the lack of a better easy option. The number of > streams is determined as a camera property in the pipeline matching. > The actual number of streams needed (one or two) is determined only when > examining roles in SimplePipelineHandler::generateConfiguration. > > In theory, software ISP could produce multiple processed streams but > this is out of scope of this patch series. Hence two streams are > sufficient at the moment. > > When software ISP is not enabled, the camera won't be able to produce > multiple streams (assuming there's no hardware converter) and only > single stream should be allocated as before. The simple pipeline > handler assumes there's a linear pipeline from the camera sensor to a > video capture device, and only supports a single stream. Branches in > the hardware pipeline that would allow capturing multiple streams from > the same camera sensor are not supported. We have no plan to change > that, as a device that can produce multiple streams will likely be > better supported by a dedicated pipeline handler. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index d45480fe7..06532ed26 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -1723,7 +1723,18 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > } > } > > - swIspEnabled_ = info->swIspEnabled; > + if (info->swIspEnabled) { > + /* > + * When the software ISP is enabled, the simple pipeline handler > + * exposes the raw stream, giving a total of two streams. This > + * is mutally exclusive with the presence of a converter. > + */ > + ASSERT(!converter_); > + numStreams = 2; > + swIspEnabled_ = true; > + } else { > + swIspEnabled_ = false; > + } simply assign swIspEnabled_ to false, at construction time and get rid of else block. This is fixed up in my branch. > > /* Locate the sensors. */ > std::vector<MediaEntity *> sensors = locateSensors(media); > -- > 2.50.1 >
Hi Umang, Umang Jain <uajain@igalia.com> writes: > On Fri, Jul 11, 2025 at 07:53:35PM +0200, Milan Zamazal wrote: >> When software ISP is enabled, we want to be able to provide a raw stream >> in addition to the processed stream. For this purpose, we need two > >> streams. If only the processed stream is requested, it doesn't harm to >> allocate two. >> >> This is a hack for the lack of a better easy option. The number of >> streams is determined as a camera property in the pipeline matching. >> The actual number of streams needed (one or two) is determined only when >> examining roles in SimplePipelineHandler::generateConfiguration. >> >> In theory, software ISP could produce multiple processed streams but >> this is out of scope of this patch series. Hence two streams are >> sufficient at the moment. >> >> When software ISP is not enabled, the camera won't be able to produce >> multiple streams (assuming there's no hardware converter) and only >> single stream should be allocated as before. The simple pipeline >> handler assumes there's a linear pipeline from the camera sensor to a >> video capture device, and only supports a single stream. Branches in >> the hardware pipeline that would allow capturing multiple streams from >> the same camera sensor are not supported. We have no plan to change >> that, as a device that can produce multiple streams will likely be >> better supported by a dedicated pipeline handler. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index d45480fe7..06532ed26 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -1723,7 +1723,18 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) >> } >> } >> >> - swIspEnabled_ = info->swIspEnabled; >> + if (info->swIspEnabled) { >> + /* >> + * When the software ISP is enabled, the simple pipeline handler >> + * exposes the raw stream, giving a total of two streams. This >> + * is mutally exclusive with the presence of a converter. >> + */ >> + ASSERT(!converter_); >> + numStreams = 2; >> + swIspEnabled_ = true; >> + } else { >> + swIspEnabled_ = false; >> + } > > simply assign swIspEnabled_ to false, at construction time and get rid > of else block. This is fixed up in my branch. I think assigning it here is both clearer and safer -- the decision point is here, not in the construction time. >> /* Locate the sensors. */ >> std::vector<MediaEntity *> sensors = locateSensors(media); >> -- >> 2.50.1 >>
On Mon, Jul 21, 2025 at 09:36:06PM +0200, Milan Zamazal wrote: > Hi Umang, > > Umang Jain <uajain@igalia.com> writes: > > > On Fri, Jul 11, 2025 at 07:53:35PM +0200, Milan Zamazal wrote: > >> When software ISP is enabled, we want to be able to provide a raw stream > >> in addition to the processed stream. For this purpose, we need two > > > >> streams. If only the processed stream is requested, it doesn't harm to > >> allocate two. > >> > >> This is a hack for the lack of a better easy option. The number of > >> streams is determined as a camera property in the pipeline matching. > >> The actual number of streams needed (one or two) is determined only when > >> examining roles in SimplePipelineHandler::generateConfiguration. > >> > >> In theory, software ISP could produce multiple processed streams but > >> this is out of scope of this patch series. Hence two streams are > >> sufficient at the moment. > >> > >> When software ISP is not enabled, the camera won't be able to produce > >> multiple streams (assuming there's no hardware converter) and only > >> single stream should be allocated as before. The simple pipeline > >> handler assumes there's a linear pipeline from the camera sensor to a > >> video capture device, and only supports a single stream. Branches in > >> the hardware pipeline that would allow capturing multiple streams from > >> the same camera sensor are not supported. We have no plan to change > >> that, as a device that can produce multiple streams will likely be > >> better supported by a dedicated pipeline handler. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/libcamera/pipeline/simple/simple.cpp | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index d45480fe7..06532ed26 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -1723,7 +1723,18 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) > >> } > >> } > >> > >> - swIspEnabled_ = info->swIspEnabled; > >> + if (info->swIspEnabled) { > >> + /* > >> + * When the software ISP is enabled, the simple pipeline handler > >> + * exposes the raw stream, giving a total of two streams. This > >> + * is mutally exclusive with the presence of a converter. > >> + */ > >> + ASSERT(!converter_); > >> + numStreams = 2; > >> + swIspEnabled_ = true; > >> + } else { > >> + swIspEnabled_ = false; > >> + } > > > > simply assign swIspEnabled_ to false, at construction time and get rid > > of else block. This is fixed up in my branch. > > I think assigning it here is both clearer and safer -- the decision > point is here, not in the construction time. I see this as just a matter of caching the info->swIspEnabled boolean passed in supportedDevices[]. I don't see this as any kind of decision point. > > >> /* Locate the sensors. */ > >> std::vector<MediaEntity *> sensors = locateSensors(media); > >> -- > >> 2.50.1 > >> >
Umang Jain <uajain@igalia.com> writes: > On Mon, Jul 21, 2025 at 09:36:06PM +0200, Milan Zamazal wrote: >> Hi Umang, >> > >> Umang Jain <uajain@igalia.com> writes: >> >> > On Fri, Jul 11, 2025 at 07:53:35PM +0200, Milan Zamazal wrote: >> >> When software ISP is enabled, we want to be able to provide a raw stream >> >> in addition to the processed stream. For this purpose, we need two >> > >> >> streams. If only the processed stream is requested, it doesn't harm to >> >> allocate two. >> >> >> >> This is a hack for the lack of a better easy option. The number of >> >> streams is determined as a camera property in the pipeline matching. >> >> The actual number of streams needed (one or two) is determined only when >> >> examining roles in SimplePipelineHandler::generateConfiguration. >> >> >> >> In theory, software ISP could produce multiple processed streams but >> >> this is out of scope of this patch series. Hence two streams are >> >> sufficient at the moment. >> >> >> >> When software ISP is not enabled, the camera won't be able to produce >> >> multiple streams (assuming there's no hardware converter) and only >> >> single stream should be allocated as before. The simple pipeline >> >> handler assumes there's a linear pipeline from the camera sensor to a >> >> video capture device, and only supports a single stream. Branches in >> >> the hardware pipeline that would allow capturing multiple streams from >> >> the same camera sensor are not supported. We have no plan to change >> >> that, as a device that can produce multiple streams will likely be >> >> better supported by a dedicated pipeline handler. >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/libcamera/pipeline/simple/simple.cpp | 13 ++++++++++++- >> >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> >> index d45480fe7..06532ed26 100644 >> >> --- a/src/libcamera/pipeline/simple/simple.cpp >> >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> >> @@ -1723,7 +1723,18 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) >> >> } >> >> } >> >> >> >> - swIspEnabled_ = info->swIspEnabled; >> >> + if (info->swIspEnabled) { >> >> + /* >> >> + * When the software ISP is enabled, the simple pipeline handler >> >> + * exposes the raw stream, giving a total of two streams. This >> >> + * is mutally exclusive with the presence of a converter. >> >> + */ >> >> + ASSERT(!converter_); >> >> + numStreams = 2; >> >> + swIspEnabled_ = true; >> >> + } else { >> >> + swIspEnabled_ = false; >> >> + } >> > >> > simply assign swIspEnabled_ to false, at construction time and get rid >> > of else block. This is fixed up in my branch. >> >> I think assigning it here is both clearer and safer -- the decision >> point is here, not in the construction time. > > I see this as just a matter of caching the info->swIspEnabled boolean > passed in supportedDevices[]. I don't see this as any kind of decision point. Yes, but there is no idea about the value at the construction time, it could be set to true there as well; if it was set to a particular value just to avoid having an `else' branch in a completely different place, it'd only indicate it shouldn't be set at the construction time at all. >> >> >> /* Locate the sensors. */ >> >> std::vector<MediaEntity *> sensors = locateSensors(media); >> >> -- >> >> 2.50.1 >> >> >>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index d45480fe7..06532ed26 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -1723,7 +1723,18 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator) } } - swIspEnabled_ = info->swIspEnabled; + if (info->swIspEnabled) { + /* + * When the software ISP is enabled, the simple pipeline handler + * exposes the raw stream, giving a total of two streams. This + * is mutally exclusive with the presence of a converter. + */ + ASSERT(!converter_); + numStreams = 2; + swIspEnabled_ = true; + } else { + swIspEnabled_ = false; + } /* Locate the sensors. */ std::vector<MediaEntity *> sensors = locateSensors(media);
When software ISP is enabled, we want to be able to provide a raw stream in addition to the processed stream. For this purpose, we need two streams. If only the processed stream is requested, it doesn't harm to allocate two. This is a hack for the lack of a better easy option. The number of streams is determined as a camera property in the pipeline matching. The actual number of streams needed (one or two) is determined only when examining roles in SimplePipelineHandler::generateConfiguration. In theory, software ISP could produce multiple processed streams but this is out of scope of this patch series. Hence two streams are sufficient at the moment. When software ISP is not enabled, the camera won't be able to produce multiple streams (assuming there's no hardware converter) and only single stream should be allocated as before. The simple pipeline handler assumes there's a linear pipeline from the camera sensor to a video capture device, and only supports a single stream. Branches in the hardware pipeline that would allow capturing multiple streams from the same camera sensor are not supported. We have no plan to change that, as a device that can produce multiple streams will likely be better supported by a dedicated pipeline handler. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)