| Message ID | 20221103104027.4197-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David On Thu, Nov 03, 2022 at 10:40:25AM +0000, David Plowman via libcamera-devel wrote: > Hi everyone > > (Warning: may affect all pipeline handlers!) > > I wanted to start the ball rolling on this. The problem is that when > we enumerate the cameras in the system, we try to set some of the > controls (specifically the HFLIP and VFLIP ones, and also later > HBLANK). But this will fail if we don't "own" the camera, for example, > it is already in use elsewhere. Feels like I'm missing one point here.. How do you get to CameraSensor::init() if your pipeline handler acquires the unicam media device at the very beginning of match() ? I understand we allow multiple application acquire the same PH as the ph might register multiple cameras, but why is this a problem at match() time ? > > These patches address just the flip bits and not the HBLANK, though I > think the latter is a more straightforward problem. The flip bits are > troublesome because they affect the reported Bayer orders. > > So here the idea is to store the formats in the sensor's native Bayer > order, as we were doing before. Only we can't reset the flip bits now, > we have to query them and transform any Bayer formats the sensor gives > us back into the native order. Two patches implement this: > > 1. The first patch adds a function to turn BayerFormats back into mbus > codes. I couldn't see that we had functions to do this already, but > please correct me if I missed something! > > 2. The second patch queries the flip bits and transforms any > BayerFormats back into the native order to be stored. > > As I said, this potentially affects all pipeline handlers, which is > why I left it "RFC" for the moment. I did take a look through them all > to see what we might need to do: > > raspberrypi.cpp > > I think this PH is OK. We assume we're given the native Bayer order > and transform it according to the final sensor transform, setting the > flip bits every time. We update the format in any raw stream to match. > > uvcvideo.cpp, simple.cpp > > These appear to hardcode the transform to the Identity. I think mostly > they should be OK because the formats here won't be raw and you can't > have raw streams (?). Probably they should clear the flip bits to zero > where these controls exist, perhaps someone who knows more about them > could comment. > > rkisp1.cpp: > > Seems to force the transform to the Identity as well. So I think this > is like the above ones, it should be OK though it probably needs to > clear the sensor flip bits as well. > > ipu3.cpp: > > This one does handle transforms and appears to set the flip bits, so > that's good. I couldn't see it transforming the Bayer order for raw > streams correctly, so that probably wants checking. (To be fair, this > looks suspect already and is not made worse by these changes!) > > Thoughts and opinions gratefully received, as always. > > Thanks! > David > > David Plowman (2): > libcamera: bayer_format: Add toMbusCode method > libcamera: camera_sensor: Do not clear camera flips when listing > formats > > include/libcamera/internal/bayer_format.h | 1 + > src/libcamera/bayer_format.cpp | 12 ++++++ > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++---- > 3 files changed, 56 insertions(+), 8 deletions(-) > > -- > 2.30.2 >
Hi Jacopo, On Thu, 3 Nov 2022 at 11:50, Jacopo Mondi via libcamera-devel < libcamera-devel@lists.libcamera.org> wrote: > Hi David > > On Thu, Nov 03, 2022 at 10:40:25AM +0000, David Plowman via > libcamera-devel wrote: > > Hi everyone > > > > (Warning: may affect all pipeline handlers!) > > > > I wanted to start the ball rolling on this. The problem is that when > > we enumerate the cameras in the system, we try to set some of the > > controls (specifically the HFLIP and VFLIP ones, and also later > > HBLANK). But this will fail if we don't "own" the camera, for example, > > it is already in use elsewhere. > > Feels like I'm missing one point here.. > > How do you get to CameraSensor::init() if your pipeline handler > acquires the unicam media device at the very beginning of match() ? > > I understand we allow multiple application acquire the same PH as the > ph might register multiple cameras, but why is this a problem at > match() time ? > In match(), the rpi, rkisp and ipu3 pipeline handlers call CameraSensor::init() as part of the Camera object registration. This call is unconditional assuming we can enumerate and register the sensor. Not sure if that answers your question though? Naush > > > > > These patches address just the flip bits and not the HBLANK, though I > > think the latter is a more straightforward problem. The flip bits are > > troublesome because they affect the reported Bayer orders. > > > > So here the idea is to store the formats in the sensor's native Bayer > > order, as we were doing before. Only we can't reset the flip bits now, > > we have to query them and transform any Bayer formats the sensor gives > > us back into the native order. Two patches implement this: > > > > 1. The first patch adds a function to turn BayerFormats back into mbus > > codes. I couldn't see that we had functions to do this already, but > > please correct me if I missed something! > > > > 2. The second patch queries the flip bits and transforms any > > BayerFormats back into the native order to be stored. > > > > As I said, this potentially affects all pipeline handlers, which is > > why I left it "RFC" for the moment. I did take a look through them all > > to see what we might need to do: > > > > raspberrypi.cpp > > > > I think this PH is OK. We assume we're given the native Bayer order > > and transform it according to the final sensor transform, setting the > > flip bits every time. We update the format in any raw stream to match. > > > > uvcvideo.cpp, simple.cpp > > > > These appear to hardcode the transform to the Identity. I think mostly > > they should be OK because the formats here won't be raw and you can't > > have raw streams (?). Probably they should clear the flip bits to zero > > where these controls exist, perhaps someone who knows more about them > > could comment. > > > > rkisp1.cpp: > > > > Seems to force the transform to the Identity as well. So I think this > > is like the above ones, it should be OK though it probably needs to > > clear the sensor flip bits as well. > > > > ipu3.cpp: > > > > This one does handle transforms and appears to set the flip bits, so > > that's good. I couldn't see it transforming the Bayer order for raw > > streams correctly, so that probably wants checking. (To be fair, this > > looks suspect already and is not made worse by these changes!) > > > > Thoughts and opinions gratefully received, as always. > > > > Thanks! > > David > > > > David Plowman (2): > > libcamera: bayer_format: Add toMbusCode method > > libcamera: camera_sensor: Do not clear camera flips when listing > > formats > > > > include/libcamera/internal/bayer_format.h | 1 + > > src/libcamera/bayer_format.cpp | 12 ++++++ > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++---- > > 3 files changed, 56 insertions(+), 8 deletions(-) > > > > -- > > 2.30.2 > > >
Hi Naush On Thu, Nov 03, 2022 at 12:05:31PM +0000, Naushir Patuck wrote: > Hi Jacopo, > > > On Thu, 3 Nov 2022 at 11:50, Jacopo Mondi via libcamera-devel < > libcamera-devel@lists.libcamera.org> wrote: > > > Hi David > > > > On Thu, Nov 03, 2022 at 10:40:25AM +0000, David Plowman via > > libcamera-devel wrote: > > > Hi everyone > > > > > > (Warning: may affect all pipeline handlers!) > > > > > > I wanted to start the ball rolling on this. The problem is that when > > > we enumerate the cameras in the system, we try to set some of the > > > controls (specifically the HFLIP and VFLIP ones, and also later > > > HBLANK). But this will fail if we don't "own" the camera, for example, > > > it is already in use elsewhere. > > > > Feels like I'm missing one point here.. > > > > How do you get to CameraSensor::init() if your pipeline handler > > acquires the unicam media device at the very beginning of match() ? > > > > I understand we allow multiple application acquire the same PH as the > > ph might register multiple cameras, but why is this a problem at > > match() time ? > > > > In match(), the rpi, rkisp and ipu3 pipeline handlers call > CameraSensor::init() as part of the Camera object registration. This call > is unconditional assuming we can enumerate and register the sensor. > Exactly, so CameraSensor::init() is called once, regardless of which application will later "own" the camera. Sensor's controls are reset indeed, is this the problem, that application do not expect to have the controls reset ? > Not sure if that answers your question though? > > Naush > > > > > > > > > > > These patches address just the flip bits and not the HBLANK, though I > > > think the latter is a more straightforward problem. The flip bits are > > > troublesome because they affect the reported Bayer orders. > > > > > > So here the idea is to store the formats in the sensor's native Bayer > > > order, as we were doing before. Only we can't reset the flip bits now, > > > we have to query them and transform any Bayer formats the sensor gives > > > us back into the native order. Two patches implement this: > > > > > > 1. The first patch adds a function to turn BayerFormats back into mbus > > > codes. I couldn't see that we had functions to do this already, but > > > please correct me if I missed something! > > > > > > 2. The second patch queries the flip bits and transforms any > > > BayerFormats back into the native order to be stored. > > > > > > As I said, this potentially affects all pipeline handlers, which is > > > why I left it "RFC" for the moment. I did take a look through them all > > > to see what we might need to do: > > > > > > raspberrypi.cpp > > > > > > I think this PH is OK. We assume we're given the native Bayer order > > > and transform it according to the final sensor transform, setting the > > > flip bits every time. We update the format in any raw stream to match. > > > > > > uvcvideo.cpp, simple.cpp > > > > > > These appear to hardcode the transform to the Identity. I think mostly > > > they should be OK because the formats here won't be raw and you can't > > > have raw streams (?). Probably they should clear the flip bits to zero > > > where these controls exist, perhaps someone who knows more about them > > > could comment. > > > > > > rkisp1.cpp: > > > > > > Seems to force the transform to the Identity as well. So I think this > > > is like the above ones, it should be OK though it probably needs to > > > clear the sensor flip bits as well. > > > > > > ipu3.cpp: > > > > > > This one does handle transforms and appears to set the flip bits, so > > > that's good. I couldn't see it transforming the Bayer order for raw > > > streams correctly, so that probably wants checking. (To be fair, this > > > looks suspect already and is not made worse by these changes!) > > > > > > Thoughts and opinions gratefully received, as always. > > > > > > Thanks! > > > David > > > > > > David Plowman (2): > > > libcamera: bayer_format: Add toMbusCode method > > > libcamera: camera_sensor: Do not clear camera flips when listing > > > formats > > > > > > include/libcamera/internal/bayer_format.h | 1 + > > > src/libcamera/bayer_format.cpp | 12 ++++++ > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++---- > > > 3 files changed, 56 insertions(+), 8 deletions(-) > > > > > > -- > > > 2.30.2 > > > > >
Hi Jacopo, On Thu, 3 Nov 2022 at 12:59, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Naush > > On Thu, Nov 03, 2022 at 12:05:31PM +0000, Naushir Patuck wrote: > > Hi Jacopo, > > > > > > On Thu, 3 Nov 2022 at 11:50, Jacopo Mondi via libcamera-devel < > > libcamera-devel@lists.libcamera.org> wrote: > > > > > Hi David > > > > > > On Thu, Nov 03, 2022 at 10:40:25AM +0000, David Plowman via > > > libcamera-devel wrote: > > > > Hi everyone > > > > > > > > (Warning: may affect all pipeline handlers!) > > > > > > > > I wanted to start the ball rolling on this. The problem is that when > > > > we enumerate the cameras in the system, we try to set some of the > > > > controls (specifically the HFLIP and VFLIP ones, and also later > > > > HBLANK). But this will fail if we don't "own" the camera, for > example, > > > > it is already in use elsewhere. > > > > > > Feels like I'm missing one point here.. > > > > > > How do you get to CameraSensor::init() if your pipeline handler > > > acquires the unicam media device at the very beginning of match() ? > > > > > > I understand we allow multiple application acquire the same PH as the > > > ph might register multiple cameras, but why is this a problem at > > > match() time ? > > > > > > > In match(), the rpi, rkisp and ipu3 pipeline handlers call > > CameraSensor::init() as part of the Camera object registration. This > call > > is unconditional assuming we can enumerate and register the sensor. > > > > Exactly, so CameraSensor::init() is called once, regardless of which > application will later "own" the camera. Sensor's controls are > reset indeed, is this the problem, that application do not expect to > have the controls reset ? > Ah, this is where our understanding may differ. The enumeration code - match(), registerCamera() and CameraSensor::init() will be called for every (concurrent or single) libcamera process. This happens even when one of these processes has acquired and is actively running the camera, and in this case the other processes doing the enumeration will try to write to the sensor when they shouldn't from CameraSensor::init(). Again, if this does not make sense or actually answer your question, please shout :-) Naush > > > Not sure if that answers your question though? > > > > Naush > > > > > > > > > > > > > > > > > These patches address just the flip bits and not the HBLANK, though I > > > > think the latter is a more straightforward problem. The flip bits are > > > > troublesome because they affect the reported Bayer orders. > > > > > > > > So here the idea is to store the formats in the sensor's native Bayer > > > > order, as we were doing before. Only we can't reset the flip bits > now, > > > > we have to query them and transform any Bayer formats the sensor > gives > > > > us back into the native order. Two patches implement this: > > > > > > > > 1. The first patch adds a function to turn BayerFormats back into > mbus > > > > codes. I couldn't see that we had functions to do this already, but > > > > please correct me if I missed something! > > > > > > > > 2. The second patch queries the flip bits and transforms any > > > > BayerFormats back into the native order to be stored. > > > > > > > > As I said, this potentially affects all pipeline handlers, which is > > > > why I left it "RFC" for the moment. I did take a look through them > all > > > > to see what we might need to do: > > > > > > > > raspberrypi.cpp > > > > > > > > I think this PH is OK. We assume we're given the native Bayer order > > > > and transform it according to the final sensor transform, setting the > > > > flip bits every time. We update the format in any raw stream to > match. > > > > > > > > uvcvideo.cpp, simple.cpp > > > > > > > > These appear to hardcode the transform to the Identity. I think > mostly > > > > they should be OK because the formats here won't be raw and you can't > > > > have raw streams (?). Probably they should clear the flip bits to > zero > > > > where these controls exist, perhaps someone who knows more about them > > > > could comment. > > > > > > > > rkisp1.cpp: > > > > > > > > Seems to force the transform to the Identity as well. So I think this > > > > is like the above ones, it should be OK though it probably needs to > > > > clear the sensor flip bits as well. > > > > > > > > ipu3.cpp: > > > > > > > > This one does handle transforms and appears to set the flip bits, so > > > > that's good. I couldn't see it transforming the Bayer order for raw > > > > streams correctly, so that probably wants checking. (To be fair, this > > > > looks suspect already and is not made worse by these changes!) > > > > > > > > Thoughts and opinions gratefully received, as always. > > > > > > > > Thanks! > > > > David > > > > > > > > David Plowman (2): > > > > libcamera: bayer_format: Add toMbusCode method > > > > libcamera: camera_sensor: Do not clear camera flips when listing > > > > formats > > > > > > > > include/libcamera/internal/bayer_format.h | 1 + > > > > src/libcamera/bayer_format.cpp | 12 ++++++ > > > > src/libcamera/camera_sensor.cpp | 51 > +++++++++++++++++++---- > > > > 3 files changed, 56 insertions(+), 8 deletions(-) > > > > > > > > -- > > > > 2.30.2 > > > > > > > >
Hi Naush On Thu, Nov 03, 2022 at 01:26:42PM +0000, Naushir Patuck wrote: > Hi Jacopo, > > On Thu, 3 Nov 2022 at 12:59, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hi Naush > > > > On Thu, Nov 03, 2022 at 12:05:31PM +0000, Naushir Patuck wrote: > > > Hi Jacopo, > > > > > > > > > On Thu, 3 Nov 2022 at 11:50, Jacopo Mondi via libcamera-devel < > > > libcamera-devel@lists.libcamera.org> wrote: > > > > > > > Hi David > > > > > > > > On Thu, Nov 03, 2022 at 10:40:25AM +0000, David Plowman via > > > > libcamera-devel wrote: > > > > > Hi everyone > > > > > > > > > > (Warning: may affect all pipeline handlers!) > > > > > > > > > > I wanted to start the ball rolling on this. The problem is that when > > > > > we enumerate the cameras in the system, we try to set some of the > > > > > controls (specifically the HFLIP and VFLIP ones, and also later > > > > > HBLANK). But this will fail if we don't "own" the camera, for > > example, > > > > > it is already in use elsewhere. > > > > > > > > Feels like I'm missing one point here.. > > > > > > > > How do you get to CameraSensor::init() if your pipeline handler > > > > acquires the unicam media device at the very beginning of match() ? > > > > > > > > I understand we allow multiple application acquire the same PH as the > > > > ph might register multiple cameras, but why is this a problem at > > > > match() time ? > > > > > > > > > > In match(), the rpi, rkisp and ipu3 pipeline handlers call > > > CameraSensor::init() as part of the Camera object registration. This > > call > > > is unconditional assuming we can enumerate and register the sensor. > > > > > > > Exactly, so CameraSensor::init() is called once, regardless of which > > application will later "own" the camera. Sensor's controls are > > reset indeed, is this the problem, that application do not expect to > > have the controls reset ? > > > > Ah, this is where our understanding may differ. The enumeration code - > match(), registerCamera() and CameraSensor::init() will be called for every > (concurrent or single) libcamera process. > This happens even when one of these processes has acquired and is actively > running the camera, and in this case the other processes doing the > enumeration will try to write to the sensor when they shouldn't from > CameraSensor::init(). > > Again, if this does not make sense or actually answer your question, please > shout :-) > No it does, I was assuming a single libcamera instance running and an upper layer arbitrator that mediates the camera access. But that would be pipewire... Sorry, it was trivial and this indeed is a valid use case. More below > Naush > > > > > > > Not sure if that answers your question though? > > > > > > Naush > > > > > > > > > > > > > > > > > > > > > > > These patches address just the flip bits and not the HBLANK, though I > > > > > think the latter is a more straightforward problem. The flip bits are > > > > > troublesome because they affect the reported Bayer orders. > > > > > > > > > > So here the idea is to store the formats in the sensor's native Bayer > > > > > order, as we were doing before. Only we can't reset the flip bits > > now, > > > > > we have to query them and transform any Bayer formats the sensor > > gives > > > > > us back into the native order. Two patches implement this: > > > > > > > > > > 1. The first patch adds a function to turn BayerFormats back into > > mbus > > > > > codes. I couldn't see that we had functions to do this already, but > > > > > please correct me if I missed something! > > > > > I don't see any helper for that, so the change is welcome > > > > > 2. The second patch queries the flip bits and transforms any > > > > > BayerFormats back into the native order to be stored. > > > > > > > > > > As I said, this potentially affects all pipeline handlers, which is > > > > > why I left it "RFC" for the moment. I did take a look through them > > all > > > > > to see what we might need to do: > > > > > > > > > > raspberrypi.cpp > > > > > > > > > > I think this PH is OK. We assume we're given the native Bayer order > > > > > and transform it according to the final sensor transform, setting the > > > > > flip bits every time. We update the format in any raw stream to > > match. > > > > > > > > > > uvcvideo.cpp, simple.cpp > > > > > > > > > > These appear to hardcode the transform to the Identity. I think > > mostly > > > > > they should be OK because the formats here won't be raw and you can't > > > > > have raw streams (?). Probably they should clear the flip bits to It is possible to have RAW streams with the simple pipeline theoretically. Even more so considering the plan of plumbing a sofware ISP/debayer to it > > zero > > > > > where these controls exist, perhaps someone who knows more about them > > > > > could comment. > > > > > > > > > > rkisp1.cpp: > > > > > > > > > > Seems to force the transform to the Identity as well. So I think this > > > > > is like the above ones, it should be OK though it probably needs to > > > > > clear the sensor flip bits as well. > > > > > > > > > > ipu3.cpp: > > > > > > > > > > This one does handle transforms and appears to set the flip bits, so > > > > > that's good. I couldn't see it transforming the Bayer order for raw > > > > > streams correctly, so that probably wants checking. (To be fair, this > > > > > looks suspect already and is not made worse by these changes!) > > > > > > > > > > Thoughts and opinions gratefully received, as always. I'll reply to the patch > > > > > > > > > > Thanks! > > > > > David > > > > > > > > > > David Plowman (2): > > > > > libcamera: bayer_format: Add toMbusCode method > > > > > libcamera: camera_sensor: Do not clear camera flips when listing > > > > > formats > > > > > > > > > > include/libcamera/internal/bayer_format.h | 1 + > > > > > src/libcamera/bayer_format.cpp | 12 ++++++ > > > > > src/libcamera/camera_sensor.cpp | 51 > > +++++++++++++++++++---- > > > > > 3 files changed, 56 insertions(+), 8 deletions(-) > > > > > > > > > > -- > > > > > 2.30.2 > > > > > > > > > > >
Hi everyone (Warning: may affect all pipeline handlers!) I wanted to start the ball rolling on this. The problem is that when we enumerate the cameras in the system, we try to set some of the controls (specifically the HFLIP and VFLIP ones, and also later HBLANK). But this will fail if we don't "own" the camera, for example, it is already in use elsewhere. These patches address just the flip bits and not the HBLANK, though I think the latter is a more straightforward problem. The flip bits are troublesome because they affect the reported Bayer orders. So here the idea is to store the formats in the sensor's native Bayer order, as we were doing before. Only we can't reset the flip bits now, we have to query them and transform any Bayer formats the sensor gives us back into the native order. Two patches implement this: 1. The first patch adds a function to turn BayerFormats back into mbus codes. I couldn't see that we had functions to do this already, but please correct me if I missed something! 2. The second patch queries the flip bits and transforms any BayerFormats back into the native order to be stored. As I said, this potentially affects all pipeline handlers, which is why I left it "RFC" for the moment. I did take a look through them all to see what we might need to do: raspberrypi.cpp I think this PH is OK. We assume we're given the native Bayer order and transform it according to the final sensor transform, setting the flip bits every time. We update the format in any raw stream to match. uvcvideo.cpp, simple.cpp These appear to hardcode the transform to the Identity. I think mostly they should be OK because the formats here won't be raw and you can't have raw streams (?). Probably they should clear the flip bits to zero where these controls exist, perhaps someone who knows more about them could comment. rkisp1.cpp: Seems to force the transform to the Identity as well. So I think this is like the above ones, it should be OK though it probably needs to clear the sensor flip bits as well. ipu3.cpp: This one does handle transforms and appears to set the flip bits, so that's good. I couldn't see it transforming the Bayer order for raw streams correctly, so that probably wants checking. (To be fair, this looks suspect already and is not made worse by these changes!) Thoughts and opinions gratefully received, as always. Thanks! David David Plowman (2): libcamera: bayer_format: Add toMbusCode method libcamera: camera_sensor: Do not clear camera flips when listing formats include/libcamera/internal/bayer_format.h | 1 + src/libcamera/bayer_format.cpp | 12 ++++++ src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++---- 3 files changed, 56 insertions(+), 8 deletions(-)