diff mbox series

[19/31] staging: bcm2835-camera: Ensure timestamps never go backwards.

Message ID 1561661788-22744-20-git-send-email-wahrenst@gmx.net (mailing list archive)
State New, archived
Headers show
Series staging: bcm2835-camera: Improvements | expand

Commit Message

Stefan Wahren June 27, 2019, 6:56 p.m. UTC
From: Dave Stevenson <dave.stevenson@raspberrypi.org>

There is an awkward situation with H264 header bytes. Currently
they are returned with a PTS of 0 because they aren't associated
with a timestamped frame to encode. These are handled by either
returning the timestamp of the last buffer to have been received,
or in the case of the first buffer the timestamp taken at
start_streaming.
This results in a race where the current frame may have started
before we take the start time, which results in the first encoded
frame having an earlier timestamp than the header bytes.

Ensure that we never return a negative delta to the user by checking
against the previous timestamp.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
---
 drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 5 +++++
 1 file changed, 5 insertions(+)

--
2.7.4

Comments

Nicolas Dufresne June 27, 2019, 8:01 p.m. UTC | #1
Le jeudi 27 juin 2019 à 20:56 +0200, Stefan Wahren a écrit :
> From: Dave Stevenson <dave.stevenson@raspberrypi.org>
> 
> There is an awkward situation with H264 header bytes. Currently
> they are returned with a PTS of 0 because they aren't associated
> with a timestamped frame to encode. These are handled by either
> returning the timestamp of the last buffer to have been received,
> or in the case of the first buffer the timestamp taken at
> start_streaming.
> This results in a race where the current frame may have started
> before we take the start time, which results in the first encoded
> frame having an earlier timestamp than the header bytes.
> 
> Ensure that we never return a negative delta to the user by checking
> against the previous timestamp.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> ---
>  drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> index 9967df9..6205793 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> @@ -387,6 +387,11 @@ static void buffer_cb(struct vchiq_mmal_instance *instance,
>  			 ktime_to_ns(dev->capture.kernel_start_ts),
>  			 dev->capture.vc_start_timestamp, pts,
>  			 ktime_to_ns(timestamp));
> +		if (timestamp < dev->capture.last_timestamp) {
> +			v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev,
> +				 "Negative delta - using last time\n");
> +			timestamp = dev->capture.last_timestamp;
> +		}

Instead of that, maybe you could request a minimum number of buffers,
and not let the header buffer go until you have a proper "following
timestamp" to give it. This way you don't need this hack, and you won't
have an off-by-one in timestamps.

>  		buf->vb.vb2_buf.timestamp = ktime_to_ns(timestamp);
>  	} else {
>  		if (dev->capture.last_timestamp) {
> --
> 2.7.4
>
diff mbox series

Patch

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index 9967df9..6205793 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -387,6 +387,11 @@  static void buffer_cb(struct vchiq_mmal_instance *instance,
 			 ktime_to_ns(dev->capture.kernel_start_ts),
 			 dev->capture.vc_start_timestamp, pts,
 			 ktime_to_ns(timestamp));
+		if (timestamp < dev->capture.last_timestamp) {
+			v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev,
+				 "Negative delta - using last time\n");
+			timestamp = dev->capture.last_timestamp;
+		}
 		buf->vb.vb2_buf.timestamp = ktime_to_ns(timestamp);
 	} else {
 		if (dev->capture.last_timestamp) {