| Message ID | 20221121154421.11732-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David, On Mon, Nov 21, 2022 at 03:44:16PM +0000, David Plowman via libcamera-devel wrote: > Hi again > > In v2 of this set, the first 3 patches are unchanged, so thanks for > some of the reviews there. > > In the light of the various discussions, I've updated the changes to > those other PHs (uvcvideo, simple and rkisp1). In all cases I'm being > more explicit that I want to preserve the previous behaviour before > this patch set was introduced. This may or may not (probably may not) > be the actual correct behaviour that people would want. So: > > uvcvideo: I've deleted the patch that updated this. These are > unaffected by CameraSensor changes so I think I'm better off just > leaving them alone. > > simple and rkisp1: Both of these, AIUI, use the CameraSensor class so > previously the flips were getting cleared. Therefore I've added > exactly this into the PHs so that the behaviour is unchanged. > > As we had discussed, I think there are concerns that the simple and > rkisp1 PHs implement incorrect behaviours in at least some respects, > but I think it's probably a separate job for someone working on those > platforms to investigate. Agreed, but I'm not sure it is worth to introduce the v4l2 device helper for this temporary solution, as I'm not sure that's the direction where we want to go (it would be different if the V4L2Device helper is used by other PH to implement Transform support). As said, my understanding is that the direction where we want to go is to reset flips to their driver defaults, and that should be happening transparently in the CameraSensor::setFormat() call path... I don't think we risk regressions if we merge the first two patches only, while we would solve your use case, so if others agree, I would just pick the first two ones.. Thanks j > > Does that make sense? > > Thanks! > > David > > David Plowman (5): > libcamera: bayer_format: Add toMbusCode method > libcamera: camera_sensor: Do not clear camera flips when listing > formats > libcamera: v4l2_device: Add setTransform method to set a device's flip > controls > libcamera: pipeline: simple: Set device's flip controls as previously > libcamera: pipeline: rkisp1: Set device's flip controls as previously > > include/libcamera/internal/bayer_format.h | 1 + > include/libcamera/internal/v4l2_device.h | 3 ++ > src/libcamera/bayer_format.cpp | 11 +++++ > src/libcamera/camera_sensor.cpp | 49 ++++++++++++++++++----- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 ++ > src/libcamera/pipeline/simple/simple.cpp | 3 ++ > src/libcamera/v4l2_device.cpp | 37 +++++++++++++++++ > 7 files changed, 97 insertions(+), 10 deletions(-) > > -- > 2.30.2 >
Hi Jacopo On Mon, 21 Nov 2022 at 16:59, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > On Mon, Nov 21, 2022 at 03:44:16PM +0000, David Plowman via libcamera-devel wrote: > > Hi again > > > > In v2 of this set, the first 3 patches are unchanged, so thanks for > > some of the reviews there. > > > > In the light of the various discussions, I've updated the changes to > > those other PHs (uvcvideo, simple and rkisp1). In all cases I'm being > > more explicit that I want to preserve the previous behaviour before > > this patch set was introduced. This may or may not (probably may not) > > be the actual correct behaviour that people would want. So: > > > > uvcvideo: I've deleted the patch that updated this. These are > > unaffected by CameraSensor changes so I think I'm better off just > > leaving them alone. > > > > simple and rkisp1: Both of these, AIUI, use the CameraSensor class so > > previously the flips were getting cleared. Therefore I've added > > exactly this into the PHs so that the behaviour is unchanged. > > > > As we had discussed, I think there are concerns that the simple and > > rkisp1 PHs implement incorrect behaviours in at least some respects, > > but I think it's probably a separate job for someone working on those > > platforms to investigate. > > Agreed, but I'm not sure it is worth to introduce the v4l2 device > helper for this temporary solution, as I'm not sure that's the > direction where we want to go (it would be different if the V4L2Device > helper is used by other PH to implement Transform support). > > As said, my understanding is that the direction where we want to go is > to reset flips to their driver defaults, and that should be happening > transparently in the CameraSensor::setFormat() call path... > > I don't think we risk regressions if we merge the first two patches > only, while we would solve your use case, so if others agree, I would > just pick the first two ones.. That certainly works for me. I think the result will be that the code, which is wrong already, will remain wrong in a slightly different way, but I also suspect no one will notice until they start looking at these specific cases (which would be the time to fix them). So I'll re-post just the first two patches. And then there's still the sensor HBLANK to sort out... :) Thanks! David > > Thanks > j > > > > > Does that make sense? > > > > Thanks! > > > > David > > > > David Plowman (5): > > libcamera: bayer_format: Add toMbusCode method > > libcamera: camera_sensor: Do not clear camera flips when listing > > formats > > libcamera: v4l2_device: Add setTransform method to set a device's flip > > controls > > libcamera: pipeline: simple: Set device's flip controls as previously > > libcamera: pipeline: rkisp1: Set device's flip controls as previously > > > > include/libcamera/internal/bayer_format.h | 1 + > > include/libcamera/internal/v4l2_device.h | 3 ++ > > src/libcamera/bayer_format.cpp | 11 +++++ > > src/libcamera/camera_sensor.cpp | 49 ++++++++++++++++++----- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 ++ > > src/libcamera/pipeline/simple/simple.cpp | 3 ++ > > src/libcamera/v4l2_device.cpp | 37 +++++++++++++++++ > > 7 files changed, 97 insertions(+), 10 deletions(-) > > > > -- > > 2.30.2 > >
Hi again In v2 of this set, the first 3 patches are unchanged, so thanks for some of the reviews there. In the light of the various discussions, I've updated the changes to those other PHs (uvcvideo, simple and rkisp1). In all cases I'm being more explicit that I want to preserve the previous behaviour before this patch set was introduced. This may or may not (probably may not) be the actual correct behaviour that people would want. So: uvcvideo: I've deleted the patch that updated this. These are unaffected by CameraSensor changes so I think I'm better off just leaving them alone. simple and rkisp1: Both of these, AIUI, use the CameraSensor class so previously the flips were getting cleared. Therefore I've added exactly this into the PHs so that the behaviour is unchanged. As we had discussed, I think there are concerns that the simple and rkisp1 PHs implement incorrect behaviours in at least some respects, but I think it's probably a separate job for someone working on those platforms to investigate. Does that make sense? Thanks! David David Plowman (5): libcamera: bayer_format: Add toMbusCode method libcamera: camera_sensor: Do not clear camera flips when listing formats libcamera: v4l2_device: Add setTransform method to set a device's flip controls libcamera: pipeline: simple: Set device's flip controls as previously libcamera: pipeline: rkisp1: Set device's flip controls as previously include/libcamera/internal/bayer_format.h | 1 + include/libcamera/internal/v4l2_device.h | 3 ++ src/libcamera/bayer_format.cpp | 11 +++++ src/libcamera/camera_sensor.cpp | 49 ++++++++++++++++++----- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 ++ src/libcamera/pipeline/simple/simple.cpp | 3 ++ src/libcamera/v4l2_device.cpp | 37 +++++++++++++++++ 7 files changed, 97 insertions(+), 10 deletions(-)