| Message ID | 20210310154414.3560115-1-niklas.soderlund@ragnatech.se |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On 10/03/2021 15:44, Niklas Söderlund wrote: > Hello, > > This series adds a compliance tool to libcamera. It was developed out of > necessity while extending and debugging the IPU3 pipeline hander. Each > test in the tool so far have at one point triggered fatal issues in one > pipeline or another. All known issues are however either fixed or have > patches on ML to address them. There's more (or at least now) but this tool has helped in flushing out a few. The raspberry pi pipeline handler seems to fail a few of these tests. But all the more reason to integrate this. I can't say I've done a detailed review but I can say this: For both: Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> On IPU3 (with my stability branch): 120 tests executed, 96 tests passed, 24 tests skipped and 0 tests failed On vivid: 120 tests executed, 96 tests passed, 24 tests skipped and 0 tests failed On UVC: 120 tests executed, 96 tests passed, 24 tests skipped and 0 tests failed On RPi: 120 tests executed, 78 tests passed, 12 tests skipped and 30 tests failed Naush - I had to do a quick fudge fix in the configure path to get this to run at all - so it might be interesting for you to try running this. > The tool is simple at the moment and only tests a single stream. As our > use-cases grow more complex I hope to find time to extend the tool to > cover more areas. Future feature request --fail-fast So if it fails, it halts? (To help see what to fix) > > Niklas Söderlund (2): > lc-compliance: Add a libcamera compliance tool > lc-compliance: Add test stopping single stream with requests queued > > src/lc-compliance/main.cpp | 139 ++++++++++++++++++ > src/lc-compliance/meson.build | 25 ++++ > src/lc-compliance/results.cpp | 75 ++++++++++ > src/lc-compliance/results.h | 45 ++++++ > src/lc-compliance/simple_capture.cpp | 212 +++++++++++++++++++++++++++ > src/lc-compliance/simple_capture.h | 68 +++++++++ > src/lc-compliance/single_stream.cpp | 98 +++++++++++++ > src/lc-compliance/tests.h | 16 ++ > src/meson.build | 2 + > 9 files changed, 680 insertions(+) > create mode 100644 src/lc-compliance/main.cpp > create mode 100644 src/lc-compliance/meson.build > create mode 100644 src/lc-compliance/results.cpp > create mode 100644 src/lc-compliance/results.h > create mode 100644 src/lc-compliance/simple_capture.cpp > create mode 100644 src/lc-compliance/simple_capture.h > create mode 100644 src/lc-compliance/single_stream.cpp > create mode 100644 src/lc-compliance/tests.h >
Hi Kieran and Niklas, On Fri, 12 Mar 2021 at 14:44, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Niklas, > > On 10/03/2021 15:44, Niklas Söderlund wrote: > > Hello, > > > > This series adds a compliance tool to libcamera. It was developed out of > > necessity while extending and debugging the IPU3 pipeline hander. Each > > test in the tool so far have at one point triggered fatal issues in one > > pipeline or another. All known issues are however either fixed or have > > patches on ML to address them. > > There's more (or at least now) but this tool has helped in flushing out > a few. > > The raspberry pi pipeline handler seems to fail a few of these tests. > But all the more reason to integrate this. > > I can't say I've done a detailed review but I can say this: > > > For both: > > Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > On IPU3 (with my stability branch): > 120 tests executed, 96 tests passed, 24 tests skipped and 0 tests failed > > On vivid: > 120 tests executed, 96 tests passed, 24 tests skipped and 0 tests failed > > On UVC: > 120 tests executed, 96 tests passed, 24 tests skipped and 0 tests failed > > > On RPi: > 120 tests executed, 78 tests passed, 12 tests skipped and 30 tests failed > > Naush - I had to do a quick fudge fix in the configure path to get this > to run at all - so it might be interesting for you to try running this. > Thanks for the heads-up. This is indeed going to be a very handy tool for everyone. Regarding the results, I think I see the cause of the failures. It is due to the RPi pipeline handler not correctly handling only a RAW stream (i.e. no ISP output). Our entire flow depends on it, including running the IPA for metering stats. The solution here would be to allow our ISP to run so that it only generates stats output and no image output. However, this is not a trivial change, and since we have never run this way before, my confidence levels are not all that high. A simpler fix would be to get the ISP to generate a small (say 320x240) output image for internal use only so the pipeline behaves correctly. Having a quick go at this change seems to fix all failures. For the skipped tests, I think we did have a brief discussion with Niklas some time back when the tool was first introduced. We advertise a particular buffer count for each SteramRole in generateConfiguration(), and I think the tool skips the tests with number of Requests lower than that count. However, this number is merely a guide and our pipeline handler ought to work correctly with only a single Request queued. We did not reach a conclusion on what was the right way to approach this back then, but I think we said that I should not set our buffer counts down to one. Niklas should we consider another number besides buffer count returned out of generateConfiguration()? Thanks, Naush > > > The tool is simple at the moment and only tests a single stream. As our > > use-cases grow more complex I hope to find time to extend the tool to > > cover more areas. > > Future feature request > --fail-fast > > So if it fails, it halts? (To help see what to fix) > > > > > Niklas Söderlund (2): > > lc-compliance: Add a libcamera compliance tool > > lc-compliance: Add test stopping single stream with requests queued > > > > src/lc-compliance/main.cpp | 139 ++++++++++++++++++ > > src/lc-compliance/meson.build | 25 ++++ > > src/lc-compliance/results.cpp | 75 ++++++++++ > > src/lc-compliance/results.h | 45 ++++++ > > src/lc-compliance/simple_capture.cpp | 212 +++++++++++++++++++++++++++ > > src/lc-compliance/simple_capture.h | 68 +++++++++ > > src/lc-compliance/single_stream.cpp | 98 +++++++++++++ > > src/lc-compliance/tests.h | 16 ++ > > src/meson.build | 2 + > > 9 files changed, 680 insertions(+) > > create mode 100644 src/lc-compliance/main.cpp > > create mode 100644 src/lc-compliance/meson.build > > create mode 100644 src/lc-compliance/results.cpp > > create mode 100644 src/lc-compliance/results.h > > create mode 100644 src/lc-compliance/simple_capture.cpp > > create mode 100644 src/lc-compliance/simple_capture.h > > create mode 100644 src/lc-compliance/single_stream.cpp > > create mode 100644 src/lc-compliance/tests.h > > > > -- > Regards > -- > Kieran >
Hi Niklas, On Wed, Mar 10, 2021 at 04:44:12PM +0100, Niklas Söderlund wrote: > Hello, > > This series adds a compliance tool to libcamera. It was developed out of > necessity while extending and debugging the IPU3 pipeline hander. Each > test in the tool so far have at one point triggered fatal issues in one > pipeline or another. All known issues are however either fixed or have > patches on ML to address them. > > The tool is simple at the moment and only tests a single stream. As our > use-cases grow more complex I hope to find time to extend the tool to > cover more areas. Lots of work remaining on the test front :-) This is a good starting point. I've reviewed both patches, with lots of comments. Please address the ones you think should be addressed immediately, and record the other ones in \todo comments. I see no blocking issue, so I expect to ack v4. > Niklas Söderlund (2): > lc-compliance: Add a libcamera compliance tool > lc-compliance: Add test stopping single stream with requests queued > > src/lc-compliance/main.cpp | 139 ++++++++++++++++++ > src/lc-compliance/meson.build | 25 ++++ > src/lc-compliance/results.cpp | 75 ++++++++++ > src/lc-compliance/results.h | 45 ++++++ > src/lc-compliance/simple_capture.cpp | 212 +++++++++++++++++++++++++++ > src/lc-compliance/simple_capture.h | 68 +++++++++ > src/lc-compliance/single_stream.cpp | 98 +++++++++++++ > src/lc-compliance/tests.h | 16 ++ > src/meson.build | 2 + > 9 files changed, 680 insertions(+) > create mode 100644 src/lc-compliance/main.cpp > create mode 100644 src/lc-compliance/meson.build > create mode 100644 src/lc-compliance/results.cpp > create mode 100644 src/lc-compliance/results.h > create mode 100644 src/lc-compliance/simple_capture.cpp > create mode 100644 src/lc-compliance/simple_capture.h > create mode 100644 src/lc-compliance/single_stream.cpp > create mode 100644 src/lc-compliance/tests.h