Fix opus codec panic on read after close (#440)

Also add test to other codecs
pion · Aug 24, 2022 · 58dc90d · 58dc90d
1 parent 8ad810e
commit 58dc90d
Show file tree

Hide file tree

Showing 8 changed files with 165 additions and 0 deletions.
diff --git a/pkg/codec/internal/codectest/codectest.go b/pkg/codec/internal/codectest/codectest.go
@@ -119,3 +119,41 @@ func VideoEncoderCloseTwiceTest(t *testing.T, c codec.VideoEncoderBuilder, p pro
 		t.Fatal(err)
 	}
 }
+
+func AudioEncoderReadAfterCloseTest(t *testing.T, c codec.AudioEncoderBuilder, p prop.Media, w wave.Audio) {
+	enc, err := c.BuildAudioEncoder(audio.ReaderFunc(func() (wave.Audio, func(), error) {
+		return w, nil, nil
+	}), p)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := assertNoPanic(t, enc.Close, "on Close()"); err != nil {
+		t.Fatal(err)
+	}
+	if err := assertNoPanic(t, func() error {
+		_, _, err := enc.Read()
+		return err
+	}, "on Read()"); err != io.EOF {
+		t.Fatalf("Expected: %v, got: %v", io.EOF, err)
+	}
+}
+
+func VideoEncoderReadAfterCloseTest(t *testing.T, c codec.VideoEncoderBuilder, p prop.Media, img image.Image) {
+	enc, err := c.BuildVideoEncoder(video.ReaderFunc(func() (image.Image, func(), error) {
+		return img, nil, nil
+	}), p)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := assertNoPanic(t, enc.Close, "on Close()"); err != nil {
+		t.Fatal(err)
+	}
+	if err := assertNoPanic(t, func() error {
+		_, _, err := enc.Read()
+		return err
+	}, "on Read()"); err != io.EOF {
+		t.Fatalf("Expected: %v, got: %v", io.EOF, err)
+	}
+}
diff --git a/pkg/codec/mmal/mmal_test.go b/pkg/codec/mmal/mmal_test.go
@@ -44,6 +44,25 @@ func TestEncoder(t *testing.T) {
 			},
 		})
 	})
+	t.Run("ReadAfterClose", func(t *testing.T) {
+		p, err := NewParams()
+		if err != nil {
+			t.Fatal(err)
+		}
+		codectest.VideoEncoderReadAfterCloseTest(t, &p,
+			prop.Media{
+				Video: prop.Video{
+					Width:       256,
+					Height:      144,
+					FrameFormat: frame.FormatI420,
+				},
+			},
+			image.NewYCbCr(
+				image.Rect(0, 0, 256, 144),
+				image.YCbCrSubsampleRatio420,
+			),
+		)
+	})
 }
 
 func TestShouldImplementBitRateControl(t *testing.T) {

diff --git a/pkg/codec/openh264/openh264_test.go b/pkg/codec/openh264/openh264_test.go
@@ -60,4 +60,23 @@ func TestEncoder(t *testing.T) {
 			},
 		})
 	})
+	t.Run("ReadAfterClose", func(t *testing.T) {
+		p, err := NewParams()
+		if err != nil {
+			t.Fatal(err)
+		}
+		codectest.VideoEncoderReadAfterCloseTest(t, &p,
+			prop.Media{
+				Video: prop.Video{
+					Width:       256,
+					Height:      144,
+					FrameFormat: frame.FormatI420,
+				},
+			},
+			image.NewYCbCr(
+				image.Rect(0, 0, 256, 144),
+				image.YCbCrSubsampleRatio420,
+			),
+		)
+	})
 }
diff --git a/pkg/codec/opus/opus.go b/pkg/codec/opus/opus.go
@@ -3,6 +3,8 @@ package opus
 import (
 	"errors"
 	"fmt"
+	"io"
+	"sync"
 
 	"github.com/pion/mediadevices/pkg/codec"
 	"github.com/pion/mediadevices/pkg/io/audio"
@@ -25,6 +27,8 @@ type encoder struct {
 	inBuff wave.Audio
 	reader audio.Reader
 	engine *C.OpusEncoder
+
+	mu sync.Mutex
 }
 
 func newEncoder(r audio.Reader, p prop.Media, params Params) (codec.ReadCloser, error) {
@@ -79,6 +83,12 @@ func (e *encoder) Read() ([]byte, func(), error) {
 		return nil, func() {}, err
 	}
 
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	if e.engine == nil {
+		return nil, nil, io.EOF
+	}
+
 	encoded := make([]byte, 1024)
 	var n C.opus_int32
 	switch b := buff.(type) {
@@ -126,6 +136,8 @@ func (e *encoder) Controller() codec.EncoderController {
 }
 
 func (e *encoder) Close() error {
+	e.mu.Lock()
+	defer e.mu.Unlock()
 	if e.engine == nil {
 		return nil
 	}

diff --git a/pkg/codec/opus/opus_test.go b/pkg/codec/opus/opus_test.go
@@ -57,4 +57,23 @@ func TestEncoder(t *testing.T) {
 			},
 		})
 	})
+	t.Run("ReadAfterClose", func(t *testing.T) {
+		p, err := NewParams()
+		if err != nil {
+			t.Fatal(err)
+		}
+		codectest.AudioEncoderReadAfterCloseTest(t, &p,
+			prop.Media{
+				Audio: prop.Audio{
+					SampleRate:   48000,
+					ChannelCount: 2,
+				},
+			},
+			wave.NewInt16Interleaved(wave.ChunkInfo{
+				Len:          960,
+				SamplingRate: 48000,
+				Channels:     2,
+			}),
+		)
+	})
 }
diff --git a/pkg/codec/vaapi/vaapi_test.go b/pkg/codec/vaapi/vaapi_test.go
@@ -65,6 +65,25 @@ func TestEncoder(t *testing.T) {
 					},
 				})
 			})
+			t.Run("ReadAfterClose", func(t *testing.T) {
+				p, err := factory()
+				if err != nil {
+					t.Fatal(err)
+				}
+				codectest.VideoEncoderReadAfterCloseTest(t, p,
+					prop.Media{
+						Video: prop.Video{
+							Width:       256,
+							Height:      144,
+							FrameFormat: frame.FormatI420,
+						},
+					},
+					image.NewYCbCr(
+						image.Rect(0, 0, 256, 144),
+						image.YCbCrSubsampleRatio420,
+					),
+				)
+			})
 		})
 	}
 }
diff --git a/pkg/codec/vpx/vpx_test.go b/pkg/codec/vpx/vpx_test.go
@@ -60,6 +60,25 @@ func TestEncoder(t *testing.T) {
 					},
 				})
 			})
+			t.Run("ReadAfterClose", func(t *testing.T) {
+				p, err := factory()
+				if err != nil {
+					t.Fatal(err)
+				}
+				codectest.VideoEncoderReadAfterCloseTest(t, p,
+					prop.Media{
+						Video: prop.Video{
+							Width:       256,
+							Height:      144,
+							FrameFormat: frame.FormatI420,
+						},
+					},
+					image.NewYCbCr(
+						image.Rect(0, 0, 256, 144),
+						image.YCbCrSubsampleRatio420,
+					),
+				)
+			})
 		})
 	}
 }

diff --git a/pkg/codec/x264/x264_test.go b/pkg/codec/x264/x264_test.go
@@ -46,6 +46,26 @@ func TestEncoder(t *testing.T) {
 			},
 		})
 	})
+	t.Run("ReadAfterClose", func(t *testing.T) {
+		p, err := NewParams()
+		if err != nil {
+			t.Fatal(err)
+		}
+		p.BitRate = 200000
+		codectest.VideoEncoderReadAfterCloseTest(t, &p,
+			prop.Media{
+				Video: prop.Video{
+					Width:       256,
+					Height:      144,
+					FrameFormat: frame.FormatI420,
+				},
+			},
+			image.NewYCbCr(
+				image.Rect(0, 0, 256, 144),
+				image.YCbCrSubsampleRatio420,
+			),
+		)
+	})
 }
 
 func TestShouldImplementKeyFrameControl(t *testing.T) {