| Message ID | 20211206233948.1351206-1-kieran.bingham@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patches. On Mon, Dec 06, 2021 at 11:39:40PM +0000, Kieran Bingham wrote: > When completing a request, the individual stream buffers contain a > sequence number. This number is generated by the device that ultimately > fills that stream, but it might not be the sensor. Processing through an > ISP could cause the sequence numbers and timestamp data associated with > the completed buffer to be values representative of the ISP processing > rather than the (intended) capture device. > > Provide a new metadata control, still to be fully sketched out which > gives us a defined value to report the Camera sequence number. This > allows pipeline handlers to correctly set the value according to the > device that represents the capture from the sensor. > > This plumbing then allows applications to detect frame drops, which were > otherwise going unnoticed, and as such some basic additions have been > made to cam, qcam, and gstreamer to support this new data. > > Still possible: > - Adding a validation to lc-compliance to make sure pipelines set the > SensorSequence on every frame. > > - Probably expecting some better gstreamer event integration perhaps? > > - qcam should report more statistics on the processing overall > > - libcamera Tracepoints could be added as an event to track when > frames are detected as dropped by the core framework. > > Anything else? A basic design question: when a frame is dropped, shouldn't we report the corresponding request as failed ? That's how the Android camera HAL API operates, and while that by itself isn't a good enough reason to do that same, I think it offers a better way to ensure that controls get synchronized with the buffers that frames are captured to. > Kieran Bingham (8): > libcamera: controls: Add SensorSequence metadata control > libcamera: pipeline: Set the Sensor sequence number for all pipelines > cam: Use SensorTimestamp rather than buffer metadata > cam: Use Sensor sequence numbers and detect frame drop > qcam: main_window: Fix include ordering > qcam: Use Sensor sequence numbers and detect frame drop > gstreamer: gstlibcamerasrc: Fix include ordering > gstreamer: Use Sensor sequence numbers and detect frame drop > > src/cam/camera_session.cpp | 24 ++++++++-- > src/cam/camera_session.h | 1 + > src/gstreamer/gstlibcamerasrc.cpp | 46 +++++++++++++++---- > src/libcamera/control_ids.yaml | 8 ++++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 3 ++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +- > src/libcamera/pipeline/simple/simple.cpp | 12 +++-- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 + > src/qcam/main_window.cpp | 28 +++++++++-- > src/qcam/main_window.h | 1 + > 11 files changed, 111 insertions(+), 22 deletions(-)
Hi Kieran and Laurent, On Tue, 7 Dec 2021 at 00:07, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Kieran, > > Thank you for the patches. > > On Mon, Dec 06, 2021 at 11:39:40PM +0000, Kieran Bingham wrote: > > When completing a request, the individual stream buffers contain a > > sequence number. This number is generated by the device that ultimately > > fills that stream, but it might not be the sensor. Processing through an > > ISP could cause the sequence numbers and timestamp data associated with > > the completed buffer to be values representative of the ISP processing > > rather than the (intended) capture device. > > > > Provide a new metadata control, still to be fully sketched out which > > gives us a defined value to report the Camera sequence number. This > > allows pipeline handlers to correctly set the value according to the > > device that represents the capture from the sensor. > > > > This plumbing then allows applications to detect frame drops, which were > > otherwise going unnoticed, and as such some basic additions have been > > made to cam, qcam, and gstreamer to support this new data. > > > > Still possible: > > - Adding a validation to lc-compliance to make sure pipelines set the > > SensorSequence on every frame. > > > > - Probably expecting some better gstreamer event integration perhaps? > > > > - qcam should report more statistics on the processing overall > > > > - libcamera Tracepoints could be added as an event to track when > > frames are detected as dropped by the core framework. > > > > Anything else? > > A basic design question: when a frame is dropped, shouldn't we report > the corresponding request as failed ? That's how the Android camera HAL > API operates, and while that by itself isn't a good enough reason to do > that same, I think it offers a better way to ensure that controls get > synchronized with the buffers that frames are captured to. > For the Raspberry Pi platforms, it is possible for the Unicam driver to drop frames without the knowledge of the pipeline handler, if for example we do not recycle bayer buffers quick enough. Allowing the application to look at sensor timestamps would allow it to detect these drops. Unless I am mistaken, this would not be possible by failing the request, as the sensor buffer recycling loop is asynchronous to the application request loop. Having said that, they could be synchronised.... Regards, Naush > > > Kieran Bingham (8): > > libcamera: controls: Add SensorSequence metadata control > > libcamera: pipeline: Set the Sensor sequence number for all pipelines > > cam: Use SensorTimestamp rather than buffer metadata > > cam: Use Sensor sequence numbers and detect frame drop > > qcam: main_window: Fix include ordering > > qcam: Use Sensor sequence numbers and detect frame drop > > gstreamer: gstlibcamerasrc: Fix include ordering > > gstreamer: Use Sensor sequence numbers and detect frame drop > > > > src/cam/camera_session.cpp | 24 ++++++++-- > > src/cam/camera_session.h | 1 + > > src/gstreamer/gstlibcamerasrc.cpp | 46 +++++++++++++++---- > > src/libcamera/control_ids.yaml | 8 ++++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 ++ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +- > > src/libcamera/pipeline/simple/simple.cpp | 12 +++-- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 + > > src/qcam/main_window.cpp | 28 +++++++++-- > > src/qcam/main_window.h | 1 + > > 11 files changed, 111 insertions(+), 22 deletions(-) > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Tue, Dec 07, 2021 at 08:42:36AM +0000, Naushir Patuck wrote: > On Tue, 7 Dec 2021 at 00:07, Laurent Pinchart wrote: > > On Mon, Dec 06, 2021 at 11:39:40PM +0000, Kieran Bingham wrote: > > > When completing a request, the individual stream buffers contain a > > > sequence number. This number is generated by the device that ultimately > > > fills that stream, but it might not be the sensor. Processing through an > > > ISP could cause the sequence numbers and timestamp data associated with > > > the completed buffer to be values representative of the ISP processing > > > rather than the (intended) capture device. > > > > > > Provide a new metadata control, still to be fully sketched out which > > > gives us a defined value to report the Camera sequence number. This > > > allows pipeline handlers to correctly set the value according to the > > > device that represents the capture from the sensor. > > > > > > This plumbing then allows applications to detect frame drops, which were > > > otherwise going unnoticed, and as such some basic additions have been > > > made to cam, qcam, and gstreamer to support this new data. > > > > > > Still possible: > > > - Adding a validation to lc-compliance to make sure pipelines set the > > > SensorSequence on every frame. > > > > > > - Probably expecting some better gstreamer event integration perhaps? > > > > > > - qcam should report more statistics on the processing overall > > > > > > - libcamera Tracepoints could be added as an event to track when > > > frames are detected as dropped by the core framework. > > > > > > Anything else? > > > > A basic design question: when a frame is dropped, shouldn't we report > > the corresponding request as failed ? That's how the Android camera HAL > > API operates, and while that by itself isn't a good enough reason to do > > that same, I think it offers a better way to ensure that controls get > > synchronized with the buffers that frames are captured to. > > For the Raspberry Pi platforms, it is possible for the Unicam driver > to drop frames without the knowledge of the pipeline handler, if for example > we do not recycle bayer buffers quick enough. So the driver doesn't increment the sequence number in that case ? Could this be fixed ? > Allowing the application > to look at sensor timestamps would allow it to detect these drops. Unless > I am mistaken, this would not be possible by failing the request, as the > sensor buffer recycling loop is asynchronous to the application request > loop. Having said that, they could be synchronised.... This is also related to implementing true per-frame control, synchronizing the controls from the request with the buffers in which images are captured :-) > > > Kieran Bingham (8): > > > libcamera: controls: Add SensorSequence metadata control > > > libcamera: pipeline: Set the Sensor sequence number for all pipelines > > > cam: Use SensorTimestamp rather than buffer metadata > > > cam: Use Sensor sequence numbers and detect frame drop > > > qcam: main_window: Fix include ordering > > > qcam: Use Sensor sequence numbers and detect frame drop > > > gstreamer: gstlibcamerasrc: Fix include ordering > > > gstreamer: Use Sensor sequence numbers and detect frame drop > > > > > > src/cam/camera_session.cpp | 24 ++++++++-- > > > src/cam/camera_session.h | 1 + > > > src/gstreamer/gstlibcamerasrc.cpp | 46 +++++++++++++++---- > > > src/libcamera/control_ids.yaml | 8 ++++ > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 ++ > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +- > > > src/libcamera/pipeline/simple/simple.cpp | 12 +++-- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 + > > > src/qcam/main_window.cpp | 28 +++++++++-- > > > src/qcam/main_window.h | 1 + > > > 11 files changed, 111 insertions(+), 22 deletions(-)
Hi Laurent, On Tue, 7 Dec 2021 at 14:22, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Tue, Dec 07, 2021 at 08:42:36AM +0000, Naushir Patuck wrote: > > On Tue, 7 Dec 2021 at 00:07, Laurent Pinchart wrote: > > > On Mon, Dec 06, 2021 at 11:39:40PM +0000, Kieran Bingham wrote: > > > > When completing a request, the individual stream buffers contain a > > > > sequence number. This number is generated by the device that > ultimately > > > > fills that stream, but it might not be the sensor. Processing > through an > > > > ISP could cause the sequence numbers and timestamp data associated > with > > > > the completed buffer to be values representative of the ISP > processing > > > > rather than the (intended) capture device. > > > > > > > > Provide a new metadata control, still to be fully sketched out which > > > > gives us a defined value to report the Camera sequence number. This > > > > allows pipeline handlers to correctly set the value according to the > > > > device that represents the capture from the sensor. > > > > > > > > This plumbing then allows applications to detect frame drops, which > were > > > > otherwise going unnoticed, and as such some basic additions have been > > > > made to cam, qcam, and gstreamer to support this new data. > > > > > > > > Still possible: > > > > - Adding a validation to lc-compliance to make sure pipelines set > the > > > > SensorSequence on every frame. > > > > > > > > - Probably expecting some better gstreamer event integration > perhaps? > > > > > > > > - qcam should report more statistics on the processing overall > > > > > > > > - libcamera Tracepoints could be added as an event to track when > > > > frames are detected as dropped by the core framework. > > > > > > > > Anything else? > > > > > > A basic design question: when a frame is dropped, shouldn't we report > > > the corresponding request as failed ? That's how the Android camera HAL > > > API operates, and while that by itself isn't a good enough reason to do > > > that same, I think it offers a better way to ensure that controls get > > > synchronized with the buffers that frames are captured to. > > > > For the Raspberry Pi platforms, it is possible for the Unicam driver > > to drop frames without the knowledge of the pipeline handler, if for > example > > we do not recycle bayer buffers quick enough. > > So the driver doesn't increment the sequence number in that case ? Could > this be fixed ? > The driver increments sequence numbers based on frame interrupts, so they change even if the frame is going to be dropped. > > > Allowing the application > > to look at sensor timestamps would allow it to detect these drops. Unless > > I am mistaken, this would not be possible by failing the request, as the > > sensor buffer recycling loop is asynchronous to the application request > > loop. Having said that, they could be synchronised.... > > This is also related to implementing true per-frame control, > synchronizing the controls from the request with the buffers in which > images are captured :-) > As long as they are ISP based controls we are fine :-) Regards, Naush > > > > > Kieran Bingham (8): > > > > libcamera: controls: Add SensorSequence metadata control > > > > libcamera: pipeline: Set the Sensor sequence number for all > pipelines > > > > cam: Use SensorTimestamp rather than buffer metadata > > > > cam: Use Sensor sequence numbers and detect frame drop > > > > qcam: main_window: Fix include ordering > > > > qcam: Use Sensor sequence numbers and detect frame drop > > > > gstreamer: gstlibcamerasrc: Fix include ordering > > > > gstreamer: Use Sensor sequence numbers and detect frame drop > > > > > > > > src/cam/camera_session.cpp | 24 ++++++++-- > > > > src/cam/camera_session.h | 1 + > > > > src/gstreamer/gstlibcamerasrc.cpp | 46 > +++++++++++++++---- > > > > src/libcamera/control_ids.yaml | 8 ++++ > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 ++ > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +- > > > > src/libcamera/pipeline/simple/simple.cpp | 12 +++-- > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 + > > > > src/qcam/main_window.cpp | 28 +++++++++-- > > > > src/qcam/main_window.h | 1 + > > > > 11 files changed, 111 insertions(+), 22 deletions(-) > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Tue, Dec 07, 2021 at 03:15:23PM +0000, Naushir Patuck wrote: > On Tue, 7 Dec 2021 at 14:22, Laurent Pinchart wrote: > > On Tue, Dec 07, 2021 at 08:42:36AM +0000, Naushir Patuck wrote: > > > On Tue, 7 Dec 2021 at 00:07, Laurent Pinchart wrote: > > > > On Mon, Dec 06, 2021 at 11:39:40PM +0000, Kieran Bingham wrote: > > > > > When completing a request, the individual stream buffers contain a > > > > > sequence number. This number is generated by the device that ultimately > > > > > fills that stream, but it might not be the sensor. Processing through an > > > > > ISP could cause the sequence numbers and timestamp data associated with > > > > > the completed buffer to be values representative of the ISP processing > > > > > rather than the (intended) capture device. > > > > > > > > > > Provide a new metadata control, still to be fully sketched out which > > > > > gives us a defined value to report the Camera sequence number. This > > > > > allows pipeline handlers to correctly set the value according to the > > > > > device that represents the capture from the sensor. > > > > > > > > > > This plumbing then allows applications to detect frame drops, which were > > > > > otherwise going unnoticed, and as such some basic additions have been > > > > > made to cam, qcam, and gstreamer to support this new data. > > > > > > > > > > Still possible: > > > > > - Adding a validation to lc-compliance to make sure pipelines set the > > > > > SensorSequence on every frame. > > > > > > > > > > - Probably expecting some better gstreamer event integration perhaps? > > > > > > > > > > - qcam should report more statistics on the processing overall > > > > > > > > > > - libcamera Tracepoints could be added as an event to track when > > > > > frames are detected as dropped by the core framework. > > > > > > > > > > Anything else? > > > > > > > > A basic design question: when a frame is dropped, shouldn't we report > > > > the corresponding request as failed ? That's how the Android camera HAL > > > > API operates, and while that by itself isn't a good enough reason to do > > > > that same, I think it offers a better way to ensure that controls get > > > > synchronized with the buffers that frames are captured to. > > > > > > For the Raspberry Pi platforms, it is possible for the Unicam driver > > > to drop frames without the knowledge of the pipeline handler, if for example > > > we do not recycle bayer buffers quick enough. > > > > So the driver doesn't increment the sequence number in that case ? Could > > this be fixed ? > > The driver increments sequence numbers based on frame interrupts, so they > change even if the frame is going to be dropped. Does this mean that the pipeline handler could then detect that condition, and react appropriately ? > > > Allowing the application > > > to look at sensor timestamps would allow it to detect these drops. Unless > > > I am mistaken, this would not be possible by failing the request, as the > > > sensor buffer recycling loop is asynchronous to the application request > > > loop. Having said that, they could be synchronised.... > > > > This is also related to implementing true per-frame control, > > synchronizing the controls from the request with the buffers in which > > images are captured :-) > > As long as they are ISP based controls we are fine :-) There's also manual exposure and analogue gain I'm afraid :-) > > > > > Kieran Bingham (8): > > > > > libcamera: controls: Add SensorSequence metadata control > > > > > libcamera: pipeline: Set the Sensor sequence number for all pipelines > > > > > cam: Use SensorTimestamp rather than buffer metadata > > > > > cam: Use Sensor sequence numbers and detect frame drop > > > > > qcam: main_window: Fix include ordering > > > > > qcam: Use Sensor sequence numbers and detect frame drop > > > > > gstreamer: gstlibcamerasrc: Fix include ordering > > > > > gstreamer: Use Sensor sequence numbers and detect frame drop > > > > > > > > > > src/cam/camera_session.cpp | 24 ++++++++-- > > > > > src/cam/camera_session.h | 1 + > > > > > src/gstreamer/gstlibcamerasrc.cpp | 46 +++++++++++++++---- > > > > > src/libcamera/control_ids.yaml | 8 ++++ > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 ++ > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +- > > > > > src/libcamera/pipeline/simple/simple.cpp | 12 +++-- > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 + > > > > > src/qcam/main_window.cpp | 28 +++++++++-- > > > > > src/qcam/main_window.h | 1 + > > > > > 11 files changed, 111 insertions(+), 22 deletions(-)
Hi Laurent, On Tue, 7 Dec 2021 at 16:47, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Tue, Dec 07, 2021 at 03:15:23PM +0000, Naushir Patuck wrote: > > On Tue, 7 Dec 2021 at 14:22, Laurent Pinchart wrote: > > > On Tue, Dec 07, 2021 at 08:42:36AM +0000, Naushir Patuck wrote: > > > > On Tue, 7 Dec 2021 at 00:07, Laurent Pinchart wrote: > > > > > On Mon, Dec 06, 2021 at 11:39:40PM +0000, Kieran Bingham wrote: > > > > > > When completing a request, the individual stream buffers contain > a > > > > > > sequence number. This number is generated by the device that > ultimately > > > > > > fills that stream, but it might not be the sensor. Processing > through an > > > > > > ISP could cause the sequence numbers and timestamp data > associated with > > > > > > the completed buffer to be values representative of the ISP > processing > > > > > > rather than the (intended) capture device. > > > > > > > > > > > > Provide a new metadata control, still to be fully sketched out > which > > > > > > gives us a defined value to report the Camera sequence number. > This > > > > > > allows pipeline handlers to correctly set the value according to > the > > > > > > device that represents the capture from the sensor. > > > > > > > > > > > > This plumbing then allows applications to detect frame drops, > which were > > > > > > otherwise going unnoticed, and as such some basic additions have > been > > > > > > made to cam, qcam, and gstreamer to support this new data. > > > > > > > > > > > > Still possible: > > > > > > - Adding a validation to lc-compliance to make sure pipelines > set the > > > > > > SensorSequence on every frame. > > > > > > > > > > > > - Probably expecting some better gstreamer event integration > perhaps? > > > > > > > > > > > > - qcam should report more statistics on the processing overall > > > > > > > > > > > > - libcamera Tracepoints could be added as an event to track when > > > > > > frames are detected as dropped by the core framework. > > > > > > > > > > > > Anything else? > > > > > > > > > > A basic design question: when a frame is dropped, shouldn't we > report > > > > > the corresponding request as failed ? That's how the Android > camera HAL > > > > > API operates, and while that by itself isn't a good enough reason > to do > > > > > that same, I think it offers a better way to ensure that controls > get > > > > > synchronized with the buffers that frames are captured to. > > > > > > > > For the Raspberry Pi platforms, it is possible for the Unicam driver > > > > to drop frames without the knowledge of the pipeline handler, if for > example > > > > we do not recycle bayer buffers quick enough. > > > > > > So the driver doesn't increment the sequence number in that case ? > Could > > > this be fixed ? > > > > The driver increments sequence numbers based on frame interrupts, so they > > change even if the frame is going to be dropped. > > Does this mean that the pipeline handler could then detect that > condition, and react appropriately ? > Yes it could. And this was discussed with Kieran before this change was made. I think we concluded, that this may be logic that is common to all pipeline handlers so might want to live in common code space rather than individual pipeline handlers. Thanks, Naush > > > > > Allowing the application > > > > to look at sensor timestamps would allow it to detect these drops. > Unless > > > > I am mistaken, this would not be possible by failing the request, as > the > > > > sensor buffer recycling loop is asynchronous to the application > request > > > > loop. Having said that, they could be synchronised.... > > > > > > This is also related to implementing true per-frame control, > > > synchronizing the controls from the request with the buffers in which > > > images are captured :-) > > > > As long as they are ISP based controls we are fine :-) > > There's also manual exposure and analogue gain I'm afraid :-) > > > > > > > Kieran Bingham (8): > > > > > > libcamera: controls: Add SensorSequence metadata control > > > > > > libcamera: pipeline: Set the Sensor sequence number for all > pipelines > > > > > > cam: Use SensorTimestamp rather than buffer metadata > > > > > > cam: Use Sensor sequence numbers and detect frame drop > > > > > > qcam: main_window: Fix include ordering > > > > > > qcam: Use Sensor sequence numbers and detect frame drop > > > > > > gstreamer: gstlibcamerasrc: Fix include ordering > > > > > > gstreamer: Use Sensor sequence numbers and detect frame drop > > > > > > > > > > > > src/cam/camera_session.cpp | 24 ++++++++-- > > > > > > src/cam/camera_session.h | 1 + > > > > > > src/gstreamer/gstlibcamerasrc.cpp | 46 > +++++++++++++++---- > > > > > > src/libcamera/control_ids.yaml | 8 ++++ > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 ++ > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +- > > > > > > src/libcamera/pipeline/simple/simple.cpp | 12 +++-- > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 + > > > > > > src/qcam/main_window.cpp | 28 +++++++++-- > > > > > > src/qcam/main_window.h | 1 + > > > > > > 11 files changed, 111 insertions(+), 22 deletions(-) > > -- > Regards, > > Laurent Pinchart >