DATA-764 RDK blocks user-specified video path on reconnect #1595
Conversation
bazile-clyde
changed the title
kharijarrett
added
the
safe to test
| return cam, err | ||
| } | ||
|
|
||
| utils2.PanicCapturingGo(func() { |
| } | ||
|
|
||
| utils2.PanicCapturingGo(func() { | ||
| ticker := time.NewTicker(time.Second / 2) |
There was a problem hiding this comment.
[nit] 500*time.Millisecond
| utils2.PanicCapturingGo(func() { | ||
| ticker := time.NewTicker(time.Second / 2) | ||
| defer ticker.Stop() | ||
| defer utils2.TryClose(ctx, cam) |
There was a problem hiding this comment.
Is this double close okay?
There was a problem hiding this comment.
Is this still an issue now that I'm using cam.Close directly? I'm not sure I understand what's double-closed here. If this is in reference to the close done by getStatusVideoSource, getStatusVideoSource now removed.
| defer utils2.TryClose(ctx, cam) | ||
|
|
||
| for { | ||
| select { |
There was a problem hiding this comment.
in the for loop you should use the goutils.SelectContextOrWaitChan or SelectContextOrWait. The ticker isn't very necessary making the latter usage easier. The risk of the select as it is is that there's a 50/50 chance of each case happening if both case are eligible
There was a problem hiding this comment.
Oh fancy. Done!
| constraints mediadevices.MediaStreamConstraints, | ||
| logger golog.Logger, | ||
| ) (driver.State, error) { | ||
| name := resolveSourceName(path, fromLabel) |
There was a problem hiding this comment.
can we call this once to avoid the extra syscalls?
There was a problem hiding this comment.
removed. No longer needed.
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| if _, err := getStatusVideoSource(attrs.Path, false, constraints, logger); err != nil { |
There was a problem hiding this comment.
What's going to reopen the camera if this closes it behind other caller's backs? This goroutine also needs to respect the Close of the camera resource itself such that it waits to shut this goroutine down.
There was a problem hiding this comment.
The first question shouldn't be an issue anymore since I'm no longer using getStatusVideoSource to get the source. I'm not sure if the second part is still relevant though.
As far as re-opening the camera in general that'll be done in DATA-597.
viambot
added
safe to test
bazile-clyde
force-pushed
the
DATA-764
branch
from
6fd9b33
to
4bb022d
Compare
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
@bhaney and @kharijarrett the gostream changes have been merged. This can be thoroughly reviewed now. |
| // mediadevices connects to the OS to get the properties for a driver. If the OS no longer knows | ||
| // about a specific driver then properties will be empty, and we can safely assume the driver no | ||
| // longer exists and should be closed. | ||
| if len(gostream.PropertiesFromMediaSource[image.Image, prop.Video](src)) == 0 { |
There was a problem hiding this comment.
For gostream.PropertiesFromMediaSource[T, U any](gostream.MediaSource[T]) there's no way to derive U from MediaSource[T] and the compiler complains of this. Because of that I pass the type information explicitly.
| src := camera.SourceFromCamera(cam) | ||
| defer func() { | ||
| if err := cam.Close(ctx); err != nil { | ||
| logger.Debugf("failed to close camera: %v", err) |
There was a problem hiding this comment.
Debugw(msg, "error", err) instead
viambot
added
safe to test
viambot
added
safe to test
| } | ||
|
|
||
| goutils.PanicCapturingGo(func() { | ||
| src := camera.SourceFromCamera(cam) |
There was a problem hiding this comment.
Very solid. We love a good fix! My only thing is maybe SourceFromCamera should return an error instead of just a nil src (when things go wrong). Then also, maybe check that error in here and handle it however if you can't get a source from ya camera.
There was a problem hiding this comment.
Good call. Done!
viambot
added
safe to test
|
Note: merge base coverage results not available, comparing against closest 6fe86df~1=(f70bc29) instead
|
| if asSrc, ok := cam.(*videoSource); ok { | ||
| return asSrc.videoSource, nil |
There was a problem hiding this comment.
camera's already fulfill the VideoSource interface too, you could also just use cam.(gostream.VideoSource) directly. Unless you want to get rid of all the extra camera methods on purpose
There was a problem hiding this comment.
So it compiles but it does not work and initially I was confused as to why it wouldn't. I printed out the type info to help my understanding.
fmt.Printf("cam: %T\n", cam)
fmt.Printf("asSrc: %T\n", asSrc)
fmt.Printf("asSrc.videoSource: %T\n", asSrc.videoSource)
...
cam: *camera.videoSource
asSrc: *camera.videoSource
asSrc.videoSource: *gostream.mediaSource[image.Image,github.com/pion/mediadevices/pkg/prop.Video]
What we ultimately need is the last type, a gostream.mediaSource[image.Image, prop.Video] which contains the driver we'll eventually query to retrieve its properties. We can't access any of the internals of camera.Camera (it's just an interface) so the chain of casting calls is used to cast that interface into types we know the internals of.
Trying to do all of this in one function wouldn't work either since that would require gostream to know about the private struct camera.videoSource.
This commit must be merged after the necessary changes in gostream are pushed to master (see edaniels/gostream#15).
This commit sets up a go routine to monitor the status a video driver at the path specified by the user. If that webcam is unplugged this go routine will close the associated driver within at most a half second. To verify this works
assuming your local copy of gostream is located at
/home/pi/gostream.The
video_pathshould be setvideo0or another preferred path.video0or your specified path from step 2:video0(andvideo1) in this example.Note, you will not be able to stream video from the app after the camera is plugged back in. This will be resolved in DATA-597