Repository: pdeljanov/Symphonia (language: rust)

Issue:
MediaSourceStream is not Send
Due to wrapping a `Box<dyn MediaSource>` instead of `Box<dyn MediaSource + Send>`, MediaSourceStream is not safe to send across thread boundaries. Is this intentional?


Discussion / hints:
Not really intentional.

Symphonia was initially just a project for me to learn Rust, so multi-threading was not something I was thinking about. The original design was actually pretty hostile to multi-threading, but it is better suited to it now.

I believe it would be advantageous if `MediaSourceStream`, `FormatReader`s, and `Decoder`s can all be moved between threads, though I haven't considered all the implications.

Fix patch:
diff --git a/symphonia-core/src/codecs.rs b/symphonia-core/src/codecs.rs
index c585cefe..e4ff4f1d 100644
--- a/symphonia-core/src/codecs.rs
+++ b/symphonia-core/src/codecs.rs
@@ -317,7 +317,7 @@ impl Default for DecoderOptions {
 
 /// A `Decoder` implements a codec's decode algorithm. It consumes `Packet`s and produces
 /// `AudioBuffer`s.
-pub trait Decoder {
+pub trait Decoder: Send {
 
     /// Attempts to instantiates a `Decoder` using the provided `CodecParameters`.
     fn try_new(params: &CodecParameters, options: &DecoderOptions) -> Result<Self>
diff --git a/symphonia-core/src/formats.rs b/symphonia-core/src/formats.rs
index baa20f59..982170cb 100644
--- a/symphonia-core/src/formats.rs
+++ b/symphonia-core/src/formats.rs
@@ -118,7 +118,7 @@ impl Stream {
 /// `FormatReader` provides an Iterator-like interface over packets for easy consumption and
 /// filtering. Seeking will invalidate the assumed state of any decoder processing packets from
 /// `FormatReader` and should be reset after a successful seek operation.
-pub trait FormatReader {
+pub trait FormatReader: Send {
     /// Attempt to instantiate a `FormatReader` using the provided `FormatOptions` and
     /// `MediaSourceStream`. The reader will probe the container to verify format support, determine
     /// the number of contained streams, and read any initial metadata.
diff --git a/symphonia-core/src/io/mod.rs b/symphonia-core/src/io/mod.rs
index b3de73dd..5e79e34b 100644
--- a/symphonia-core/src/io/mod.rs
+++ b/symphonia-core/src/io/mod.rs
@@ -25,7 +25,7 @@ pub use scoped_stream::ScopedStream;
 
 /// A `MediaSource` is a composite trait of `std::io::Read` and `std::io::Seek`. Despite requiring
 /// the `Seek` trait, seeking is an optional capability that can be queried at runtime.
-pub trait MediaSource: io::Read + io::Seek {
+pub trait MediaSource: io::Read + io::Seek + Send {
     /// Returns if the source is seekable. This may be an expensive operation.
     fn is_seekable(&self) -> bool;
 
@@ -60,7 +60,7 @@ impl MediaSource for std::fs::File {
     }
 }
 
-impl<T: std::convert::AsRef<[u8]>> MediaSource for io::Cursor<T> {
+impl<T: std::convert::AsRef<[u8]> + Send> MediaSource for io::Cursor<T> {
     /// Always returns true since a `io::Cursor<u8>` is always seekable.
     fn is_seekable(&self) -> bool {
         true
@@ -81,7 +81,7 @@ pub struct ReadOnlySource<R: io::Read> {
     inner: R,
 }
 
-impl<R: io::Read> ReadOnlySource<R> {
+impl<R: io::Read + Send> ReadOnlySource<R> {
     /// Instantiates a new `ReadOnlySource<R>` by taking ownership and wrapping the provided
     /// `Read`er.
     pub fn new(inner: R) -> Self {
@@ -104,7 +104,7 @@ impl<R: io::Read> ReadOnlySource<R> {
     }
 }
 
-impl<R: io::Read> MediaSource for ReadOnlySource<R> {
+impl<R: io::Read + Send> MediaSource for ReadOnlySource<R> {
     fn is_seekable(&self) -> bool {
         false
     }
diff --git a/symphonia-core/src/meta.rs b/symphonia-core/src/meta.rs
index 34b859f0..b9a2f74c 100644
--- a/symphonia-core/src/meta.rs
+++ b/symphonia-core/src/meta.rs
@@ -522,7 +522,7 @@ impl MetadataQueue {
     }
 }
 
-pub trait MetadataReader {
+pub trait MetadataReader: Send {
     /// Instantiates the `MetadataReader` with the provided `MetadataOptions`.
     fn new(options: &MetadataOptions) -> Self
     where
diff --git a/symphonia-format-isomp4/src/demuxer.rs b/symphonia-format-isomp4/src/demuxer.rs
index 3fa6118f..889b0c4c 100644
--- a/symphonia-format-isomp4/src/demuxer.rs
+++ b/symphonia-format-isomp4/src/demuxer.rs
@@ -16,8 +16,8 @@ use symphonia_core::meta::MetadataQueue;
 use symphonia_core::probe::{Descriptor, Instantiate, QueryDescriptor};
 use symphonia_core::units::Time;
 
-use std::rc::Rc;
 use std::io::{Seek, SeekFrom};
+use std::sync::Arc;
 
 use crate::atoms::{AtomIterator, AtomType};
 use crate::atoms::{FtypAtom, MoovAtom, MoofAtom, SidxAtom, TrakAtom, MetaAtom, MvexAtom};
@@ -109,7 +109,7 @@ pub struct IsoMp4Reader {
     /// State tracker for each track.
     tracks: Vec<TrackState>,
     /// Optional, movie extends atom used for fragmented streams.
-    mvex: Option<Rc<MvexAtom>>,
+    mvex: Option<Arc<MvexAtom>>,
 }
 
 impl IsoMp4Reader {
@@ -404,8 +404,8 @@ impl FormatReader for IsoMp4Reader {
                             .collect();
 
         // A Movie Extends (mvex) atom is required to support segmented streams. If the mvex atom is
-        // present, wrap it in an Rc so it can be shared amongst all segments.
-        let mvex = moov.mvex.take().map(|m| Rc::new(m));
+        // present, wrap it in an Arc so it can be shared amongst all segments.
+        let mvex = moov.mvex.take().map(|m| Arc::new(m));
 
         let segs: Vec<Box<dyn StreamSegment>> = vec![ Box::new(MoovSegment::new(moov)) ];
 
diff --git a/symphonia-format-isomp4/src/stream.rs b/symphonia-format-isomp4/src/stream.rs
index 1836d15a..396ab16b 100644
--- a/symphonia-format-isomp4/src/stream.rs
+++ b/symphonia-format-isomp4/src/stream.rs
@@ -8,14 +8,14 @@ use symphonia_core::errors::{Result, Error, decode_error};
 
 use crate::atoms::{MoofAtom, MoovAtom, StcoAtom, Co64Atom, MvexAtom, stsz::SampleSize};
 
-use std::rc::Rc;
+use std::sync::Arc;
 
 pub struct SampleDataDesc {
     pub base_pos: u64,
     pub size: u32,
 }
 
-pub trait StreamSegment {
+pub trait StreamSegment: Send {
     /// Gets the sequence number of this segment.
     fn sequence_num(&self) -> u32;
 
@@ -45,13 +45,13 @@ struct SequenceInfo {
 
 pub struct MoofSegment {
     moof: MoofAtom,
-    mvex: Rc<MvexAtom>,
+    mvex: Arc<MvexAtom>,
     seq: Vec<SequenceInfo>,
 }
 
 impl MoofSegment {
     /// Instantiate a new segment from a `MoofAtom`.
-    pub fn new(moof: MoofAtom, mvex: Rc<MvexAtom>, last: &Box<dyn StreamSegment>) -> MoofSegment {
+    pub fn new(moof: MoofAtom, mvex: Arc<MvexAtom>, last: &Box<dyn StreamSegment>) -> MoofSegment {
         let mut seq = Vec::new();
 
         // Calculate the sequence information for each track of this segment.
diff --git a/symphonia-format-ogg/src/mappings/mod.rs b/symphonia-format-ogg/src/mappings/mod.rs
index d5a73a2f..6fd8c78d 100644
--- a/symphonia-format-ogg/src/mappings/mod.rs
+++ b/symphonia-format-ogg/src/mappings/mod.rs
@@ -30,7 +30,7 @@ pub enum MapResult {
 }
 
 /// A `Mapper` implements packet-handling for a specific `Codec`.
-pub trait Mapper {
+pub trait Mapper: Send {
     fn codec(&self) -> &CodecParameters;
     fn map_packet(&mut self, buf: &[u8]) -> Result<MapResult>;
 }


Test patch: