-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
discard buffered frames for < 1fps #470
Conversation
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed "Late frames should be discarded." since this PR discards late frames.
return err | ||
} | ||
|
||
c.prevFrameTime = time.Now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ReportPatch coverage has no change and project coverage change:
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/driver/camera/camera_linux.go
Outdated
@@ -23,6 +24,7 @@ import ( | |||
const ( | |||
maxEmptyFrameCount = 5 | |||
prioritizedDevice = "video0" | |||
bufferedFrameCount = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -70,6 +72,7 @@ type camera struct { | |||
started bool | |||
mutex sync.Mutex | |||
cancel func() | |||
prevFrameTime time.Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 setDiscardFramesOlderThan
. - We could add a generic method for setting properties to the
Driver
interface. Something likeSet(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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prop.Video would work as the input to this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it needs to be a constraint since that's a selection criteria.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh lame, okay!
pkg/driver/camera/camera_linux.go
Outdated
} | ||
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can factor this logic into the above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/driver/camera/camera_linux.go
Outdated
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do this if if p.DiscardFramesOlderThan != 0 {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we can remove that loop
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
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
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.