| Message ID | 20210310172348.4312-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David, On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote: > Hi > > I'm just submitting this patch for comments in the first instance > (mostly from Naush, I guess, but everyone is welcome!). It's part of > our plan for more flexible handling of metadata from the sensor. > > (The background is that we have some interesting sensors coming up > that give us other forms of embedded data, not just register > dumps, and we need to be able to deal with those!) > > The plan is to give our CamHelpers a Prepare() and a Process() method, > just like all our algorithms. As usual, Prepare() runs just before the > ISP starts, Process() just after. A version of Prepare() is provided > that has basically just sucked that little bit of > register-dump-parsing functionality out of the IPA file > (raspberrypi.cpp). Process() does nothing by default. > > There aren't actually many changes, but some observations on what I've > done: > > * I've not updated various CamHelper comments yet, that can wait! > > * I've made the Prepare() method responsible for reading the delayed > control values if we can't use the metadata to get the > exposure/gain. I wonder if perhaps that is better left in the > IPA. Prepare() might indicate via a return value whether it found > them in the embedded data or not. > > * The parser object is completely hidden behind the helper now, so the > distinction between them is rather blurring. Maybe they could be > combined, but that can happen in a later patch. I'm trying to figure out how this will play along with the refactoring of camera sensor helpers. There are a few points to consider: - The helpers are not specific to the Raspberry Pi IPA, and should thus be moved to a common location. This is more or less a mechanical change and shouldn't cause any issue by itself. The code could be moved to libipa for instance. - We have sensor-specific data on the pipeline handler side, provided by the CameraSensor class and passed to the IPA through CameraSensorInfo. All the information is currently retrieved from the kernel driver. Work is planned to create a sensor database that will allow hardcoding sensor-specific information in libcamera. I'd like to see if we could centralize all sensor-specific information on the pipeline handler side, or least for static data. This would cover information related to delayed controls, which is currently provided by the IPA but not used by it, but also information about mistrusted frames, that would then be passed to the IPA by the pipeline handler. - There's a need for sensor-specific code (as opposed to data), currently used on the IPA side. There's room for refactoring this though, including replacing the virtual Gain and GainCode functions with parametric formulas. For instance, the CCS specification computes the sensor gain as either - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register value, m0, m1, c0 and c1 being static parameters, and either m0 or m1 being equal to 0 ; or - gain = a * 2^x, with a and x being register values. It seems this wouldn't match the IMX290, but we could add additional formulas. - For embedded data parsing, I wonder if it would be best handled in the IPA or in the pipeline handler. The latter would have the advantage of gathering more sensor-specific code on the pipeline handler side, as well as not having to involve the IPA in the parsing for other types of ancillary data produced by the sensors. > David Plowman (1): > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing > > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++ > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++- > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++--------------------- > 3 files changed, 84 insertions(+), 67 deletions(-)
Hi David and Laurent, On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi David, > > On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote: > > Hi > > > > I'm just submitting this patch for comments in the first instance > > (mostly from Naush, I guess, but everyone is welcome!). It's part of > > our plan for more flexible handling of metadata from the sensor. > > > > (The background is that we have some interesting sensors coming up > > that give us other forms of embedded data, not just register > > dumps, and we need to be able to deal with those!) > > > > The plan is to give our CamHelpers a Prepare() and a Process() method, > > just like all our algorithms. As usual, Prepare() runs just before the > > ISP starts, Process() just after. A version of Prepare() is provided > > that has basically just sucked that little bit of > > register-dump-parsing functionality out of the IPA file > > (raspberrypi.cpp). Process() does nothing by default. > > > > There aren't actually many changes, but some observations on what I've > > done: > > > > * I've not updated various CamHelper comments yet, that can wait! > > > > * I've made the Prepare() method responsible for reading the delayed > > control values if we can't use the metadata to get the > > exposure/gain. I wonder if perhaps that is better left in the > > IPA. Prepare() might indicate via a return value whether it found > > them in the embedded data or not. > > > > * The parser object is completely hidden behind the helper now, so the > > distinction between them is rather blurring. Maybe they could be > > combined, but that can happen in a later patch. > > I'm trying to figure out how this will play along with the refactoring > of camera sensor helpers. There are a few points to consider: > > - The helpers are not specific to the Raspberry Pi IPA, and should thus > be moved to a common location. This is more or less a mechanical > change and shouldn't cause any issue by itself. The code could be > moved to libipa for instance. > > - We have sensor-specific data on the pipeline handler side, provided by > the CameraSensor class and passed to the IPA through CameraSensorInfo. > All the information is currently retrieved from the kernel driver. > Work is planned to create a sensor database that will allow hardcoding > sensor-specific information in libcamera. > Would the sensor database be a drop in replacement for the CamHelper, or do you think we will still have a CamHelper doing certain operations and the sensor database is for static sensor properties? > > I'd like to see if we could centralize all sensor-specific information > on the pipeline handler side, or least for static data. This would > cover information related to delayed controls, which is currently > provided by the IPA but not used by it, but also information about > mistrusted frames, that would then be passed to the IPA by the > pipeline handler. > This does seem like the right approach to me. > > - There's a need for sensor-specific code (as opposed to data), > currently used on the IPA side. There's room for refactoring this > though, including replacing the virtual Gain and GainCode functions > with parametric formulas. For instance, the CCS specification computes > the sensor gain as either > > - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register > value, m0, m1, c0 and c1 being static parameters, and either m0 or > m1 being equal to 0 ; or > > - gain = a * 2^x, with a and x being register values. > > It seems this wouldn't match the IMX290, but we could add additional > formulas. > I think allowing this to remain virtual might still be needed. Some sensors have entirely tables based gain -> code translations, so we should not restrict to only parametric formulas. Similar comment for exposure lines -> time calculations. Some sensors I've encountered (global shutter and ultra long exposure modes) do not follow the typical line length * frame_length approach. > > - For embedded data parsing, I wonder if it would be best handled in the > IPA or in the pipeline handler. The latter would have the advantage of > gathering more sensor-specific code on the pipeline handler side, as > well as not having to involve the IPA in the parsing for other types > of ancillary data produced by the sensors. > Having the embedded data parsing in the pipeline handler does seem like a reasonable thing to do. However, we must keep in mind that there may be vendor specific data in there (e.g. focus pixel stats) that will need to be passed into the IPA. This may happen through named controls, or some other opaque way. Regards, Naush > > > David Plowman (1): > > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing > > > > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++ > > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++- > > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++--------------------- > > 3 files changed, 84 insertions(+), 67 deletions(-) > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Naush, Laurent Thanks for the replies on this subject. In fact I'm still fiddling around, and discovering some new problems, so there's certainly still some way to go! One new thing I'm wrestling with at the moment is a sensor where the "usual" relationship between binned and full res modes, in respect of noise levels, doesn't hold. I suppose this is all information you could encode in a database, though I wonder whether it might become quite burdensome. As things stand currently, I'm thinking about making CamHelper::SetCameraMode virtual, so that we can amend the mode for certain sensors. The CameraMode that we pass to our SwitchMode methods would have to come from the helper. On some of the other questions: On Tue, 16 Mar 2021 at 10:33, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi David and Laurent, > > On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > >> Hi David, >> >> On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote: >> > Hi >> > >> > I'm just submitting this patch for comments in the first instance >> > (mostly from Naush, I guess, but everyone is welcome!). It's part of >> > our plan for more flexible handling of metadata from the sensor. >> > >> > (The background is that we have some interesting sensors coming up >> > that give us other forms of embedded data, not just register >> > dumps, and we need to be able to deal with those!) >> > >> > The plan is to give our CamHelpers a Prepare() and a Process() method, >> > just like all our algorithms. As usual, Prepare() runs just before the >> > ISP starts, Process() just after. A version of Prepare() is provided >> > that has basically just sucked that little bit of >> > register-dump-parsing functionality out of the IPA file >> > (raspberrypi.cpp). Process() does nothing by default. >> > >> > There aren't actually many changes, but some observations on what I've >> > done: >> > >> > * I've not updated various CamHelper comments yet, that can wait! >> > >> > * I've made the Prepare() method responsible for reading the delayed >> > control values if we can't use the metadata to get the >> > exposure/gain. I wonder if perhaps that is better left in the >> > IPA. Prepare() might indicate via a return value whether it found >> > them in the embedded data or not. >> > I don't like what I did here. I'd prefer to move the code that fetches values from the controls out of the helper into the IPA, that is, src/ipa/raspberrypi/raspberrypi.cpp. > > >> > * The parser object is completely hidden behind the helper now, so the >> > distinction between them is rather blurring. Maybe they could be >> > combined, but that can happen in a later patch. > > >> I'm trying to figure out how this will play along with the refactoring >> of camera sensor helpers. There are a few points to consider: >> >> - The helpers are not specific to the Raspberry Pi IPA, and should thus >> be moved to a common location. This is more or less a mechanical >> change and shouldn't cause any issue by itself. The code could be >> moved to libipa for instance. >> >> - We have sensor-specific data on the pipeline handler side, provided by >> the CameraSensor class and passed to the IPA through CameraSensorInfo. >> All the information is currently retrieved from the kernel driver. >> Work is planned to create a sensor database that will allow hardcoding >> sensor-specific information in libcamera. >> > > Would the sensor database be a drop in replacement for the CamHelper, > or do you think we will still have a CamHelper doing certain operations and > the sensor database is for static sensor properties? > I suspect a database is not enough. More below... > > >> >> I'd like to see if we could centralize all sensor-specific information >> on the pipeline handler side, or least for static data. This would >> cover information related to delayed controls, which is currently >> provided by the IPA but not used by it, but also information about >> mistrusted frames, that would then be passed to the IPA by the >> pipeline handler. >> > > This does seem like the right approach to me. > > >> >> - There's a need for sensor-specific code (as opposed to data), >> currently used on the IPA side. There's room for refactoring this >> though, including replacing the virtual Gain and GainCode functions >> with parametric formulas. For instance, the CCS specification computes >> the sensor gain as either >> >> - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register >> value, m0, m1, c0 and c1 being static parameters, and either m0 or >> m1 being equal to 0 ; or >> >> - gain = a * 2^x, with a and x being register values. >> >> It seems this wouldn't match the IMX290, but we could add additional >> formulas. >> > > I think allowing this to remain virtual might still be needed. > > Some sensors have entirely tables based gain -> code translations, so > we should not restrict to only parametric formulas. Similar comment for > exposure lines -> time calculations. Some sensors I've encountered > (global shutter and ultra long exposure modes) do not follow the typical > line length * frame_length approach. > Agree. I have a sensor where some modes can't have a gain less than (in this case) 1.12x. I'm "hiding" that in the CamHelper methods. Also the binned mode produces 2x the signal level of the full res mode. This too I'm "hiding" in the CamHelper methods (though it doesn't feel like the best solution.) There seem to be quite a few "awkward" devices out there! > > >> >> - For embedded data parsing, I wonder if it would be best handled in the >> IPA or in the pipeline handler. The latter would have the advantage of >> gathering more sensor-specific code on the pipeline handler side, as >> well as not having to involve the IPA in the parsing for other types >> of ancillary data produced by the sensors. >> > > Having the embedded data parsing in the pipeline handler does seem > like a reasonable thing to do. However, we must keep in mind that > there may be vendor specific data in there (e.g. focus pixel stats) that > will need to be passed into the IPA. This may happen through named > controls, or some other opaque way. > I'd be OK to see embedded data parsing move out of the helper, though I think that being able to cope with other types of metadata is a powerful feature. It might be focus pixels, or other kinds of image analysis. In general I would expect the CamHelper::Prepare() method to put this information into the Raspberry Pi metadata object. From there, an algorithm would be loaded by being listed in the json file, which would retrieve the item from the Raspberry Pi metadata and handle it. I'm thinking I should perhaps submit another version that encapsulates my latest thoughts on what I think I need - then we can consider what might benefit from being done differently. Thanks! David > > Regards, > Naush > > >> >> > David Plowman (1): >> > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing >> > >> > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++ >> > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++- >> > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++--------------------- >> > 3 files changed, 84 insertions(+), 67 deletions(-) >> >> -- >> Regards, >> >> Laurent Pinchart >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Naush, On Tue, Mar 16, 2021 at 10:33:40AM +0000, Naushir Patuck wrote: > On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart wrote: > > On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote: > > > Hi > > > > > > I'm just submitting this patch for comments in the first instance > > > (mostly from Naush, I guess, but everyone is welcome!). It's part of > > > our plan for more flexible handling of metadata from the sensor. > > > > > > (The background is that we have some interesting sensors coming up > > > that give us other forms of embedded data, not just register > > > dumps, and we need to be able to deal with those!) > > > > > > The plan is to give our CamHelpers a Prepare() and a Process() method, > > > just like all our algorithms. As usual, Prepare() runs just before the > > > ISP starts, Process() just after. A version of Prepare() is provided > > > that has basically just sucked that little bit of > > > register-dump-parsing functionality out of the IPA file > > > (raspberrypi.cpp). Process() does nothing by default. > > > > > > There aren't actually many changes, but some observations on what I've > > > done: > > > > > > * I've not updated various CamHelper comments yet, that can wait! > > > > > > * I've made the Prepare() method responsible for reading the delayed > > > control values if we can't use the metadata to get the > > > exposure/gain. I wonder if perhaps that is better left in the > > > IPA. Prepare() might indicate via a return value whether it found > > > them in the embedded data or not. > > > > > > * The parser object is completely hidden behind the helper now, so the > > > distinction between them is rather blurring. Maybe they could be > > > combined, but that can happen in a later patch. > > > > I'm trying to figure out how this will play along with the refactoring > > of camera sensor helpers. There are a few points to consider: > > > > - The helpers are not specific to the Raspberry Pi IPA, and should thus > > be moved to a common location. This is more or less a mechanical > > change and shouldn't cause any issue by itself. The code could be > > moved to libipa for instance. > > > > - We have sensor-specific data on the pipeline handler side, provided by > > the CameraSensor class and passed to the IPA through CameraSensorInfo. > > All the information is currently retrieved from the kernel driver. > > Work is planned to create a sensor database that will allow hardcoding > > sensor-specific information in libcamera. > > Would the sensor database be a drop in replacement for the CamHelper, > or do you think we will still have a CamHelper doing certain operations and > the sensor database is for static sensor properties? The sensor database will certainly contain static information. Generally speaking, the more we can use static data, the better. We can then have a cam helper class that would use the static data to perform common tasks, such as converting exposure time and gain between sensor-specific units and standard units. Those functions wouldn't need to be virtual anymore. I believe we'll still need virtual functions though, for things like embedded data parsing at least, and possibly other operations, as not everything can be expressed as data (well, the logic of a function can be expressed as data, in the worst case as assembly instructions, but that's going too far :-) - jokes aside, where to draw the line is an interesting question). > > I'd like to see if we could centralize all sensor-specific information > > on the pipeline handler side, or least for static data. This would > > cover information related to delayed controls, which is currently > > provided by the IPA but not used by it, but also information about > > mistrusted frames, that would then be passed to the IPA by the > > pipeline handler. > > This does seem like the right approach to me. > > > - There's a need for sensor-specific code (as opposed to data), > > currently used on the IPA side. There's room for refactoring this > > though, including replacing the virtual Gain and GainCode functions > > with parametric formulas. For instance, the CCS specification computes > > the sensor gain as either > > > > - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register > > value, m0, m1, c0 and c1 being static parameters, and either m0 or > > m1 being equal to 0 ; or > > > > - gain = a * 2^x, with a and x being register values. > > > > It seems this wouldn't match the IMX290, but we could add additional > > formulas. > > I think allowing this to remain virtual might still be needed. > > Some sensors have entirely tables based gain -> code translations, so > we should not restrict to only parametric formulas. :-( Is this common ? The tables could be stored in the sensor database though. > Similar comment for > exposure lines -> time calculations. Some sensors I've encountered > (global shutter and ultra long exposure modes) do not follow the typical > line length * frame_length approach. Do they need very device-specific conversion, or are there a set of common conversions that would cover all our needs ? > > - For embedded data parsing, I wonder if it would be best handled in the > > IPA or in the pipeline handler. The latter would have the advantage of > > gathering more sensor-specific code on the pipeline handler side, as > > well as not having to involve the IPA in the parsing for other types > > of ancillary data produced by the sensors. > > Having the embedded data parsing in the pipeline handler does seem > like a reasonable thing to do. However, we must keep in mind that > there may be vendor specific data in there (e.g. focus pixel stats) that > will need to be passed into the IPA. This may happen through named > controls, or some other opaque way. Good point. When sensors produce raw PDAF statistics, are they transmitted as part of the "regular" embedded data, or separately ? > > > David Plowman (1): > > > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing > > > > > > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++ > > > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++- > > > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++--------------------- > > > 3 files changed, 84 insertions(+), 67 deletions(-)
Hi David, On Tue, Mar 16, 2021 at 04:40:41PM +0000, David Plowman wrote: > Hi Naush, Laurent > > Thanks for the replies on this subject. In fact I'm still fiddling around, > and discovering some new problems, so there's certainly still some way to > go! > > One new thing I'm wrestling with at the moment is a sensor where the > "usual" relationship between binned and full res modes, in respect of noise > levels, doesn't hold. I suppose this is all information you could encode in > a database, though I wonder whether it might become quite burdensome. It has to be encoded somewhere, right ? I don't know the details for this specific sensor, but from your description I don't think we could/should convey that information through V4L2, so it would need to be stored somewhere in userspace. > As things stand currently, I'm thinking about making > CamHelper::SetCameraMode virtual, so that we can amend the mode for certain > sensors. The CameraMode that we pass to our SwitchMode methods would have > to come from the helper. Following our recent discussions on the topic of virtual functions, I'm not completely opposed to them as they have valid use cases, but I also believe that most sensor features could be described with static data in most cases. Static data has the advantage of providing more information than "I've mangled the parameters you gave me, here's the result" to the IPA, possibly enabling it to make more clever choices. Maybe a good middle ground would be to make these camera helper functions virtual for corner cases, with a base implementation that works for most sensors, based on static data. > On some of the other questions: > > On Tue, 16 Mar 2021 at 10:33, Naushir Patuck wrote: > > On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart wrote: > >> On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote: > >> > Hi > >> > > >> > I'm just submitting this patch for comments in the first instance > >> > (mostly from Naush, I guess, but everyone is welcome!). It's part of > >> > our plan for more flexible handling of metadata from the sensor. > >> > > >> > (The background is that we have some interesting sensors coming up > >> > that give us other forms of embedded data, not just register > >> > dumps, and we need to be able to deal with those!) > >> > > >> > The plan is to give our CamHelpers a Prepare() and a Process() method, > >> > just like all our algorithms. As usual, Prepare() runs just before the > >> > ISP starts, Process() just after. A version of Prepare() is provided > >> > that has basically just sucked that little bit of > >> > register-dump-parsing functionality out of the IPA file > >> > (raspberrypi.cpp). Process() does nothing by default. > >> > > >> > There aren't actually many changes, but some observations on what I've > >> > done: > >> > > >> > * I've not updated various CamHelper comments yet, that can wait! > >> > > >> > * I've made the Prepare() method responsible for reading the delayed > >> > control values if we can't use the metadata to get the > >> > exposure/gain. I wonder if perhaps that is better left in the > >> > IPA. Prepare() might indicate via a return value whether it found > >> > them in the embedded data or not. > > I don't like what I did here. I'd prefer to move the code that fetches > values from the controls out of the helper into the IPA, that is, > src/ipa/raspberrypi/raspberrypi.cpp. Sounds good to me. > >> > * The parser object is completely hidden behind the helper now, so the > >> > distinction between them is rather blurring. Maybe they could be > >> > combined, but that can happen in a later patch. > >> > >> I'm trying to figure out how this will play along with the refactoring > >> of camera sensor helpers. There are a few points to consider: > >> > >> - The helpers are not specific to the Raspberry Pi IPA, and should thus > >> be moved to a common location. This is more or less a mechanical > >> change and shouldn't cause any issue by itself. The code could be > >> moved to libipa for instance. > >> > >> - We have sensor-specific data on the pipeline handler side, provided by > >> the CameraSensor class and passed to the IPA through CameraSensorInfo. > >> All the information is currently retrieved from the kernel driver. > >> Work is planned to create a sensor database that will allow hardcoding > >> sensor-specific information in libcamera. > > > > Would the sensor database be a drop in replacement for the CamHelper, > > or do you think we will still have a CamHelper doing certain operations and > > the sensor database is for static sensor properties? > > I suspect a database is not enough. More below... > > >> I'd like to see if we could centralize all sensor-specific information > >> on the pipeline handler side, or least for static data. This would > >> cover information related to delayed controls, which is currently > >> provided by the IPA but not used by it, but also information about > >> mistrusted frames, that would then be passed to the IPA by the > >> pipeline handler. > > > > This does seem like the right approach to me. > > > >> - There's a need for sensor-specific code (as opposed to data), > >> currently used on the IPA side. There's room for refactoring this > >> though, including replacing the virtual Gain and GainCode functions > >> with parametric formulas. For instance, the CCS specification computes > >> the sensor gain as either > >> > >> - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register > >> value, m0, m1, c0 and c1 being static parameters, and either m0 or > >> m1 being equal to 0 ; or > >> > >> - gain = a * 2^x, with a and x being register values. > >> > >> It seems this wouldn't match the IMX290, but we could add additional > >> formulas. > > > > I think allowing this to remain virtual might still be needed. > > > > Some sensors have entirely tables based gain -> code translations, so > > we should not restrict to only parametric formulas. Similar comment for > > exposure lines -> time calculations. Some sensors I've encountered > > (global shutter and ultra long exposure modes) do not follow the typical > > line length * frame_length approach. > > Agree. I have a sensor where some modes can't have a gain less than (in > this case) 1.12x. I'm "hiding" that in the CamHelper methods. Also the > binned mode produces 2x the signal level of the full res mode. This too I'm > "hiding" in the CamHelper methods (though it doesn't feel like the best > solution.) There seem to be quite a few "awkward" devices out there! That seems a reasonable place to handle that, regarless of whether we express the information as code, or as data used by helper functions. I believe we'll end up merging the camera helpers and the camera sensor database into a single entity at some point. > >> - For embedded data parsing, I wonder if it would be best handled in the > >> IPA or in the pipeline handler. The latter would have the advantage of > >> gathering more sensor-specific code on the pipeline handler side, as > >> well as not having to involve the IPA in the parsing for other types > >> of ancillary data produced by the sensors. > > > > Having the embedded data parsing in the pipeline handler does seem > > like a reasonable thing to do. However, we must keep in mind that > > there may be vendor specific data in there (e.g. focus pixel stats) that > > will need to be passed into the IPA. This may happen through named > > controls, or some other opaque way. > > I'd be OK to see embedded data parsing move out of the helper, though I > think that being able to cope with other types of metadata is a powerful > feature. It might be focus pixels, or other kinds of image analysis. In > general I would expect the CamHelper::Prepare() method to put this > information into the Raspberry Pi metadata object. From there, an algorithm > would be loaded by being listed in the json file, which would retrieve the > item from the Raspberry Pi metadata and handle it. > > I'm thinking I should perhaps submit another version that encapsulates my > latest thoughts on what I think I need - then we can consider what might > benefit from being done differently. Sounds good to me too. We probably want to experiment anyway, as there are quite a few new features and use cases here. We don't need to aim for perfection from day one :-) Please consider my comments as part of the brainstorming effort, I'm particularly interested in knowing what you think makes sense, either right away or as a future enhancement, and what wouldn't be a good idea. > >> > David Plowman (1): > >> > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing > >> > > >> > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++ > >> > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++- > >> > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++--------------------- > >> > 3 files changed, 84 insertions(+), 67 deletions(-)
Hi Laurent, On Tue, 4 May 2021 at 13:45, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Tue, Mar 16, 2021 at 10:33:40AM +0000, Naushir Patuck wrote: > > On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart wrote: > > > On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote: > > > > Hi > > > > > > > > I'm just submitting this patch for comments in the first instance > > > > (mostly from Naush, I guess, but everyone is welcome!). It's part of > > > > our plan for more flexible handling of metadata from the sensor. > > > > > > > > (The background is that we have some interesting sensors coming up > > > > that give us other forms of embedded data, not just register > > > > dumps, and we need to be able to deal with those!) > > > > > > > > The plan is to give our CamHelpers a Prepare() and a Process() > method, > > > > just like all our algorithms. As usual, Prepare() runs just before > the > > > > ISP starts, Process() just after. A version of Prepare() is provided > > > > that has basically just sucked that little bit of > > > > register-dump-parsing functionality out of the IPA file > > > > (raspberrypi.cpp). Process() does nothing by default. > > > > > > > > There aren't actually many changes, but some observations on what > I've > > > > done: > > > > > > > > * I've not updated various CamHelper comments yet, that can wait! > > > > > > > > * I've made the Prepare() method responsible for reading the delayed > > > > control values if we can't use the metadata to get the > > > > exposure/gain. I wonder if perhaps that is better left in the > > > > IPA. Prepare() might indicate via a return value whether it found > > > > them in the embedded data or not. > > > > > > > > * The parser object is completely hidden behind the helper now, so > the > > > > distinction between them is rather blurring. Maybe they could be > > > > combined, but that can happen in a later patch. > > > > > > I'm trying to figure out how this will play along with the refactoring > > > of camera sensor helpers. There are a few points to consider: > > > > > > - The helpers are not specific to the Raspberry Pi IPA, and should thus > > > be moved to a common location. This is more or less a mechanical > > > change and shouldn't cause any issue by itself. The code could be > > > moved to libipa for instance. > > > > > > - We have sensor-specific data on the pipeline handler side, provided > by > > > the CameraSensor class and passed to the IPA through > CameraSensorInfo. > > > All the information is currently retrieved from the kernel driver. > > > Work is planned to create a sensor database that will allow > hardcoding > > > sensor-specific information in libcamera. > > > > Would the sensor database be a drop in replacement for the CamHelper, > > or do you think we will still have a CamHelper doing certain operations > and > > the sensor database is for static sensor properties? > > The sensor database will certainly contain static information. Generally > speaking, the more we can use static data, the better. We can then have > a cam helper class that would use the static data to perform common > tasks, such as converting exposure time and gain between sensor-specific > units and standard units. Those functions wouldn't need to be virtual > anymore. > > I believe we'll still need virtual functions though, for things like > embedded data parsing at least, and possibly other operations, as not > everything can be expressed as data (well, the logic of a function can > be expressed as data, in the worst case as assembly instructions, but > that's going too far :-) - jokes aside, where to draw the line is an > interesting question). > Yes, this is a tricky one. I suspect we will just need to add support for a whole bunch of sensors slowly, and a logical partition may emerge. > > > > I'd like to see if we could centralize all sensor-specific > information > > > on the pipeline handler side, or least for static data. This would > > > cover information related to delayed controls, which is currently > > > provided by the IPA but not used by it, but also information about > > > mistrusted frames, that would then be passed to the IPA by the > > > pipeline handler. > > > > This does seem like the right approach to me. > > > > > - There's a need for sensor-specific code (as opposed to data), > > > currently used on the IPA side. There's room for refactoring this > > > though, including replacing the virtual Gain and GainCode functions > > > with parametric formulas. For instance, the CCS specification > computes > > > the sensor gain as either > > > > > > - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register > > > value, m0, m1, c0 and c1 being static parameters, and either m0 or > > > m1 being equal to 0 ; or > > > > > > - gain = a * 2^x, with a and x being register values. > > > > > > It seems this wouldn't match the IMX290, but we could add additional > > > formulas. > > > > I think allowing this to remain virtual might still be needed. > > > > Some sensors have entirely tables based gain -> code translations, so > > we should not restrict to only parametric formulas. > > :-( Is this common ? The tables could be stored in the sensor database > though. > I would not say it is common, but I have encountered a few over the years. In these cases, storing a table in the database should not be too bothersome I would hope. > > > Similar comment for > > exposure lines -> time calculations. Some sensors I've encountered > > (global shutter and ultra long exposure modes) do not follow the typical > > line length * frame_length approach. > > Do they need very device-specific conversion, or are there a set of > common conversions that would cover all our needs ? > Again, this is not common, but I have seen device specific conversions that may be required. Unfortunately, this may not even be vendor specific. > > > > - For embedded data parsing, I wonder if it would be best handled in > the > > > IPA or in the pipeline handler. The latter would have the advantage > of > > > gathering more sensor-specific code on the pipeline handler side, as > > > well as not having to involve the IPA in the parsing for other types > > > of ancillary data produced by the sensors. > > > > Having the embedded data parsing in the pipeline handler does seem > > like a reasonable thing to do. However, we must keep in mind that > > there may be vendor specific data in there (e.g. focus pixel stats) that > > will need to be passed into the IPA. This may happen through named > > controls, or some other opaque way. > > Good point. When sensors produce raw PDAF statistics, are they > transmitted as part of the "regular" embedded data, or separately ? > From my very little experience with such sensors, it comes as embedded data - the register set itself includes pdaf statistics. I do know one other vendor that can provide an entirely separate stream for such statistics, but our Unciam block does not support more than 2 streams. Regards, Naush > > > > David Plowman (1): > > > > ipa: raspberrypi: Use CamHelpers to generalise embedded data > parsing > > > > > > > > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++ > > > > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++- > > > > src/ipa/raspberrypi/raspberrypi.cpp | 88 > ++++++++--------------------- > > > > 3 files changed, 84 insertions(+), 67 deletions(-) > > -- > Regards, > > Laurent Pinchart >