| Message ID | 20251127211932.122463-10-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan On 2025-11-28 02:49, Milan Zamazal wrote: > Configuration validation computes the maximum size of all the requested > streams and compares it to the output sizes. When e.g. only a raw > stream is requested then this may result in an invalid adjustment of its > size. This is because the output sizes are computed for processed > streams and may be smaller than capture sizes. If a raw stream with the > capture size is requested, it may then be wrongly adjusted to a larger > size because the output sizes, which are irrelevant for raw streams > anyway, are smaller than the requested capture size. > The problem is somewhere else and a quick look suggests that a wrong pipeConfig is getting selected to start with. Is this a bug with current series? Could you post steps to reproduce this ? > Let's fix the problem by tracking raw and processed streams maximum > sizes separately and comparing raw stream sizes against capture rather > than output sizes. This should ideally be squashed with the original validation patch, no ? Introducing new patches at v16, is not going to help here. It just slows things down for merging (unless there is a bug interaction happening then a split fixup! would've helped for easier review). I had problem with 8/9 as well as it should have been a separate patch and not merged with this but I let it slide already. But you do get my point of initially recommending to split out the colorspace patch (which you ultimately did), it progressed and got merged out of this series - but then you again had a fixup! to be put back in this series. This is just unwanted hindrance on the reviewer's end. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 2820d1254..bb67000e2 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -1153,12 +1153,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > } > > /* Find the largest stream size. */ > - Size maxStreamSize; > - for (const StreamConfiguration &cfg : config_) > - maxStreamSize.expandTo(cfg.size); > + Size maxProcessedStreamSize; > + Size maxRawStreamSize; > + for (const StreamConfiguration &cfg : config_) { > + if (isRaw(cfg)) > + maxRawStreamSize.expandTo(cfg.size); > + else > + maxProcessedStreamSize.expandTo(cfg.size); > + } > > LOG(SimplePipeline, Debug) > - << "Largest stream size is " << maxStreamSize; > + << "Largest stream size is " > + << maxProcessedStreamSize << "/" << maxRawStreamSize; This will be a very confusing log to parse. > > /* Cap the number of raw stream configurations */ > unsigned int rawCount = 0; > @@ -1217,8 +1223,10 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > const Size &captureSize = pipeConfig->captureSize; > const Size &maxOutputSize = pipeConfig->outputSizes.max; > > - if (maxOutputSize.width >= maxStreamSize.width && > - maxOutputSize.height >= maxStreamSize.height) { > + if (maxOutputSize.width >= maxProcessedStreamSize.width && > + maxOutputSize.height >= maxProcessedStreamSize.height && > + captureSize.width >= maxRawStreamSize.width && > + captureSize.height >= maxRawStreamSize.height) { > if (!pipeConfig_ || captureSize < pipeConfig_->captureSize) > pipeConfig_ = pipeConfig; > } > @@ -1236,7 +1244,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } > << " -> " << pipeConfig_->captureSize > << "-" << pipeConfig_->captureFormat > - << " for max stream size " << maxStreamSize; > + << " for max stream size " > + << maxProcessedStreamSize << "/" << maxRawStreamSize; > > /* > * Adjust the requested streams.
Hi Umang, thank you for review. Umang Jain <uajain@igalia.com> writes: > Hi Milan > > On 2025-11-28 02:49, Milan Zamazal wrote: >> Configuration validation computes the maximum size of all the requested >> streams and compares it to the output sizes. When e.g. only a raw >> stream is requested then this may result in an invalid adjustment of its >> size. This is because the output sizes are computed for processed >> streams and may be smaller than capture sizes. If a raw stream with the >> capture size is requested, it may then be wrongly adjusted to a larger >> size because the output sizes, which are irrelevant for raw streams >> anyway, are smaller than the requested capture size. >> > > The problem is somewhere else and a quick look suggests that a wrong > pipeConfig is getting selected to start with. Could you elaborate? > Is this a bug with current series? Could you post steps to reproduce > this ? I guess it is related to the fix of selecting the right processed stream resolution that has been merged to master some time ago. It can be reproduced by asking for a single raw stream with a capture resolution that is supported by the sensor but is lower than the maximum capture resolution. For example, in case of imx219, when asking for 1920x1080 resolution, a higher resolution 3280x246 is used instead, although 1920x1080 should be used. >> Let's fix the problem by tracking raw and processed streams maximum >> sizes separately and comparing raw stream sizes against capture rather >> than output sizes. > > This should ideally be squashed with the original validation patch, no ? > Introducing new patches at v16, is not going to help here. It just slows > things down for merging (unless there is a bug interaction happening > then a split fixup! would've helped for easier review). Maybe, but I don't like large patches and this change is easy to separate. But I can squash it if you prefer. > I had problem with 8/9 as well as it should have been a separate patch > and not merged with this but I let it slide already. But you do get my > point of initially recommending to split out the colorspace patch (which > you ultimately did), it progressed and got merged out of this series - > but then you again had a fixup! to be put back in this series. This is > just unwanted hindrance on the reviewer's end. Sorry about it, I'll remove that patch from v17. >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 23 ++++++++++++++++------- >> 1 file changed, 16 insertions(+), 7 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 2820d1254..bb67000e2 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -1153,12 +1153,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> } >> >> /* Find the largest stream size. */ >> - Size maxStreamSize; >> - for (const StreamConfiguration &cfg : config_) >> - maxStreamSize.expandTo(cfg.size); >> + Size maxProcessedStreamSize; >> + Size maxRawStreamSize; >> + for (const StreamConfiguration &cfg : config_) { >> + if (isRaw(cfg)) >> + maxRawStreamSize.expandTo(cfg.size); >> + else >> + maxProcessedStreamSize.expandTo(cfg.size); >> + } >> >> LOG(SimplePipeline, Debug) >> - << "Largest stream size is " << maxStreamSize; >> + << "Largest stream size is " >> + << maxProcessedStreamSize << "/" << maxRawStreamSize; > > This will be a very confusing log to parse. I was thinking about putting "processed" and "raw" around but I didn't like the output, so I left with the version above, it's only a debug message after all. But suggestions for improvement are welcome. >> >> /* Cap the number of raw stream configurations */ >> unsigned int rawCount = 0; >> @@ -1217,8 +1223,10 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> const Size &captureSize = pipeConfig->captureSize; >> const Size &maxOutputSize = pipeConfig->outputSizes.max; >> >> - if (maxOutputSize.width >= maxStreamSize.width && >> - maxOutputSize.height >= maxStreamSize.height) { >> + if (maxOutputSize.width >= maxProcessedStreamSize.width && >> + maxOutputSize.height >= maxProcessedStreamSize.height && >> + captureSize.width >= maxRawStreamSize.width && >> + captureSize.height >= maxRawStreamSize.height) { >> if (!pipeConfig_ || captureSize < pipeConfig_->captureSize) >> pipeConfig_ = pipeConfig; >> } >> @@ -1236,7 +1244,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } >> << " -> " << pipeConfig_->captureSize >> << "-" << pipeConfig_->captureFormat >> - << " for max stream size " << maxStreamSize; >> + << " for max stream size " >> + << maxProcessedStreamSize << "/" << maxRawStreamSize; >> >> /* >> * Adjust the requested streams.
On 2025-12-01 19:06, Milan Zamazal wrote: > Hi Umang, > > thank you for review. > > Umang Jain <uajain@igalia.com> writes: > >> Hi Milan >> >> On 2025-11-28 02:49, Milan Zamazal wrote: >>> Configuration validation computes the maximum size of all the requested >>> streams and compares it to the output sizes. When e.g. only a raw >>> stream is requested then this may result in an invalid adjustment of its >>> size. This is because the output sizes are computed for processed >>> streams and may be smaller than capture sizes. If a raw stream with the >>> capture size is requested, it may then be wrongly adjusted to a larger >>> size because the output sizes, which are irrelevant for raw streams >>> anyway, are smaller than the requested capture size. >>> >> >> The problem is somewhere else and a quick look suggests that a wrong >> pipeConfig is getting selected to start with. > > Could you elaborate? > >> Is this a bug with current series? Could you post steps to reproduce >> this ? > > I guess it is related to the fix of selecting the right processed stream > resolution that has been merged to master some time ago. > > It can be reproduced by asking for a single raw stream with a capture > resolution that is supported by the sensor but is lower than the maximum > capture resolution. For example, in case of imx219, when asking for > 1920x1080 resolution, a higher resolution 3280x246 is used instead, > although 1920x1080 should be used. I quickly checked on my earlier implementation, I do not face this issue: ``` uajain1@uajain:~$ ./libcamera/build/src/apps/cam/cam -c1 -srole=raw,width=1920,height=1080 -C [0:35:56.822871937] [451] INFO IPAManager ipa_manager.cpp:137 libcamera is not installed. Adding '/home/uajain1/libcamera/build/src/ipa' to the IPA search path [0:35:56.846769072] [451] INFO Camera camera_manager.cpp:326 libcamera v0.5.1+64-eba4ccc8 [0:35:56.942874228] [452] INFO IPAProxy ipa_proxy.cpp:151 libcamera is not installed. Loading IPA configuration from '/home/uajain1/libcamera/src/ipa/simple/data' [0:35:56.943061676] [452] WARN IPAProxy ipa_proxy.cpp:177 Configuration file 'imx219.yaml' not found for IPA module 'simple', falling back to 'uncalibrated.yaml' [0:35:56.943659124] [452] INFO IPAProxy ipa_proxy.cpp:151 libcamera is not installed. Loading IPA configuration from '/home/uajain1/libcamera/src/ipa/simple/data' Using camera /base/soc/i2c0mux/i2c@1/imx219@10 as cam0 [0:35:56.950627718] [451] INFO Camera camera.cpp:1205 configuring streams: (0) 1920x1080-SRGGB10/RAW [0:35:56.951063708] [452] INFO SimplePipeline simple.cpp:1479 data->useConversion_ : 0 cam0: Capture until user interrupts by SIGINT 2157.040698 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 4147200 2157.074024 (30.01 fps) cam0-stream0 seq: 000001 bytesused: 4147200 2157.107350 (30.01 fps) cam0-stream0 seq: 000002 bytesused: 4147200 2157.140677 (30.01 fps) cam0-stream0 seq: 000003 bytesused: 4147200 2157.174004 (30.01 fps) cam0-stream0 seq: 000004 bytesused: 4147200 2157.207331 (30.01 fps) cam0-stream0 seq: 000005 bytesused: 4147200 ... ``` So probably I'll take a look deeper in this week to see what's really going with the pipeConfig selection. > >>> Let's fix the problem by tracking raw and processed streams maximum >>> sizes separately and comparing raw stream sizes against capture rather >>> than output sizes. >> >> This should ideally be squashed with the original validation patch, no ? >> Introducing new patches at v16, is not going to help here. It just slows >> things down for merging (unless there is a bug interaction happening >> then a split fixup! would've helped for easier review). > > Maybe, but I don't like large patches and this change is easy to > separate. But I can squash it if you prefer. > >> I had problem with 8/9 as well as it should have been a separate patch >> and not merged with this but I let it slide already. But you do get my >> point of initially recommending to split out the colorspace patch (which >> you ultimately did), it progressed and got merged out of this series - >> but then you again had a fixup! to be put back in this series. This is >> just unwanted hindrance on the reviewer's end. > > Sorry about it, I'll remove that patch from v17. > >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/libcamera/pipeline/simple/simple.cpp | 23 ++++++++++++++++------- >>> 1 file changed, 16 insertions(+), 7 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>> index 2820d1254..bb67000e2 100644 >>> --- a/src/libcamera/pipeline/simple/simple.cpp >>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>> @@ -1153,12 +1153,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> } >>> >>> /* Find the largest stream size. */ >>> - Size maxStreamSize; >>> - for (const StreamConfiguration &cfg : config_) >>> - maxStreamSize.expandTo(cfg.size); >>> + Size maxProcessedStreamSize; >>> + Size maxRawStreamSize; >>> + for (const StreamConfiguration &cfg : config_) { >>> + if (isRaw(cfg)) >>> + maxRawStreamSize.expandTo(cfg.size); >>> + else >>> + maxProcessedStreamSize.expandTo(cfg.size); >>> + } >>> >>> LOG(SimplePipeline, Debug) >>> - << "Largest stream size is " << maxStreamSize; >>> + << "Largest stream size is " >>> + << maxProcessedStreamSize << "/" << maxRawStreamSize; >> >> This will be a very confusing log to parse. > > I was thinking about putting "processed" and "raw" around but I didn't > like the output, so I left with the version above, it's only a debug > message after all. But suggestions for improvement are welcome. > >>> >>> /* Cap the number of raw stream configurations */ >>> unsigned int rawCount = 0; >>> @@ -1217,8 +1223,10 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> const Size &captureSize = pipeConfig->captureSize; >>> const Size &maxOutputSize = pipeConfig->outputSizes.max; >>> >>> - if (maxOutputSize.width >= maxStreamSize.width && >>> - maxOutputSize.height >= maxStreamSize.height) { >>> + if (maxOutputSize.width >= maxProcessedStreamSize.width && >>> + maxOutputSize.height >= maxProcessedStreamSize.height && >>> + captureSize.width >= maxRawStreamSize.width && >>> + captureSize.height >= maxRawStreamSize.height) { >>> if (!pipeConfig_ || captureSize < pipeConfig_->captureSize) >>> pipeConfig_ = pipeConfig; >>> } >>> @@ -1236,7 +1244,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } >>> << " -> " << pipeConfig_->captureSize >>> << "-" << pipeConfig_->captureFormat >>> - << " for max stream size " << maxStreamSize; >>> + << " for max stream size " >>> + << maxProcessedStreamSize << "/" << maxRawStreamSize; >>> >>> /* >>> * Adjust the requested streams.
Umang Jain <uajain@igalia.com> writes: > On 2025-12-01 19:06, Milan Zamazal wrote: >> Hi Umang, >> > >> thank you for review. >> >> Umang Jain <uajain@igalia.com> writes: >> >>> Hi Milan >>> >>> On 2025-11-28 02:49, Milan Zamazal wrote: >>>> Configuration validation computes the maximum size of all the requested >>>> streams and compares it to the output sizes. When e.g. only a raw >>>> stream is requested then this may result in an invalid adjustment of its >>>> size. This is because the output sizes are computed for processed >>>> streams and may be smaller than capture sizes. If a raw stream with the >>>> capture size is requested, it may then be wrongly adjusted to a larger >>>> size because the output sizes, which are irrelevant for raw streams >>>> anyway, are smaller than the requested capture size. >>>> >>> >>> The problem is somewhere else and a quick look suggests that a wrong >>> pipeConfig is getting selected to start with. >> >> Could you elaborate? >> >>> Is this a bug with current series? Could you post steps to reproduce >>> this ? >> >> I guess it is related to the fix of selecting the right processed stream >> resolution that has been merged to master some time ago. >> >> It can be reproduced by asking for a single raw stream with a capture >> resolution that is supported by the sensor but is lower than the maximum >> capture resolution. For example, in case of imx219, when asking for >> 1920x1080 resolution, a higher resolution 3280x246 is used instead, >> although 1920x1080 should be used. > > I quickly checked on my earlier implementation, I do not face this > issue: Did you rebase on current master? > > ``` > uajain1@uajain:~$ ./libcamera/build/src/apps/cam/cam -c1 > -srole=raw,width=1920,height=1080 -C > [0:35:56.822871937] [451] INFO IPAManager ipa_manager.cpp:137 libcamera > is not installed. Adding '/home/uajain1/libcamera/build/src/ipa' to the > IPA search path > [0:35:56.846769072] [451] INFO Camera camera_manager.cpp:326 libcamera > v0.5.1+64-eba4ccc8 > [0:35:56.942874228] [452] INFO IPAProxy ipa_proxy.cpp:151 libcamera is > not installed. Loading IPA configuration from > '/home/uajain1/libcamera/src/ipa/simple/data' > [0:35:56.943061676] [452] WARN IPAProxy ipa_proxy.cpp:177 Configuration > file 'imx219.yaml' not found for IPA module 'simple', falling back to > 'uncalibrated.yaml' > [0:35:56.943659124] [452] INFO IPAProxy ipa_proxy.cpp:151 libcamera is > not installed. Loading IPA configuration from > '/home/uajain1/libcamera/src/ipa/simple/data' > Using camera /base/soc/i2c0mux/i2c@1/imx219@10 as cam0 > [0:35:56.950627718] [451] INFO Camera camera.cpp:1205 configuring > streams: (0) 1920x1080-SRGGB10/RAW > [0:35:56.951063708] [452] INFO SimplePipeline simple.cpp:1479 > data->useConversion_ : 0 > cam0: Capture until user interrupts by SIGINT > 2157.040698 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 4147200 > 2157.074024 (30.01 fps) cam0-stream0 seq: 000001 bytesused: 4147200 > 2157.107350 (30.01 fps) cam0-stream0 seq: 000002 bytesused: 4147200 > 2157.140677 (30.01 fps) cam0-stream0 seq: 000003 bytesused: 4147200 > 2157.174004 (30.01 fps) cam0-stream0 seq: 000004 bytesused: 4147200 > 2157.207331 (30.01 fps) cam0-stream0 seq: 000005 bytesused: 4147200 > ... > ``` > > So probably I'll take a look deeper in this week to see what's really > going with the pipeConfig selection. OK, thank you. >> >>>> Let's fix the problem by tracking raw and processed streams maximum >>>> sizes separately and comparing raw stream sizes against capture rather >>>> than output sizes. >>> >>> This should ideally be squashed with the original validation patch, no ? >>> Introducing new patches at v16, is not going to help here. It just slows >>> things down for merging (unless there is a bug interaction happening >>> then a split fixup! would've helped for easier review). >> >> Maybe, but I don't like large patches and this change is easy to >> separate. But I can squash it if you prefer. >> >>> I had problem with 8/9 as well as it should have been a separate patch >>> and not merged with this but I let it slide already. But you do get my >>> point of initially recommending to split out the colorspace patch (which >>> you ultimately did), it progressed and got merged out of this series - >>> but then you again had a fixup! to be put back in this series. This is >>> just unwanted hindrance on the reviewer's end. >> >> Sorry about it, I'll remove that patch from v17. >> >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/libcamera/pipeline/simple/simple.cpp | 23 ++++++++++++++++------- >>>> 1 file changed, 16 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>>> index 2820d1254..bb67000e2 100644 >>>> --- a/src/libcamera/pipeline/simple/simple.cpp >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>>> @@ -1153,12 +1153,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>>> } >>>> >>>> /* Find the largest stream size. */ >>>> - Size maxStreamSize; >>>> - for (const StreamConfiguration &cfg : config_) >>>> - maxStreamSize.expandTo(cfg.size); >>>> + Size maxProcessedStreamSize; >>>> + Size maxRawStreamSize; >>>> + for (const StreamConfiguration &cfg : config_) { >>>> + if (isRaw(cfg)) >>>> + maxRawStreamSize.expandTo(cfg.size); >>>> + else >>>> + maxProcessedStreamSize.expandTo(cfg.size); >>>> + } >>>> >>>> LOG(SimplePipeline, Debug) >>>> - << "Largest stream size is " << maxStreamSize; >>>> + << "Largest stream size is " >>>> + << maxProcessedStreamSize << "/" << maxRawStreamSize; >>> >>> This will be a very confusing log to parse. >> >> I was thinking about putting "processed" and "raw" around but I didn't >> like the output, so I left with the version above, it's only a debug >> message after all. But suggestions for improvement are welcome. >> >>>> >>>> /* Cap the number of raw stream configurations */ >>>> unsigned int rawCount = 0; >>>> @@ -1217,8 +1223,10 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>>> const Size &captureSize = pipeConfig->captureSize; >>>> const Size &maxOutputSize = pipeConfig->outputSizes.max; >>>> >>>> - if (maxOutputSize.width >= maxStreamSize.width && >>>> - maxOutputSize.height >= maxStreamSize.height) { >>>> + if (maxOutputSize.width >= maxProcessedStreamSize.width && >>>> + maxOutputSize.height >= maxProcessedStreamSize.height && >>>> + captureSize.width >= maxRawStreamSize.width && >>>> + captureSize.height >= maxRawStreamSize.height) { >>>> if (!pipeConfig_ || captureSize < pipeConfig_->captureSize) >>>> pipeConfig_ = pipeConfig; >>>> } >>>> @@ -1236,7 +1244,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>>> << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } >>>> << " -> " << pipeConfig_->captureSize >>>> << "-" << pipeConfig_->captureFormat >>>> - << " for max stream size " << maxStreamSize; >>>> + << " for max stream size " >>>> + << maxProcessedStreamSize << "/" << maxRawStreamSize; >>>> >>>> /* >>>> * Adjust the requested streams.
On 2025-12-01 20:07, Milan Zamazal wrote: > Umang Jain <uajain@igalia.com> writes: > >> On 2025-12-01 19:06, Milan Zamazal wrote: >>> Hi Umang, >>> >> >>> thank you for review. >>> >>> Umang Jain <uajain@igalia.com> writes: >>> >>>> Hi Milan >>>> >>>> On 2025-11-28 02:49, Milan Zamazal wrote: >>>>> Configuration validation computes the maximum size of all the requested >>>>> streams and compares it to the output sizes. When e.g. only a raw >>>>> stream is requested then this may result in an invalid adjustment of its >>>>> size. This is because the output sizes are computed for processed >>>>> streams and may be smaller than capture sizes. If a raw stream with the >>>>> capture size is requested, it may then be wrongly adjusted to a larger >>>>> size because the output sizes, which are irrelevant for raw streams >>>>> anyway, are smaller than the requested capture size. >>>>> >>>> >>>> The problem is somewhere else and a quick look suggests that a wrong >>>> pipeConfig is getting selected to start with. >>> >>> Could you elaborate? >>> >>>> Is this a bug with current series? Could you post steps to reproduce >>>> this ? >>> >>> I guess it is related to the fix of selecting the right processed stream >>> resolution that has been merged to master some time ago. >>> >>> It can be reproduced by asking for a single raw stream with a capture >>> resolution that is supported by the sensor but is lower than the maximum >>> capture resolution. For example, in case of imx219, when asking for >>> 1920x1080 resolution, a higher resolution 3280x246 is used instead, >>> although 1920x1080 should be used. >> >> I quickly checked on my earlier implementation, I do not face this >> issue: > > Did you rebase on current master? Ack, this was it, some changes led to due interaction with Commit 94d32fdc55a3d (pipeline: simple: Consider output sizes when choosing pipe config), which was missing on my non-rebased branch (and I also completely forgot about it). > >> >> ``` >> uajain1@uajain:~$ ./libcamera/build/src/apps/cam/cam -c1 >> -srole=raw,width=1920,height=1080 -C >> [0:35:56.822871937] [451] INFO IPAManager ipa_manager.cpp:137 libcamera >> is not installed. Adding '/home/uajain1/libcamera/build/src/ipa' to the >> IPA search path >> [0:35:56.846769072] [451] INFO Camera camera_manager.cpp:326 libcamera >> v0.5.1+64-eba4ccc8 >> [0:35:56.942874228] [452] INFO IPAProxy ipa_proxy.cpp:151 libcamera is >> not installed. Loading IPA configuration from >> '/home/uajain1/libcamera/src/ipa/simple/data' >> [0:35:56.943061676] [452] WARN IPAProxy ipa_proxy.cpp:177 Configuration >> file 'imx219.yaml' not found for IPA module 'simple', falling back to >> 'uncalibrated.yaml' >> [0:35:56.943659124] [452] INFO IPAProxy ipa_proxy.cpp:151 libcamera is >> not installed. Loading IPA configuration from >> '/home/uajain1/libcamera/src/ipa/simple/data' >> Using camera /base/soc/i2c0mux/i2c@1/imx219@10 as cam0 >> [0:35:56.950627718] [451] INFO Camera camera.cpp:1205 configuring >> streams: (0) 1920x1080-SRGGB10/RAW >> [0:35:56.951063708] [452] INFO SimplePipeline simple.cpp:1479 >> data->useConversion_ : 0 >> cam0: Capture until user interrupts by SIGINT >> 2157.040698 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 4147200 >> 2157.074024 (30.01 fps) cam0-stream0 seq: 000001 bytesused: 4147200 >> 2157.107350 (30.01 fps) cam0-stream0 seq: 000002 bytesused: 4147200 >> 2157.140677 (30.01 fps) cam0-stream0 seq: 000003 bytesused: 4147200 >> 2157.174004 (30.01 fps) cam0-stream0 seq: 000004 bytesused: 4147200 >> 2157.207331 (30.01 fps) cam0-stream0 seq: 000005 bytesused: 4147200 >> ... >> ``` >> >> So probably I'll take a look deeper in this week to see what's really >> going with the pipeConfig selection. > > OK, thank you. > >>> >>>>> Let's fix the problem by tracking raw and processed streams maximum >>>>> sizes separately and comparing raw stream sizes against capture rather >>>>> than output sizes. >>>> >>>> This should ideally be squashed with the original validation patch, no ? >>>> Introducing new patches at v16, is not going to help here. It just slows >>>> things down for merging (unless there is a bug interaction happening >>>> then a split fixup! would've helped for easier review). >>> >>> Maybe, but I don't like large patches and this change is easy to >>> separate. But I can squash it if you prefer. >>> >>>> I had problem with 8/9 as well as it should have been a separate patch >>>> and not merged with this but I let it slide already. But you do get my >>>> point of initially recommending to split out the colorspace patch (which >>>> you ultimately did), it progressed and got merged out of this series - >>>> but then you again had a fixup! to be put back in this series. This is >>>> just unwanted hindrance on the reviewer's end. >>> >>> Sorry about it, I'll remove that patch from v17. >>> >>>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>>> --- >>>>> src/libcamera/pipeline/simple/simple.cpp | 23 ++++++++++++++++------- >>>>> 1 file changed, 16 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>>>> index 2820d1254..bb67000e2 100644 >>>>> --- a/src/libcamera/pipeline/simple/simple.cpp >>>>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>>>> @@ -1153,12 +1153,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>>>> } >>>>> >>>>> /* Find the largest stream size. */ s/size/sizes/ >>>>> - Size maxStreamSize; >>>>> - for (const StreamConfiguration &cfg : config_) >>>>> - maxStreamSize.expandTo(cfg.size); >>>>> + Size maxProcessedStreamSize; >>>>> + Size maxRawStreamSize; >>>>> + for (const StreamConfiguration &cfg : config_) { >>>>> + if (isRaw(cfg)) >>>>> + maxRawStreamSize.expandTo(cfg.size); >>>>> + else >>>>> + maxProcessedStreamSize.expandTo(cfg.size); >>>>> + } >>>>> >>>>> LOG(SimplePipeline, Debug) >>>>> - << "Largest stream size is " << maxStreamSize; >>>>> + << "Largest stream size is " >>>>> + << maxProcessedStreamSize << "/" << maxRawStreamSize; >>>> >>>> This will be a very confusing log to parse. >>> >>> I was thinking about putting "processed" and "raw" around but I didn't >>> like the output, so I left with the version above, it's only a debug >>> message after all. But suggestions for improvement are welcome. IMO, it is completely fine to see: Largest processed stream size <size> Largest RAW stream size <size> The problem with our version is that, it is only printing <size> not the format (this makes differentiation between raw/non-raw). So it's confusing if someone is parsing the debug log with <size> as the only parameter. Rest now looks good to me. Probably this can be squashed with validation patch. >>> >>>>> >>>>> /* Cap the number of raw stream configurations */ >>>>> unsigned int rawCount = 0; >>>>> @@ -1217,8 +1223,10 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>>>> const Size &captureSize = pipeConfig->captureSize; >>>>> const Size &maxOutputSize = pipeConfig->outputSizes.max; >>>>> >>>>> - if (maxOutputSize.width >= maxStreamSize.width && >>>>> - maxOutputSize.height >= maxStreamSize.height) { >>>>> + if (maxOutputSize.width >= maxProcessedStreamSize.width && >>>>> + maxOutputSize.height >= maxProcessedStreamSize.height && >>>>> + captureSize.width >= maxRawStreamSize.width && >>>>> + captureSize.height >= maxRawStreamSize.height) { >>>>> if (!pipeConfig_ || captureSize < pipeConfig_->captureSize) >>>>> pipeConfig_ = pipeConfig; >>>>> } >>>>> @@ -1236,7 +1244,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>>>> << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } >>>>> << " -> " << pipeConfig_->captureSize >>>>> << "-" << pipeConfig_->captureFormat >>>>> - << " for max stream size " << maxStreamSize; >>>>> + << " for max stream size " >>>>> + << maxProcessedStreamSize << "/" << maxRawStreamSize; >>>>> >>>>> /* >>>>> * Adjust the requested streams.
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 2820d1254..bb67000e2 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -1153,12 +1153,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() } /* Find the largest stream size. */ - Size maxStreamSize; - for (const StreamConfiguration &cfg : config_) - maxStreamSize.expandTo(cfg.size); + Size maxProcessedStreamSize; + Size maxRawStreamSize; + for (const StreamConfiguration &cfg : config_) { + if (isRaw(cfg)) + maxRawStreamSize.expandTo(cfg.size); + else + maxProcessedStreamSize.expandTo(cfg.size); + } LOG(SimplePipeline, Debug) - << "Largest stream size is " << maxStreamSize; + << "Largest stream size is " + << maxProcessedStreamSize << "/" << maxRawStreamSize; /* Cap the number of raw stream configurations */ unsigned int rawCount = 0; @@ -1217,8 +1223,10 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() const Size &captureSize = pipeConfig->captureSize; const Size &maxOutputSize = pipeConfig->outputSizes.max; - if (maxOutputSize.width >= maxStreamSize.width && - maxOutputSize.height >= maxStreamSize.height) { + if (maxOutputSize.width >= maxProcessedStreamSize.width && + maxOutputSize.height >= maxProcessedStreamSize.height && + captureSize.width >= maxRawStreamSize.width && + captureSize.height >= maxRawStreamSize.height) { if (!pipeConfig_ || captureSize < pipeConfig_->captureSize) pipeConfig_ = pipeConfig; } @@ -1236,7 +1244,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } << " -> " << pipeConfig_->captureSize << "-" << pipeConfig_->captureFormat - << " for max stream size " << maxStreamSize; + << " for max stream size " + << maxProcessedStreamSize << "/" << maxRawStreamSize; /* * Adjust the requested streams.
Configuration validation computes the maximum size of all the requested streams and compares it to the output sizes. When e.g. only a raw stream is requested then this may result in an invalid adjustment of its size. This is because the output sizes are computed for processed streams and may be smaller than capture sizes. If a raw stream with the capture size is requested, it may then be wrongly adjusted to a larger size because the output sizes, which are irrelevant for raw streams anyway, are smaller than the requested capture size. Let's fix the problem by tracking raw and processed streams maximum sizes separately and comparing raw stream sizes against capture rather than output sizes. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)