From 58dc90d03a893db18c338659723c365e6d3d252d Mon Sep 17 00:00:00 2001 From: Atsushi Watanabe Date: Wed, 24 Aug 2022 20:23:57 +0900 Subject: [PATCH] Fix opus codec panic on read after close (#440) Also add test to other codecs --- pkg/codec/internal/codectest/codectest.go | 38 +++++++++++++++++++++++ pkg/codec/mmal/mmal_test.go | 19 ++++++++++++ pkg/codec/openh264/openh264_test.go | 19 ++++++++++++ pkg/codec/opus/opus.go | 12 +++++++ pkg/codec/opus/opus_test.go | 19 ++++++++++++ pkg/codec/vaapi/vaapi_test.go | 19 ++++++++++++ pkg/codec/vpx/vpx_test.go | 19 ++++++++++++ pkg/codec/x264/x264_test.go | 20 ++++++++++++ 8 files changed, 165 insertions(+) diff --git a/pkg/codec/internal/codectest/codectest.go b/pkg/codec/internal/codectest/codectest.go index b2f266e4..a740f9ec 100644 --- 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 index ea3e00af..61b42549 100644 --- 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 index 04806ba1..5aaadd99 100644 --- 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 index 84b8feac..adfac43d 100644 --- 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 index ea89d1c7..5e45edd6 100644 --- 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 index 3a59af21..deaaefd4 100644 --- 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 index 01fb5402..53010fae 100644 --- 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 index a3e1eed2..fb930d83 100644 --- 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) {