| Message ID | 20250723180815.82450-3-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Wed, Jul 23, 2025 at 08:08:07PM +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. I don't think that's a hack. numStreams in the match() function indicates how many streams the camera can produce. By allowing capture of raw frames and processed frames, the simple pipeline handler can produce two streams. How many streams are selected when configuring the camera is a different matter. > 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; > + } I'd write it as 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_ = info->swIspEnabled; Also, shouldn't this patch be moved later in the series ? This will create a second stream that applications can request, but generateConfiguration() and validate() are not ready. > > /* Locate the sensors. */ > std::vector<MediaEntity *> sensors = locateSensors(media);
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Wed, Jul 23, 2025 at 08:08:07PM +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. > > I don't think that's a hack. numStreams in the match() function > indicates how many streams the camera can produce. By allowing capture > of raw frames and processed frames, the simple pipeline handler can > produce two streams. How many streams are selected when configuring the > camera is a different matter. > >> 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; >> + } > > I'd write it as > > 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_ = info->swIspEnabled; OK. > Also, shouldn't this patch be moved later in the series ? This will > create a second stream that applications can request, but > generateConfiguration() and validate() are not ready. I can move it but all the patches are needed to make raw (=> multiple) streams working and are of no real value without the other patches so the order is not that important. With the exception of the first patch to assign colour spaces, which fixes a `cam' crash and can be merged separately. >> >> /* Locate the sensors. */ >> std::vector<MediaEntity *> sensors = locateSensors(media);
On Mon, Jul 28, 2025 at 11:53:17AM +0200, Milan Zamazal wrote: > Laurent Pinchart writes: > > On Wed, Jul 23, 2025 at 08:08:07PM +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. > > > > I don't think that's a hack. numStreams in the match() function > > indicates how many streams the camera can produce. By allowing capture > > of raw frames and processed frames, the simple pipeline handler can > > produce two streams. How many streams are selected when configuring the > > camera is a different matter. > > > >> 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; > >> + } > > > > I'd write it as > > > > 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_ = info->swIspEnabled; > > OK. > > > Also, shouldn't this patch be moved later in the series ? This will > > create a second stream that applications can request, but > > generateConfiguration() and validate() are not ready. > > I can move it but all the patches are needed to make raw (=> multiple) > streams working and are of no real value without the other patches so > the order is not that important. With the exception of the first patch > to assign colour spaces, which fixes a `cam' crash and can be merged > separately. Sure, I understand. My concern is that applying this patch early will likely break bisection. If patches can be easily reordered to avoid that, it would be nice. > >> > >> /* Locate the sensors. */ > >> std::vector<MediaEntity *> sensors = locateSensors(media);
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(-)