Fix invalid constraints merging

mediadevices.go

@@ -217,8 +217,9 @@ func selectBestDriver(filter driver.FilterFn, constraints MediaTrackConstraints)
 		return nil, MediaTrackConstraints{}, errNotFound
 	}
 
-	constraints.selectedMedia = bestProp
-	constraints.selectedMedia.Merge(constraints.MediaConstraints)
+	constraints.selectedMedia = prop.Media{}
+	constraints.selectedMedia.MergeConstraints(constraints.MediaConstraints)
+	constraints.selectedMedia.Merge(bestProp)
 	return bestDriver, constraints, nil
 }
 

mediadevices_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/pion/webrtc/v2/pkg/media"
 
 	"github.com/pion/mediadevices/pkg/codec"
+	"github.com/pion/mediadevices/pkg/driver"
 	_ "github.com/pion/mediadevices/pkg/driver/audiotest"
 	_ "github.com/pion/mediadevices/pkg/driver/videotest"
 	"github.com/pion/mediadevices/pkg/io/audio"
@@ -32,11 +33,11 @@ func TestGetUserMedia(t *testing.T) {
 	}
 	md := NewMediaDevicesFromCodecs(
 		map[webrtc.RTPCodecType][]*webrtc.RTPCodec{
-			webrtc.RTPCodecTypeVideo: []*webrtc.RTPCodec{
-				&webrtc.RTPCodec{Type: webrtc.RTPCodecTypeVideo, Name: "MockVideo", PayloadType: 1},
+			webrtc.RTPCodecTypeVideo: {
+				{Type: webrtc.RTPCodecTypeVideo, Name: "MockVideo", PayloadType: 1},
 			},
-			webrtc.RTPCodecTypeAudio: []*webrtc.RTPCodec{
-				&webrtc.RTPCodec{Type: webrtc.RTPCodecTypeAudio, Name: "MockAudio", PayloadType: 2},
+			webrtc.RTPCodecTypeAudio: {
+				{Type: webrtc.RTPCodecTypeAudio, Name: "MockAudio", PayloadType: 2},
 			},
 		},
 		WithTrackGenerator(
@@ -122,6 +123,9 @@ func TestGetUserMedia(t *testing.T) {
 		})
 	}
 	time.Sleep(50 * time.Millisecond)
+	for _, track := range tracks {
+		track.Stop()
+	}
 }
 
 type mockTrack struct {
@@ -217,3 +221,56 @@ func (m *mockAudioCodec) Read(b []byte) (int, error) {
 	return len(b), nil
 }
 func (m *mockAudioCodec) Close() error { return nil }
+
+func TestSelectBestDriverConstraintsResultIsSetProperly(t *testing.T) {
+	filterFn := driver.FilterVideoRecorder()
+	drivers := driver.GetManager().Query(filterFn)
+	if len(drivers) == 0 {
+		t.Fatal("expect to get at least 1 driver")
+	}
+
+	driver := drivers[0]
+	err := driver.Open()
+	if err != nil {
+		t.Fatal("expect to open driver successfully")
+	}
+	defer driver.Close()
+
+	if len(driver.Properties()) == 0 {
+		t.Fatal("expect to get at least 1 property")
+	}
+	expectedProp := driver.Properties()[0]
+	// Since this is a continuous value, bestConstraints should be set with the value that user specified
+	expectedProp.FrameRate = 30.0
+
+	wantConstraints := MediaTrackConstraints{
+		MediaConstraints: prop.MediaConstraints{
+			VideoConstraints: prop.VideoConstraints{
+				// By reducing the width from the driver by a tiny amount, this property should be chosen.
+				// At the same time, we'll be able to find out if the return constraints will be properly set
+				// to the best constraints.
+				Width:       prop.Int(expectedProp.Width - 1),
+				Height:      prop.Int(expectedProp.Width),
+				FrameFormat: prop.FrameFormat(expectedProp.FrameFormat),
+				FrameRate:   prop.Float(30.0),
+			},
+		},
+	}
+
+	bestDriver, bestConstraints, err := selectBestDriver(filterFn, wantConstraints)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if driver != bestDriver {
+		t.Fatal("best driver is not expected")
+	}
+
+	s := bestConstraints.selectedMedia
+	if s.Width != expectedProp.Width ||
+		s.Height != expectedProp.Height ||
+		s.FrameFormat != expectedProp.FrameFormat ||
+		s.FrameRate != expectedProp.FrameRate {
+		t.Fatalf("failed to return best constraints\nexpected:\n%v\n\ngot:\n%v", expectedProp, bestConstraints.selectedMedia)
+	}
+}

pkg/prop/prop.go

@@ -22,8 +22,12 @@ type Media struct {
 	Audio
 }
 
-// Merge merges all the field values from o to p, except zero values.
-func (p *Media) Merge(o MediaConstraints) {
+// setterFn is a callback function to set value from fieldB to fieldA
+type setterFn func(fieldA, fieldB reflect.Value)
+
+// merge merges all the field values from o to p, except zero values. It's guaranteed that setterFn will be called
+// when fieldA and fieldB are not struct.
+func (p *Media) merge(o interface{}, set setterFn) {
 	rp := reflect.ValueOf(p).Elem()
 	ro := reflect.ValueOf(o)
 
@@ -49,36 +53,48 @@ func (p *Media) Merge(o MediaConstraints) {
 				continue
 			}
 
-			switch c := fieldB.Interface().(type) {
-			case IntConstraint:
-				if v, ok := c.Value(); ok {
-					fieldA.Set(reflect.ValueOf(v))
-				}
-			case FloatConstraint:
-				if v, ok := c.Value(); ok {
-					fieldA.Set(reflect.ValueOf(v))
-				}
-			case DurationConstraint:
-				if v, ok := c.Value(); ok {
-					fieldA.Set(reflect.ValueOf(v))
-				}
-			case FrameFormatConstraint:
-				if v, ok := c.Value(); ok {
-					fieldA.Set(reflect.ValueOf(v))
-				}
-			case StringConstraint:
-				if v, ok := c.Value(); ok {
-					fieldA.Set(reflect.ValueOf(v))
-				}
-			default:
-				panic("unsupported property type")
-			}
+			set(fieldA, fieldB)
 		}
 	}
 
 	merge(rp, ro)
 }
 
+func (p *Media) Merge(o Media) {
+	p.merge(o, func(fieldA, fieldB reflect.Value) {
+		fieldA.Set(fieldB)
+	})
+}
+
+func (p *Media) MergeConstraints(o MediaConstraints) {
+	p.merge(o, func(fieldA, fieldB reflect.Value) {
+		switch c := fieldB.Interface().(type) {
+		case IntConstraint:
+			if v, ok := c.Value(); ok {
+				fieldA.Set(reflect.ValueOf(v))
+			}
+		case FloatConstraint:
+			if v, ok := c.Value(); ok {
+				fieldA.Set(reflect.ValueOf(v))
+			}
+		case DurationConstraint:
+			if v, ok := c.Value(); ok {
+				fieldA.Set(reflect.ValueOf(v))
+			}
+		case FrameFormatConstraint:
+			if v, ok := c.Value(); ok {
+				fieldA.Set(reflect.ValueOf(v))
+			}
+		case StringConstraint:
+			if v, ok := c.Value(); ok {
+				fieldA.Set(reflect.ValueOf(v))
+			}
+		default:
+			panic("unsupported property type")
+		}
+	})
+}
+
 // FitnessDistance calculates fitness of media property and media constraints.
 // If no media satisfies given constraints, second return value will be false.
 func (p *MediaConstraints) FitnessDistance(o Media) (float64, bool) {

pkg/prop/prop_test.go

@@ -159,9 +159,9 @@ func TestMergeWithZero(t *testing.T) {
 		},
 	}
 
-	b := MediaConstraints{
-		VideoConstraints: VideoConstraints{
-			Height: Int(100),
+	b := Media{
+		Video: Video{
+			Height: 100,
 		},
 	}
 
@@ -183,9 +183,9 @@ func TestMergeWithSameField(t *testing.T) {
 		},
 	}
 
-	b := MediaConstraints{
-		VideoConstraints: VideoConstraints{
-			Width: Int(100),
+	b := Media{
+		Video: Video{
+			Width: 100,
 		},
 	}
 
@@ -209,9 +209,9 @@ func TestMergeNested(t *testing.T) {
 		},
 	}
 
-	b := MediaConstraints{
-		VideoConstraints: VideoConstraints{
-			Width: Int(100),
+	b := Media{
+		Video: Video{
+			Width: 100,
 		},
 	}
 
@@ -221,3 +221,73 @@ func TestMergeNested(t *testing.T) {
 		t.Error("expected a.Width to be 100, but got 0")
 	}
 }
+
+func TestMergeConstraintsWithZero(t *testing.T) {
+	a := Media{
+		Video: Video{
+			Width: 30,
+		},
+	}
+
+	b := MediaConstraints{
+		VideoConstraints: VideoConstraints{
+			Height: Int(100),
+		},
+	}
+
+	a.MergeConstraints(b)
+
+	if a.Width == 0 {
+		t.Error("expected a.Width to be 30, but got 0")
+	}
+
+	if a.Height == 0 {
+		t.Error("expected a.Height to be 100, but got 0")
+	}
+}
+
+func TestMergeConstraintsWithSameField(t *testing.T) {
+	a := Media{
+		Video: Video{
+			Width: 30,
+		},
+	}
+
+	b := MediaConstraints{
+		VideoConstraints: VideoConstraints{
+			Width: Int(100),
+		},
+	}
+
+	a.MergeConstraints(b)
+
+	if a.Width != 100 {
+		t.Error("expected a.Width to be 100, but got 0")
+	}
+}
+
+func TestMergeConstraintsNested(t *testing.T) {
+	type constraints struct {
+		Media
+	}
+
+	a := constraints{
+		Media{
+			Video: Video{
+				Width: 30,
+			},
+		},
+	}
+
+	b := MediaConstraints{
+		VideoConstraints: VideoConstraints{
+			Width: Int(100),
+		},
+	}
+
+	a.MergeConstraints(b)
+
+	if a.Width != 100 {
+		t.Error("expected a.Width to be 100, but got 0")
+	}
+}