Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upDeinterleave decoded audio #117
Conversation
|
r? @Manishearth |
| decodebin.connect_pad_added(move |_, src_pad| { | ||
| // We only want a sink per audio stream. | ||
| // Ignore any additional source pads just in case. | ||
| if src_pad.is_linked() { |
This comment has been minimized.
This comment has been minimized.
sdroege
Aug 28, 2018
Contributor
This is never going to be true. What you want to check here is if linking to the next sinkpad fails because the pad is already linked
| .map_err(|_| ())?; | ||
| let pipeline_ = pipeline.clone(); | ||
| let callbacks_ = callbacks.clone(); | ||
| deinterleave.connect_pad_added(move |_, src_pad| { |
This comment has been minimized.
This comment has been minimized.
sdroege
Aug 28, 2018
Contributor
You create a reference cycle here: pipeline -> deinterleave -> pipeline.
Best to use weak references (pipeline.downgrade() and later pipeline.upgrade()) for passing into the closure. Same bug elsewhere probably
| @@ -44,13 +54,14 @@ unsafe impl Send for AudioDecoderCallbacks {} | |||
| unsafe impl Sync for AudioDecoderCallbacks {} | |||
|
|
|||
| pub struct AudioDecoderCallbacksBuilder { | |||
| eos: Option<Box<FnBox(u32) + Send + 'static>>, | |||
| eos: Option<Box<FnBox() + Send + 'static>>, | |||
This comment has been minimized.
This comment has been minimized.
Manishearth
Aug 28, 2018
Member
can we still pass down channels here so we don't have to use shared mutable state on the DOM side to deal with this?
This comment has been minimized.
This comment has been minimized.
ferjm
Aug 29, 2018
Author
Member
We still need to use shared mutable state to build the final decoded audio from the progress callbacks and to create the final AudioBuffer content when receiving the eos callback. I think we can infer the number of channels from the final decoded audio array, so we don't need to send the channel count param through the eos callback.
|
I don't understand most of this, I'll defer to @sdroege's review |
|
r? @sdroege |
|
Looks good but you probably want to check for similar circular references in other closures |
ferjm commentedAug 28, 2018
Fixes #55