Allow double close of codecs (#364)

Video/AudioTrack.NewRTPReader() internally closes encoder on error.
It caused double free if user closes reader after error.

pkg/codec/internal/codectest/codectest.go

@@ -13,6 +13,15 @@ import (
 	"github.com/pion/mediadevices/pkg/wave"
 )
 
+func assertNoPanic(t *testing.T, fn func() error, msg string) error {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("panic: %v: %s", r, msg)
+		}
+	}()
+	return fn()
+}
+
 func AudioEncoderSimpleReadTest(t *testing.T, c codec.AudioEncoderBuilder, p prop.Media, w wave.Audio) {
 	var eof bool
 	enc, err := c.BuildAudioEncoder(audio.ReaderFunc(func() (wave.Audio, func(), error) {
@@ -78,3 +87,35 @@ func VideoEncoderSimpleReadTest(t *testing.T, c codec.VideoEncoderBuilder, p pro
 		t.Fatal(err)
 	}
 }
+
+func AudioEncoderCloseTwiceTest(t *testing.T, c codec.AudioEncoderBuilder, p prop.Media) {
+	enc, err := c.BuildAudioEncoder(audio.ReaderFunc(func() (wave.Audio, func(), error) {
+		return nil, nil, io.EOF
+	}), p)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := assertNoPanic(t, enc.Close, "on first Close()"); err != nil {
+		t.Fatal(err)
+	}
+	if err := assertNoPanic(t, enc.Close, "on second Close()"); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func VideoEncoderCloseTwiceTest(t *testing.T, c codec.VideoEncoderBuilder, p prop.Media) {
+	enc, err := c.BuildVideoEncoder(video.ReaderFunc(func() (image.Image, func(), error) {
+		return nil, nil, io.EOF
+	}), p)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := assertNoPanic(t, enc.Close, "on first Close()"); err != nil {
+		t.Fatal(err)
+	}
+	if err := assertNoPanic(t, enc.Close, "on second Close()"); err != nil {
+		t.Fatal(err)
+	}
+}

pkg/codec/mmal/mmal_test.go

@@ -29,4 +29,18 @@ func TestEncoder(t *testing.T) {
 			),
 		)
 	})
+	t.Run("CloseTwice", func(t *testing.T) {
+		p, err := NewParams()
+		if err != nil {
+			t.Fatal(err)
+		}
+		codectest.VideoEncoderCloseTwiceTest(t, &p, prop.Media{
+			Video: prop.Video{
+				Width:       640,
+				Height:      480,
+				FrameRate:   30,
+				FrameFormat: frame.FormatI420,
+			},
+		})
+	})
 }

pkg/codec/openh264/openh264.go

@@ -92,6 +92,10 @@ func (e *encoder) Close() error {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 
+	if e.closed {
+		return nil
+	}
+
 	e.closed = true
 
 	var rv C.int

pkg/codec/openh264/openh264_test.go

@@ -29,4 +29,18 @@ func TestEncoder(t *testing.T) {
 			),
 		)
 	})
+	t.Run("CloseTwice", func(t *testing.T) {
+		p, err := NewParams()
+		if err != nil {
+			t.Fatal(err)
+		}
+		codectest.VideoEncoderCloseTwiceTest(t, &p, prop.Media{
+			Video: prop.Video{
+				Width:       640,
+				Height:      480,
+				FrameRate:   30,
+				FrameFormat: frame.FormatI420,
+			},
+		})
+	})
 }

pkg/codec/opus/opus.go

@@ -126,6 +126,9 @@ func (e *encoder) ForceKeyFrame() error {
 }
 
 func (e *encoder) Close() error {
+	if e.engine == nil {
+		return nil
+	}
 	C.opus_encoder_destroy(e.engine)
 	e.engine = nil
 	return nil

pkg/codec/opus/opus_test.go

@@ -1,10 +1,11 @@
 package opus
 
 import (
+	"testing"
+
 	"github.com/pion/mediadevices/pkg/codec/internal/codectest"
 	"github.com/pion/mediadevices/pkg/prop"
 	"github.com/pion/mediadevices/pkg/wave"
-	"testing"
 )
 
 func TestEncoder(t *testing.T) {
@@ -27,4 +28,16 @@ func TestEncoder(t *testing.T) {
 			}),
 		)
 	})
+	t.Run("CloseTwice", func(t *testing.T) {
+		p, err := NewParams()
+		if err != nil {
+			t.Fatal(err)
+		}
+		codectest.AudioEncoderCloseTwiceTest(t, &p, prop.Media{
+			Audio: prop.Audio{
+				SampleRate:   48000,
+				ChannelCount: 2,
+			},
+		})
+	})
 }

pkg/codec/vaapi/vaapi_test.go

@@ -51,6 +51,20 @@ func TestEncoder(t *testing.T) {
 					),
 				)
 			})
+			t.Run("CloseTwice", func(t *testing.T) {
+				p, err := factory()
+				if err != nil {
+					t.Fatal(err)
+				}
+				codectest.VideoEncoderCloseTwiceTest(t, p, prop.Media{
+					Video: prop.Video{
+						Width:       640,
+						Height:      480,
+						FrameRate:   30,
+						FrameFormat: frame.FormatI420,
+					},
+				})
+			})
 		})
 	}
 }

pkg/codec/vaapi/vp8.go

@@ -1,3 +1,4 @@
+//go:build dragonfly || freebsd || linux || netbsd || openbsd || solaris
 // +build dragonfly freebsd linux netbsd openbsd solaris
 
 package vaapi
@@ -552,6 +553,10 @@ func (e *encoderVP8) Close() error {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 
+	if e.closed {
+		return nil
+	}
+
 	C.vaDestroySurfaces(e.display, &e.surfs[0], C.int(len(e.surfs)))
 	C.vaDestroyContext(e.display, e.ctxID)
 	C.vaDestroyConfig(e.display, e.confID)

pkg/codec/vaapi/vp9.go

@@ -1,3 +1,4 @@
+//go:build dragonfly || freebsd || linux || netbsd || openbsd || solaris
 // +build dragonfly freebsd linux netbsd openbsd solaris
 
 package vaapi
@@ -487,6 +488,10 @@ func (e *encoderVP9) Close() error {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 
+	if e.closed {
+		return nil
+	}
+
 	C.vaDestroySurfaces(e.display, &e.surfs[0], C.int(len(e.surfs)))
 	C.vaDestroyContext(e.display, e.ctxID)
 	C.vaDestroyConfig(e.display, e.confID)

pkg/codec/vpx/vpx.go

@@ -310,6 +310,10 @@ func (e *encoder) Close() error {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 
+	if e.closed {
+		return nil
+	}
+
 	e.closed = true
 
 	C.free(unsafe.Pointer(e.raw))

pkg/codec/vpx/vpx_test.go

@@ -46,6 +46,20 @@ func TestEncoder(t *testing.T) {
 					),
 				)
 			})
+			t.Run("CloseTwice", func(t *testing.T) {
+				p, err := factory()
+				if err != nil {
+					t.Fatal(err)
+				}
+				codectest.VideoEncoderCloseTwiceTest(t, p, prop.Media{
+					Video: prop.Video{
+						Width:       640,
+						Height:      480,
+						FrameRate:   30,
+						FrameFormat: frame.FormatI420,
+					},
+				})
+			})
 		})
 	}
 }

pkg/codec/x264/x264_test.go

@@ -30,4 +30,19 @@ func TestEncoder(t *testing.T) {
 			),
 		)
 	})
+	t.Run("CloseTwice", func(t *testing.T) {
+		p, err := NewParams()
+		if err != nil {
+			t.Fatal(err)
+		}
+		p.BitRate = 200000
+		codectest.VideoEncoderCloseTwiceTest(t, &p, prop.Media{
+			Video: prop.Video{
+				Width:       640,
+				Height:      480,
+				FrameRate:   30,
+				FrameFormat: frame.FormatI420,
+			},
+		})
+	})
 }