| Message ID | 20241009200110.275544-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, On Wed, Oct 09, 2024 at 10:01:06PM +0200, Hans de Goede wrote: > Hi All, > > Here is a patch series adding a processFrame() method to gather > statistics on an entire frame in one go, rather then using the > line by line approach which is used by the somewhat tightly coupled > DebayerCpu class. > > ATM there are no users for processFrame() yet, so the code is > compile tested only which is why it is marked as RFC. > > I see at least 3 possible use-cases for this: > > 1. Gathering sw statistics for 3A on raw bayer buffers before > passing them along for a to-be-added software CPU ISP raw mode. > > 2. Gathering sw statistics for 3A on raw bayer buffers before > doing further processing on the GPU for the software GPU ISP > (possibly as an intermediate step until the stats are gathered > on the GPU too and/or as a way to compare CPU vs GPU stats > for verification purposes). > > 3. I'm working on a pipeline handler for the atomISP2, where > there is a working hw ISP but no usable statistics. Why are the stats not usable ? Is it a hardware issue, a firmware issue, or "just" that we don't have information about the statistics format ? > The plan is to gather sw statistics on the output buffers > (so on e.g. YUV images) and then re-use the 3A code from > the software ISP (I realize gathering statistics on the > pipeline output buffers is not ideal, but the atomISP driver > has no way to access the raw bayer data). It's more than non-ideal, it will make algorithms development much more painful :-( > Usage would look something like this: > > once: > > if (stats_->configure(inputCfg) != 0) > return -EINVAL; > > stats_->setWindow(Rectangle(window_.size())); > > per frame: > > stats_->startFrame(); > stats_->processFrame(inputFrameBuffer); > stats_->finishFrame(frameNo, bufferID); > > Regards, > > Hans > > > Hans de Goede (4): > libcamera: swstats_cpu: Update statsProcessFn() / processLine0() > documentation > libcamera: swstats_cpu: Drop patternSize_ documentation > libcamera: software_isp: Move benchmark code to its own class > libcamera: swstats_cpu: Add processFrame() method > > src/libcamera/software_isp/benchmark.cpp | 78 +++++++++++++++ > src/libcamera/software_isp/benchmark.h | 36 +++++++ > src/libcamera/software_isp/debayer_cpu.cpp | 32 +----- > src/libcamera/software_isp/debayer_cpu.h | 7 +- > src/libcamera/software_isp/meson.build | 1 + > src/libcamera/software_isp/swstats_cpu.cpp | 108 +++++++++++++++++---- > src/libcamera/software_isp/swstats_cpu.h | 10 ++ > 7 files changed, 216 insertions(+), 56 deletions(-) > create mode 100644 src/libcamera/software_isp/benchmark.cpp > create mode 100644 src/libcamera/software_isp/benchmark.h
Hi Laurent, On 10-Oct-24 9:45 PM, Laurent Pinchart wrote: > Hi Hans, > > On Wed, Oct 09, 2024 at 10:01:06PM +0200, Hans de Goede wrote: >> Hi All, >> >> Here is a patch series adding a processFrame() method to gather >> statistics on an entire frame in one go, rather then using the >> line by line approach which is used by the somewhat tightly coupled >> DebayerCpu class. >> >> ATM there are no users for processFrame() yet, so the code is >> compile tested only which is why it is marked as RFC. >> >> I see at least 3 possible use-cases for this: >> >> 1. Gathering sw statistics for 3A on raw bayer buffers before >> passing them along for a to-be-added software CPU ISP raw mode. >> >> 2. Gathering sw statistics for 3A on raw bayer buffers before >> doing further processing on the GPU for the software GPU ISP >> (possibly as an intermediate step until the stats are gathered >> on the GPU too and/or as a way to compare CPU vs GPU stats >> for verification purposes). >> >> 3. I'm working on a pipeline handler for the atomISP2, where >> there is a working hw ISP but no usable statistics. > > Why are the stats not usable ? Is it a hardware issue, a firmware issue, > or "just" that we don't have information about the statistics format ? There are 2 problems: 1. As you say '"just" that we don't have information about the statistics format ?' I'll email Sakari to see if he can help with at least a .h files with a C struct definition for the buffers. One thing to keep in mind here is that the hw using the atomisp is getting pretty old. So while I'm very interested in getting this going at the same time I also want to not spend too much time on this. Using the swstats_cpu code + reusing the ipa_soft_simple.so IPA is a relatively easy / low-risk way of doing this. I actually am about to post a first version of an atomisp pipeline handler using this approach to do aec/agc. AWB will follow later. First I need to find out where the atomisp kernel driver sets the ISP R/G/B gains and export those as standard V4L2 controls, rather then whatever custom ioctl it is currently using ... 2. The kernel driver does receive what it calls "3a stat" buffers but atm it simply discards these. I think there is or used to be a custom ioctl to retrieve these, but I may have ripped that out already. Regardless we need to design a proper userspace API for this. Maybe a separate /dev/video# node using videobuf2 to pass the buffers? Anyways this is something to look at if we can get help with figuring out the statistics format. So for now I have not really looked at this yet. >> The plan is to gather sw statistics on the output buffers >> (so on e.g. YUV images) and then re-use the 3A code from >> the software ISP (I realize gathering statistics on the >> pipeline output buffers is not ideal, but the atomISP driver >> has no way to access the raw bayer data). > > It's more than non-ideal, it will make algorithms development much more > painful :-( Yes this is less then ideal. But at least for basic agc + awb I think we can make this work. Actually for aec/awb it works pretty well since the ISP outputs YUV data getting a Y histogram is easy and even in the postprocessed data it is easy to see if the image is too light / dark. Regards, Hans
On Sun, Nov 03, 2024 at 01:05:29PM +0100, Hans de Goede wrote: > On 10-Oct-24 9:45 PM, Laurent Pinchart wrote: > > On Wed, Oct 09, 2024 at 10:01:06PM +0200, Hans de Goede wrote: > >> Hi All, > >> > >> Here is a patch series adding a processFrame() method to gather > >> statistics on an entire frame in one go, rather then using the > >> line by line approach which is used by the somewhat tightly coupled > >> DebayerCpu class. > >> > >> ATM there are no users for processFrame() yet, so the code is > >> compile tested only which is why it is marked as RFC. > >> > >> I see at least 3 possible use-cases for this: > >> > >> 1. Gathering sw statistics for 3A on raw bayer buffers before > >> passing them along for a to-be-added software CPU ISP raw mode. > >> > >> 2. Gathering sw statistics for 3A on raw bayer buffers before > >> doing further processing on the GPU for the software GPU ISP > >> (possibly as an intermediate step until the stats are gathered > >> on the GPU too and/or as a way to compare CPU vs GPU stats > >> for verification purposes). > >> > >> 3. I'm working on a pipeline handler for the atomISP2, where > >> there is a working hw ISP but no usable statistics. > > > > Why are the stats not usable ? Is it a hardware issue, a firmware issue, > > or "just" that we don't have information about the statistics format ? > > There are 2 problems: > > 1. As you say '"just" that we don't have information about > the statistics format ?' > > I'll email Sakari to see if he can help with at least a .h files > with a C struct definition for the buffers. > > One thing to keep in mind here is that the hw using the atomisp > is getting pretty old. So while I'm very interested in getting > this going at the same time I also want to not spend too much > time on this. I understand that, but let's not forget that the driver is in staging, and what it implies. There should be continuous effort to clean up the driver to get it out of staging, and I believe that should include things like stats support. It doesn't have to be done right away, but we don't want drivers to bitrot in staging, we have too many of those already (I'm thinking about the IPU3 ImgU driver in particular that will need work, or should be removed from the kernel if it becomes clear nobody will fix its issues). > Using the swstats_cpu code + reusing the ipa_soft_simple.so IPA > is a relatively easy / low-risk way of doing this. As a first step I have no issue about that, we don't have to make everything perfect overnight :-) > I actually am about to post a first version of an atomisp > pipeline handler using this approach to do aec/agc. > > AWB will follow later. First I need to find out where > the atomisp kernel driver sets the ISP R/G/B gains and export > those as standard V4L2 controls, rather then whatever custom > ioctl it is currently using ... > > 2. The kernel driver does receive what it calls "3a stat" buffers > but atm it simply discards these. I think there is or used to be > a custom ioctl to retrieve these, but I may have ripped that out > already. Regardless we need to design a proper userspace API for > this. Maybe a separate /dev/video# node using videobuf2 to pass > the buffers? Yes, that's what ISP drivers do. It also implies using the MC API. > Anyways this is something to look at if we can > get help with figuring out the statistics format. So for now I > have not really looked at this yet. Being able to get the stats buffers out would also mean someone could give a go at reverse engineering the format, if we need to go that route. I don't know how big the atomisp community is though, and if there could be a volunteer for this task, or if you're alone :-) > >> The plan is to gather sw statistics on the output buffers > >> (so on e.g. YUV images) and then re-use the 3A code from > >> the software ISP (I realize gathering statistics on the > >> pipeline output buffers is not ideal, but the atomISP driver > >> has no way to access the raw bayer data). > > > > It's more than non-ideal, it will make algorithms development much more > > painful :-( > > Yes this is less then ideal. But at least for basic agc + awb > I think we can make this work. > > Actually for aec/awb it works pretty well since the ISP outputs > YUV data getting a Y histogram is easy and even in > the postprocessed data it is easy to see if the image is too > light / dark.
Hi Laurent, On 6-Nov-24 12:22 PM, Laurent Pinchart wrote: > On Sun, Nov 03, 2024 at 01:05:29PM +0100, Hans de Goede wrote: >> On 10-Oct-24 9:45 PM, Laurent Pinchart wrote: >>> On Wed, Oct 09, 2024 at 10:01:06PM +0200, Hans de Goede wrote: >>>> Hi All, >>>> >>>> Here is a patch series adding a processFrame() method to gather >>>> statistics on an entire frame in one go, rather then using the >>>> line by line approach which is used by the somewhat tightly coupled >>>> DebayerCpu class. >>>> >>>> ATM there are no users for processFrame() yet, so the code is >>>> compile tested only which is why it is marked as RFC. >>>> >>>> I see at least 3 possible use-cases for this: >>>> >>>> 1. Gathering sw statistics for 3A on raw bayer buffers before >>>> passing them along for a to-be-added software CPU ISP raw mode. >>>> >>>> 2. Gathering sw statistics for 3A on raw bayer buffers before >>>> doing further processing on the GPU for the software GPU ISP >>>> (possibly as an intermediate step until the stats are gathered >>>> on the GPU too and/or as a way to compare CPU vs GPU stats >>>> for verification purposes). >>>> >>>> 3. I'm working on a pipeline handler for the atomISP2, where >>>> there is a working hw ISP but no usable statistics. >>> >>> Why are the stats not usable ? Is it a hardware issue, a firmware issue, >>> or "just" that we don't have information about the statistics format ? >> >> There are 2 problems: >> >> 1. As you say '"just" that we don't have information about >> the statistics format ?' >> >> I'll email Sakari to see if he can help with at least a .h files >> with a C struct definition for the buffers. >> >> One thing to keep in mind here is that the hw using the atomisp >> is getting pretty old. So while I'm very interested in getting >> this going at the same time I also want to not spend too much >> time on this. > > I understand that, but let's not forget that the driver is in staging, > and what it implies. There should be continuous effort to clean up the > driver to get it out of staging, and I believe that should include > things like stats support. It doesn't have to be done right away, but we > don't want drivers to bitrot in staging, we have too many of those > already (I'm thinking about the IPU3 ImgU driver in particular that will > need work, or should be removed from the kernel if it becomes clear > nobody will fix its issues). I do plan to keep supporting and improving the atomisp code. We first got it working (as in displaying any image at all) in 2021 and since then a lot has been improved / changed already. But it has been very slow going since this is a spare time side project for me. > >> Using the swstats_cpu code + reusing the ipa_soft_simple.so IPA >> is a relatively easy / low-risk way of doing this. > > As a first step I have no issue about that, we don't have to make > everything perfect overnight :-) > >> I actually am about to post a first version of an atomisp >> pipeline handler using this approach to do aec/agc. >> >> AWB will follow later. First I need to find out where >> the atomisp kernel driver sets the ISP R/G/B gains and export >> those as standard V4L2 controls, rather then whatever custom >> ioctl it is currently using ... >> >> 2. The kernel driver does receive what it calls "3a stat" buffers >> but atm it simply discards these. I think there is or used to be >> a custom ioctl to retrieve these, but I may have ripped that out >> already. Regardless we need to design a proper userspace API for >> this. Maybe a separate /dev/video# node using videobuf2 to pass >> the buffers? > > Yes, that's what ISP drivers do. It also implies using the MC API. Right, note atomisp already used the MC API. So this would mean having a second /dev/video# MC node with the ISP as the parent / source I presume. And then use videobuf2 >> Anyways this is something to look at if we can >> get help with figuring out the statistics format. So for now I >> have not really looked at this yet. > > Being able to get the stats buffers out would also mean someone could > give a go at reverse engineering the format, if we need to go that > route. I don't know how big the atomisp community is though, and if > there could be a volunteer for this task, or if you're alone :-) ATM I'm mostly alone. It seems there is some interest in getting things to work but no one has really stepped up to help. I hope that getting a minimal POC with Firefox / snapshot showing video through pipewire on these devices ready will help to gather some attention and hopefully more help. So I'm happy to read that you write "As a first step I have no issue about that, we don't have to make everything perfect overnight" which I hope means that we can merge a swstats based pipeline handler as a first step and hopefully that will help with bootstrapping some sort of community of atomisp users and contributors. Regards, Hans
On Wed, Nov 06, 2024 at 02:42:08PM +0100, Hans de Goede wrote: > On 6-Nov-24 12:22 PM, Laurent Pinchart wrote: > > On Sun, Nov 03, 2024 at 01:05:29PM +0100, Hans de Goede wrote: > >> On 10-Oct-24 9:45 PM, Laurent Pinchart wrote: > >>> On Wed, Oct 09, 2024 at 10:01:06PM +0200, Hans de Goede wrote: > >>>> Hi All, > >>>> > >>>> Here is a patch series adding a processFrame() method to gather > >>>> statistics on an entire frame in one go, rather then using the > >>>> line by line approach which is used by the somewhat tightly coupled > >>>> DebayerCpu class. > >>>> > >>>> ATM there are no users for processFrame() yet, so the code is > >>>> compile tested only which is why it is marked as RFC. > >>>> > >>>> I see at least 3 possible use-cases for this: > >>>> > >>>> 1. Gathering sw statistics for 3A on raw bayer buffers before > >>>> passing them along for a to-be-added software CPU ISP raw mode. > >>>> > >>>> 2. Gathering sw statistics for 3A on raw bayer buffers before > >>>> doing further processing on the GPU for the software GPU ISP > >>>> (possibly as an intermediate step until the stats are gathered > >>>> on the GPU too and/or as a way to compare CPU vs GPU stats > >>>> for verification purposes). > >>>> > >>>> 3. I'm working on a pipeline handler for the atomISP2, where > >>>> there is a working hw ISP but no usable statistics. > >>> > >>> Why are the stats not usable ? Is it a hardware issue, a firmware issue, > >>> or "just" that we don't have information about the statistics format ? > >> > >> There are 2 problems: > >> > >> 1. As you say '"just" that we don't have information about > >> the statistics format ?' > >> > >> I'll email Sakari to see if he can help with at least a .h files > >> with a C struct definition for the buffers. > >> > >> One thing to keep in mind here is that the hw using the atomisp > >> is getting pretty old. So while I'm very interested in getting > >> this going at the same time I also want to not spend too much > >> time on this. > > > > I understand that, but let's not forget that the driver is in staging, > > and what it implies. There should be continuous effort to clean up the > > driver to get it out of staging, and I believe that should include > > things like stats support. It doesn't have to be done right away, but we > > don't want drivers to bitrot in staging, we have too many of those > > already (I'm thinking about the IPU3 ImgU driver in particular that will > > need work, or should be removed from the kernel if it becomes clear > > nobody will fix its issues). > > I do plan to keep supporting and improving the atomisp code. We first > got it working (as in displaying any image at all) in 2021 and since > then a lot has been improved / changed already. > > But it has been very slow going since this is a spare time side > project for me. Thanks for the confirmation, and for the work you're doing there. Slow is absolutely fine. > >> Using the swstats_cpu code + reusing the ipa_soft_simple.so IPA > >> is a relatively easy / low-risk way of doing this. > > > > As a first step I have no issue about that, we don't have to make > > everything perfect overnight :-) > > > >> I actually am about to post a first version of an atomisp > >> pipeline handler using this approach to do aec/agc. > >> > >> AWB will follow later. First I need to find out where > >> the atomisp kernel driver sets the ISP R/G/B gains and export > >> those as standard V4L2 controls, rather then whatever custom > >> ioctl it is currently using ... > >> > >> 2. The kernel driver does receive what it calls "3a stat" buffers > >> but atm it simply discards these. I think there is or used to be > >> a custom ioctl to retrieve these, but I may have ripped that out > >> already. Regardless we need to design a proper userspace API for > >> this. Maybe a separate /dev/video# node using videobuf2 to pass > >> the buffers? > > > > Yes, that's what ISP drivers do. It also implies using the MC API. > > Right, note atomisp already used the MC API. So this would mean > having a second /dev/video# MC node with the ISP as the parent / > source I presume. Yes that's correct. And eventually there should be a third video node, to pass parameters to the ISP. > And then use videobuf2 > > >> Anyways this is something to look at if we can > >> get help with figuring out the statistics format. So for now I > >> have not really looked at this yet. > > > > Being able to get the stats buffers out would also mean someone could > > give a go at reverse engineering the format, if we need to go that > > route. I don't know how big the atomisp community is though, and if > > there could be a volunteer for this task, or if you're alone :-) > > ATM I'm mostly alone. It seems there is some interest in getting > things to work but no one has really stepped up to help. > > I hope that getting a minimal POC with Firefox / snapshot > showing video through pipewire on these devices ready will help > to gather some attention and hopefully more help. > > So I'm happy to read that you write "As a first step I have no > issue about that, we don't have to make everything perfect overnight" > > which I hope means that we can merge a swstats based pipeline > handler as a first step and hopefully that will help with > bootstrapping some sort of community of atomisp users and > contributors. Yes, I'm fine with that. I trust that you will work with the other swisp developers to make sure you won't step on each other's toes, now that there will be multiple users of the code.
Hi All, Here is a patch series adding a processFrame() method to gather statistics on an entire frame in one go, rather then using the line by line approach which is used by the somewhat tightly coupled DebayerCpu class. ATM there are no users for processFrame() yet, so the code is compile tested only which is why it is marked as RFC. I see at least 3 possible use-cases for this: 1. Gathering sw statistics for 3A on raw bayer buffers before passing them along for a to-be-added software CPU ISP raw mode. 2. Gathering sw statistics for 3A on raw bayer buffers before doing further processing on the GPU for the software GPU ISP (possibly as an intermediate step until the stats are gathered on the GPU too and/or as a way to compare CPU vs GPU stats for verification purposes). 3. I'm working on a pipeline handler for the atomISP2, where there is a working hw ISP but no usable statistics. The plan is to gather sw statistics on the output buffers (so on e.g. YUV images) and then re-use the 3A code from the software ISP (I realize gathering statistics on the pipeline output buffers is not ideal, but the atomISP driver has no way to access the raw bayer data). Usage would look something like this: once: if (stats_->configure(inputCfg) != 0) return -EINVAL; stats_->setWindow(Rectangle(window_.size())); per frame: stats_->startFrame(); stats_->processFrame(inputFrameBuffer); stats_->finishFrame(frameNo, bufferID); Regards, Hans Hans de Goede (4): libcamera: swstats_cpu: Update statsProcessFn() / processLine0() documentation libcamera: swstats_cpu: Drop patternSize_ documentation libcamera: software_isp: Move benchmark code to its own class libcamera: swstats_cpu: Add processFrame() method src/libcamera/software_isp/benchmark.cpp | 78 +++++++++++++++ src/libcamera/software_isp/benchmark.h | 36 +++++++ src/libcamera/software_isp/debayer_cpu.cpp | 32 +----- src/libcamera/software_isp/debayer_cpu.h | 7 +- src/libcamera/software_isp/meson.build | 1 + src/libcamera/software_isp/swstats_cpu.cpp | 108 +++++++++++++++++---- src/libcamera/software_isp/swstats_cpu.h | 10 ++ 7 files changed, 216 insertions(+), 56 deletions(-) create mode 100644 src/libcamera/software_isp/benchmark.cpp create mode 100644 src/libcamera/software_isp/benchmark.h