Repository: pion/mediadevices (go)

Issue:
GetUserMedia doesn't close driver if request is partially successed
When calling GetUserMedia with audio and video constraints, if one of audio/video failed to be initialized, other one is left opened. Then, the next GetUserMedia call always fail with `invalid state: driver is already opened`.

Pull request:
Close drivers on stream initialization failure
In GetUserMedia and GetDisplayMedia, driver was left opened if
initialization failure. Since Trackers are not returned on error,
there was no way to close them.
This commit closes Trackers on GetUserMedia/GetDisplayMedia failure.

### Reference issue
Fixes #74


Patch:
diff --git a/mediadevices.go b/mediadevices.go
index 04c705c..72edf74 100644
--- a/mediadevices.go
+++ b/mediadevices.go
@@ -82,6 +82,12 @@ func WithTrackGenerator(gen TrackGenerator) MediaDevicesOption {
 func (m *mediaDevices) GetDisplayMedia(constraints MediaStreamConstraints) (MediaStream, error) {
 	trackers := make([]Tracker, 0)
 
+	cleanTrackers := func() {
+		for _, t := range trackers {
+			t.Stop()
+		}
+	}
+
 	var videoConstraints MediaTrackConstraints
 	if constraints.Video != nil {
 		constraints.Video(&videoConstraints)
@@ -90,6 +96,7 @@ func (m *mediaDevices) GetDisplayMedia(constraints MediaStreamConstraints) (Medi
 	if videoConstraints.Enabled {
 		tracker, err := m.selectScreen(videoConstraints)
 		if err != nil {
+			cleanTrackers()
 			return nil, err
 		}
 
@@ -98,6 +105,7 @@ func (m *mediaDevices) GetDisplayMedia(constraints MediaStreamConstraints) (Medi
 
 	s, err := NewMediaStream(trackers...)
 	if err != nil {
+		cleanTrackers()
 		return nil, err
 	}
 
@@ -111,6 +119,12 @@ func (m *mediaDevices) GetUserMedia(constraints MediaStreamConstraints) (MediaSt
 	// TODO: It should return media stream based on constraints
 	trackers := make([]Tracker, 0)
 
+	cleanTrackers := func() {
+		for _, t := range trackers {
+			t.Stop()
+		}
+	}
+
 	var videoConstraints, audioConstraints MediaTrackConstraints
 	if constraints.Video != nil {
 		constraints.Video(&videoConstraints)
@@ -123,6 +137,7 @@ func (m *mediaDevices) GetUserMedia(constraints MediaStreamConstraints) (MediaSt
 	if videoConstraints.Enabled {
 		tracker, err := m.selectVideo(videoConstraints)
 		if err != nil {
+			cleanTrackers()
 			return nil, err
 		}
 
@@ -132,6 +147,7 @@ func (m *mediaDevices) GetUserMedia(constraints MediaStreamConstraints) (MediaSt
 	if audioConstraints.Enabled {
 		tracker, err := m.selectAudio(audioConstraints)
 		if err != nil {
+			cleanTrackers()
 			return nil, err
 		}
 
@@ -140,6 +156,7 @@ func (m *mediaDevices) GetUserMedia(constraints MediaStreamConstraints) (MediaSt
 
 	s, err := NewMediaStream(trackers...)
 	if err != nil {
+		cleanTrackers()
 		return nil, err
 	}
 
diff --git a/pkg/driver/camera/camera_linux.go b/pkg/driver/camera/camera_linux.go
index ec30832..e6cabf9 100644
--- a/pkg/driver/camera/camera_linux.go
+++ b/pkg/driver/camera/camera_linux.go
@@ -123,6 +123,8 @@ func (c *camera) VideoRecord(p prop.Media) (video.Reader, error) {
 		return nil, err
 	}
 
+	cam := c.cam
+
 	ctx, cancel := context.WithCancel(context.Background())
 	c.cancel = cancel
 	var buf []byte
@@ -138,7 +140,7 @@ func (c *camera) VideoRecord(p prop.Media) (video.Reader, error) {
 				return nil, io.EOF
 			}
 
-			err := c.cam.WaitForFrame(5) // 5 seconds
+			err := cam.WaitForFrame(5) // 5 seconds
 			switch err.(type) {
 			case nil:
 			case *webcam.Timeout:
@@ -148,7 +150,7 @@ func (c *camera) VideoRecord(p prop.Media) (video.Reader, error) {
 				return nil, err
 			}
 
-			b, err := c.cam.ReadFrame()
+			b, err := cam.ReadFrame()
 			if err != nil {
 				// Camera has been stopped.
 				return nil, err
diff --git a/pkg/driver/screen/x11_linux.go b/pkg/driver/screen/x11_linux.go
index 12b3559..c07db90 100644
--- a/pkg/driver/screen/x11_linux.go
+++ b/pkg/driver/screen/x11_linux.go
@@ -66,10 +66,11 @@ func (s *screen) VideoRecord(p prop.Media) (video.Reader, error) {
 	s.tick = time.NewTicker(time.Duration(float32(time.Second) / p.FrameRate))
 
 	var dst image.RGBA
+	reader := s.reader
 
 	r := video.ReaderFunc(func() (image.Image, error) {
 		<-s.tick.C
-		return s.reader.Read().ToRGBA(&dst), nil
+		return reader.Read().ToRGBA(&dst), nil
 	})
 	return r, nil
 }
diff --git a/pkg/driver/wrapper.go b/pkg/driver/wrapper.go
index a751415..ea2a46d 100644
--- a/pkg/driver/wrapper.go
+++ b/pkg/driver/wrapper.go
@@ -79,6 +79,9 @@ func (w *adapterWrapper) VideoRecord(p prop.Media) (r video.Reader, err error) {
 		r, err = w.VideoRecorder.VideoRecord(p)
 		return err
 	})
+	if err != nil {
+		_ = w.Close()
+	}
 	return
 }
 
diff --git a/track.go b/track.go
index 4b23348..8b49642 100644
--- a/track.go
+++ b/track.go
@@ -139,6 +139,7 @@ func newVideoTrack(opts *MediaDevicesOptions, d driver.Driver, constraints Media
 
 	encoder, err := codec.BuildVideoEncoder(r, constraints.Media)
 	if err != nil {
+		_ = d.Close()
 		return nil, err
 	}
 
@@ -214,6 +215,7 @@ func newAudioTrack(opts *MediaDevicesOptions, d driver.Driver, constraints Media
 
 	encoder, err := codec.BuildAudioEncoder(reader, constraints.Media)
 	if err != nil {
+		_ = d.Close()
 		return nil, err
 	}
 


Test patch:
diff --git a/mediadevices_test.go b/mediadevices_test.go
new file mode 100644
index 0000000..8ef1239
--- /dev/null
+++ b/mediadevices_test.go
@@ -0,0 +1,184 @@
+package mediadevices
+
+import (
+	"errors"
+	"io"
+	"testing"
+	"time"
+
+	"github.com/pion/webrtc/v2"
+	"github.com/pion/webrtc/v2/pkg/media"
+
+	"github.com/pion/mediadevices/pkg/codec"
+	_ "github.com/pion/mediadevices/pkg/driver/audiotest"
+	_ "github.com/pion/mediadevices/pkg/driver/videotest"
+	"github.com/pion/mediadevices/pkg/io/audio"
+	"github.com/pion/mediadevices/pkg/io/video"
+	"github.com/pion/mediadevices/pkg/prop"
+)
+
+func TestGetUserMedia(t *testing.T) {
+	codec.Register("MockVideo", codec.VideoEncoderBuilder(func(r video.Reader, p prop.Media) (io.ReadCloser, error) {
+		if p.BitRate == 0 {
+			// This is a dummy error to test the failure condition.
+			return nil, errors.New("wrong codec parameter")
+		}
+		return &mockVideoCodec{
+			r:      r,
+			closed: make(chan struct{}),
+		}, nil
+	}))
+	codec.Register("MockAudio", codec.AudioEncoderBuilder(func(r audio.Reader, p prop.Media) (io.ReadCloser, error) {
+		return &mockAudioCodec{
+			r:      r,
+			closed: make(chan struct{}),
+		}, nil
+	}))
+
+	md := NewMediaDevicesFromCodecs(
+		map[webrtc.RTPCodecType][]*webrtc.RTPCodec{
+			webrtc.RTPCodecTypeVideo: []*webrtc.RTPCodec{
+				&webrtc.RTPCodec{Type: webrtc.RTPCodecTypeVideo, Name: "MockVideo", PayloadType: 1},
+			},
+			webrtc.RTPCodecTypeAudio: []*webrtc.RTPCodec{
+				&webrtc.RTPCodec{Type: webrtc.RTPCodecTypeAudio, Name: "MockAudio", PayloadType: 2},
+			},
+		},
+		WithTrackGenerator(
+			func(_ uint8, _ uint32, id, _ string, codec *webrtc.RTPCodec) (
+				LocalTrack, error,
+			) {
+				return newMockTrack(codec, id), nil
+			},
+		),
+	)
+	constraints := MediaStreamConstraints{
+		Video: func(c *MediaTrackConstraints) {
+			c.CodecName = "MockVideo"
+			c.Enabled = true
+			c.Width = 640
+			c.Height = 480
+			c.BitRate = 100000
+		},
+		Audio: func(c *MediaTrackConstraints) {
+			c.CodecName = "MockAudio"
+			c.Enabled = true
+			c.BitRate = 32000
+		},
+	}
+	constraintsWrong := MediaStreamConstraints{
+		Video: func(c *MediaTrackConstraints) {
+			c.CodecName = "MockVideo"
+			c.Enabled = true
+			c.Width = 640
+			c.Height = 480
+			c.BitRate = 0
+		},
+		Audio: func(c *MediaTrackConstraints) {
+			c.CodecName = "MockAudio"
+			c.Enabled = true
+			c.BitRate = 32000
+		},
+	}
+
+	// GetUserMedia with broken parameters
+	ms, err := md.GetUserMedia(constraintsWrong)
+	if err == nil {
+		t.Fatal("Expected error, but got nil")
+	}
+
+	// GetUserMedia with correct parameters
+	ms, err = md.GetUserMedia(constraints)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+	tracks := ms.GetTracks()
+	if l := len(tracks); l != 2 {
+		t.Fatalf("Number of the tracks is expected to be 2, got %d", l)
+	}
+	for _, track := range tracks {
+		track.OnEnded(func(err error) {
+			if err != io.EOF {
+				t.Errorf("OnEnded called: %v", err)
+			}
+		})
+	}
+	time.Sleep(50 * time.Millisecond)
+
+	for _, track := range tracks {
+		track.Stop()
+	}
+
+	// Stop and retry GetUserMedia
+	ms, err = md.GetUserMedia(constraints)
+	if err != nil {
+		t.Fatalf("Failed to GetUserMedia after the previsous tracks stopped: %v", err)
+	}
+	tracks = ms.GetTracks()
+	if l := len(tracks); l != 2 {
+		t.Fatalf("Number of the tracks is expected to be 2, got %d", l)
+	}
+	for _, track := range tracks {
+		track.OnEnded(func(err error) {
+			if err != io.EOF {
+				t.Errorf("OnEnded called: %v", err)
+			}
+		})
+	}
+	time.Sleep(50 * time.Millisecond)
+}
+
+type mockTrack struct {
+	codec *webrtc.RTPCodec
+	id    string
+}
+
+func newMockTrack(codec *webrtc.RTPCodec, id string) *mockTrack {
+	return &mockTrack{
+		codec: codec,
+		id:    id,
+	}
+}
+
+func (t *mockTrack) WriteSample(s media.Sample) error {
+	return nil
+}
+
+func (t *mockTrack) Codec() *webrtc.RTPCodec {
+	return t.codec
+}
+
+func (t *mockTrack) ID() string {
+	return t.id
+}
+
+func (t *mockTrack) Kind() webrtc.RTPCodecType {
+	return t.codec.Type
+}
+
+type mockVideoCodec struct {
+	r      video.Reader
+	closed chan struct{}
+}
+
+func (m *mockVideoCodec) Read(b []byte) (int, error) {
+	if _, err := m.r.Read(); err != nil {
+		return 0, err
+	}
+	return len(b), nil
+}
+func (m *mockVideoCodec) Close() error { return nil }
+
+type mockAudioCodec struct {
+	r      audio.Reader
+	closed chan struct{}
+}
+
+func (m *mockAudioCodec) Read(b []byte) (int, error) {
+	buf := make([][2]float32, 100)
+	if _, err := m.r.Read(buf); err != nil {
+		return 0, err
+	}
+	return len(b), nil
+}
+func (m *mockAudioCodec) Close() error { return nil }
diff --git a/pkg/driver/audiotest/dummy.go b/pkg/driver/audiotest/dummy.go
index f36893e..31e5f2f 100644
--- a/pkg/driver/audiotest/dummy.go
+++ b/pkg/driver/audiotest/dummy.go
@@ -44,9 +44,11 @@ func (d *dummy) AudioRecord(p prop.Media) (audio.Reader, error) {
 	nextReadTime := time.Now()
 	var phase int
 
+	closed := d.closed
+
 	reader := audio.ReaderFunc(func(samples [][2]float32) (int, error) {
 		select {
-		case <-d.closed:
+		case <-closed:
 			return 0, io.EOF
 		default:
 		}
diff --git a/pkg/driver/videotest/dummy.go b/pkg/driver/videotest/dummy.go
index c0a1c2f..c734136 100644
--- a/pkg/driver/videotest/dummy.go
+++ b/pkg/driver/videotest/dummy.go
@@ -99,16 +99,18 @@ func (d *dummy) VideoRecord(p prop.Media) (video.Reader, error) {
 	}
 	random := rand.New(rand.NewSource(0))
 
-	d.tick = time.NewTicker(time.Duration(float32(time.Second) / p.FrameRate))
+	tick := time.NewTicker(time.Duration(float32(time.Second) / p.FrameRate))
+	d.tick = tick
+	closed := d.closed
 
 	r := video.ReaderFunc(func() (image.Image, error) {
 		select {
-		case <-d.closed:
+		case <-closed:
 			return nil, io.EOF
 		default:
 		}
 
-		<-d.tick.C
+		<-tick.C
 
 		copy(yy, yyBase)
 		copy(cb, cbBase)