discard buffered frames for < 1fps #470

bazile-clyde · 2023-02-02T21:46:40Z

Problem Description

This library sets a buffer size of 1 upon opening cameras in Linux. This will always result in stale frames. For streaming this isn't a big deal since the staleness of a frame could be as little as about 30ms assuming a camera is capable of streaming at 30fps or even less for higher frame rates.

For taking snapshots this is much more problematic. Consider the following use case: a user wants to take a snapshot once a day at noon. Every image would be a day behind. That is, Monday's snapshot would be from Sunday, Sunday's snapshot would be from Saturday, and so forth.

Simply making an extra call in the application wrapping this one is difficult since

the size of the buffer differs by OS
the wrapping application would then depend on the buffer size, which may change over time
the reader function this library returns does not distinguish between snapshots and streams.

I do not think it’s possible to add unbuffered support to blackjack/webcam either. According to the V4L spec (link) there are three ways to read or stream frames from cameras:

Method 2 is what blackjack uses currently. Both methods 2 and 3 require the allocation of buffers. Method 1 doesn’t use buffers but it isn’t as widely supported as 2 and usually not supported for USB webcams. To further drive home this point, the following is what the V4L spec says about reading frames from cameras with read/write functions

It is considered inferior though because no meta-information like frame counters or timestamps are passed. This information is necessary to recognize frame dropping and to synchronize with other data streams.

In short, read/write is bad and isn’t widely supported. Streaming using mmap is good and is widely supported.

Solution

To fix this issue I made a simple assumption: if the user is reading frames at <1fps they are taking snapshots and not streaming. The solution then is to save a timestamp of the last time we read a frame from the camera. If it was more than a second ago, clear the buffer. If it was less than a second ago, it's business as usual. This adds virtually no overhead for streaming but does add a small amount for taking snapshots. Testing this locally with a C270 USB webcam resulted in <4ms of additional latency per snapshot. If you're taking snapshots every second or greater this extra overhead is negligible.

If there are better ways to fix this problem please let me know. I also plan to investigate ways to fix the same issue on Darwin which has a buffer size of 3.

Note: The example code for streaming MJPEG images in blackjack/webcam solves this issue by discarding the first frame it receives: the stale image. See their code here and here. That seems to be the workaround.

bazile-clyde · 2023-02-02T21:47:21Z

pkg/driver/camera/camera_linux.go

@@ -158,8 +161,13 @@ func (c *camera) Open() error {
 		return err
 	}

-	// Late frames should be discarded. Buffering should be handled in higher level.
-	cam.SetBufferCount(1)
+	// Buffering should be handled in higher level.


Removed "Late frames should be discarded." since this PR discards late frames.

bazile-clyde · 2023-02-02T21:49:15Z

pkg/driver/camera/camera_linux.go

+		return err
+	}
+
+	c.prevFrameTime = time.Now()


This will cause the first call to the reader function to discard a stale frame if needed. That is, a frame from more than a second ago. For streaming at >1fps this will only happen once to get the stream on track then never again.

codecov · 2023-02-02T21:49:46Z

Codecov Report

Patch coverage has no change and project coverage change: +0.23 🎉

Comparison is base (76ba048) 58.08% compared to head (2964ae2) 58.32%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
+ Coverage   58.08%   58.32%   +0.23%     
==========================================
  Files          62       62              
  Lines        3691     3736      +45     
==========================================
+ Hits         2144     2179      +35     
- Misses       1420     1430      +10     
  Partials      127      127

Impacted Files	Coverage Δ
pkg/driver/camera/camera_linux.go	`28.57% <0.00%> (-1.87%)`	⬇️
pkg/prop/prop.go	`74.00% <ø> (+1.33%)`	⬆️
pkg/codec/vpx/vpx.go	`83.59% <0.00%> (+0.08%)`	⬆️
pkg/codec/x264/x264.go	`65.51% <0.00%> (+0.40%)`	⬆️
pkg/codec/openh264/openh264.go	`81.69% <0.00%> (+0.53%)`	⬆️
pkg/io/video/convert_cgo.go	`71.23% <0.00%> (+2.57%)`	⬆️
pkg/io/video/convert.go	`72.80% <0.00%> (+6.13%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

edaniels

I like this change as a workaround for the reasons you specified that make it hard to implement elsewhere. I just propose we make this opt in since we don't know what expectations others already have on this library.

edaniels · 2023-03-03T15:06:22Z

pkg/driver/camera/camera_linux.go

@@ -23,6 +24,7 @@ import (
 const (
 	maxEmptyFrameCount = 5
 	prioritizedDevice  = "video0"
+	bufferedFrameCount = 1


instead of making this a const, we know that we don't really ever want to be buffering more than this, so I think we can keep the 1 below

This const is used for the number of reads we need to perform to clear the buffer. If this wasn't here we'd risk SetBufferCount and the clear buffer logic becoming out of sync.

but it's always 1. that's why the loop is also unnecessary below. I think it's fine to keep fixed as 1 in both other parts of code.

There's a comment above SetBufferCount that suggest it may be eliminated in the future so it may not always be 1 and might go away. If that happens then the context will be lost for why we're flushing a single frame. Either way, I don't have a strong opinion on this so removed.

edaniels · 2023-03-03T15:11:12Z

pkg/driver/camera/camera_linux.go

@@ -70,6 +72,7 @@ type camera struct {
 	started         bool
 	mutex           sync.Mutex
 	cancel          func()
+	prevFrameTime   time.Time


Instead of making an assumption for users of this library, how would you feel about exposing an option called DiscardFramesOlderThan time.Duration which would still utilize the logic you've added here?

I like that idea! I'm not sure how it'd be best implemented but I have a few ideas:

We could make this struct public so users can cast their Driver interface into this concrete type and set DiscardFramesOlderThan.

We could add a generic method for setting properties to the Driver interface. Something like Set(prop map[string]interface{})

Add a non-const global variable.

I think 2 would make the most sense Driver isn't camera specific. All would require locking the mutex which isn't exposed, so 1 and 3 seems dangerous unless you expose a mutex.

prop.Video would work as the input to this function.

Cool. I added the option to prop.Video and prop.VideoConstraints. Thinking about this a bit more we wouldn't need a second function. We could set the option on the prop.Media passed to camera.VideoRecord. Updated this code to do so. Tested this manually locally and it worked just as well.

I don't think it needs to be a constraint since that's a selection criteria.

prop.Video and prop.VideoConstraints are merged in prop.go::merge using reflections. This is only possible because the two structs are identical. If they differ we'll get a reflect: Field index out of range error unless we handle this field specially in that function.

oh lame, okay!

pkg/driver/camera/camera_linux.go

edaniels · 2023-03-03T15:12:03Z

pkg/driver/camera/camera_linux.go

+	}
+
+	r := video.ReaderFunc(func() (img image.Image, release func(), err error) {
+		// If frames are requested at less than 1fps, assume snapshots, not streaming, and discard all


I think you can factor this logic into the above

edaniels · 2023-03-06T20:57:39Z

pkg/driver/camera/camera_linux.go

@@ -243,6 +260,7 @@ func (c *camera) VideoRecord(p prop.Media) (video.Reader, error) {
 				// Camera has been stopped.
 				return nil, func() {}, err
 			}
+			c.prevFrameTime = time.Now()


let's do this if if p.DiscardFramesOlderThan != 0 {

edaniels

Think we can remove that loop

discard buffered frames for < 1fps

49f90b4

bazile-clyde commented Feb 2, 2023

View reviewed changes

edaniels requested changes Mar 3, 2023

View reviewed changes

allow users to set discard after time

f3659f5

bazile-clyde requested a review from edaniels March 6, 2023 20:14

edaniels reviewed Mar 6, 2023

View reviewed changes

edaniels requested changes Mar 6, 2023

View reviewed changes

address comments

2964ae2

bazile-clyde requested a review from edaniels March 6, 2023 21:43

edaniels approved these changes Mar 6, 2023

View reviewed changes

edaniels merged commit 62009a8 into pion:master Mar 6, 2023

bazile-clyde deleted the discard-buffered-frames branch March 6, 2023 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discard buffered frames for < 1fps #470

discard buffered frames for < 1fps #470

bazile-clyde commented Feb 2, 2023 •

edited

bazile-clyde Feb 2, 2023 •

edited

bazile-clyde Feb 2, 2023 •

edited

codecov bot commented Feb 2, 2023 •

edited

edaniels left a comment

edaniels Mar 3, 2023

bazile-clyde Mar 6, 2023

edaniels Mar 6, 2023

bazile-clyde Mar 6, 2023

edaniels Mar 3, 2023

bazile-clyde Mar 3, 2023

edaniels Mar 4, 2023

bazile-clyde Mar 6, 2023

edaniels Mar 6, 2023

bazile-clyde Mar 6, 2023

edaniels Mar 6, 2023

edaniels Mar 3, 2023

bazile-clyde Mar 6, 2023

edaniels Mar 6, 2023

bazile-clyde Mar 6, 2023

edaniels left a comment

discard buffered frames for < 1fps #470

discard buffered frames for < 1fps #470

Conversation

bazile-clyde commented Feb 2, 2023 • edited

Problem Description

Solution

bazile-clyde Feb 2, 2023 • edited

Choose a reason for hiding this comment

bazile-clyde Feb 2, 2023 • edited

Choose a reason for hiding this comment

codecov bot commented Feb 2, 2023 • edited

Codecov Report

edaniels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edaniels left a comment

Choose a reason for hiding this comment

bazile-clyde commented Feb 2, 2023 •

edited

bazile-clyde Feb 2, 2023 •

edited

bazile-clyde Feb 2, 2023 •

edited

codecov bot commented Feb 2, 2023 •

edited