-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move audio sink into audio destination node #38
Conversation
r? @Manishearth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should just store the destination node separately from the graph? I'm not sure.
Looks fine as is, either way.
I'd rather not have the Any stuff here. I don't think we need it.
c5d749e
to
7f63337
Compare
I ended up storing the destination node separated from the graph. I think it'll be slightly simpler this way. Once we add graph support, we can simply store the indexes that are connected to the destination node input. |
} | ||
} | ||
|
||
impl AudioNodeEngine for DestinationNode { | ||
fn process(&mut self, inputs: Chunk, _: &BlockInfo) -> Chunk { | ||
inputs | ||
fn process(&mut self, inputs: Chunk, _: &BlockInfo) -> Option<Chunk> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to return Option here, Chunk::default is empty. Chunk contains the number of blocks equal to the number of output ports, here zero.
Also, can we switch back to the old sink model? My graph PR needs it to work since there's some extra work that may need to be done in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Since the PR was approved and you didn't comment any further, I assumed you were ok with merging this. I'll revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I just forgot to comment, that was my bad 😅😅
…On Thu, Jun 7, 2018, 5:53 AM Fernando Jiménez Moreno < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/audio/destination_node.rs
<#38 (comment)>:
> }
}
impl AudioNodeEngine for DestinationNode {
- fn process(&mut self, inputs: Chunk, _: &BlockInfo) -> Chunk {
- inputs
+ fn process(&mut self, inputs: Chunk, _: &BlockInfo) -> Option<Chunk> {
Ok. Since the PR was approved and you didn't comment any further, I
assumed you were ok with merging this. I'll revert it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#38 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABivSNqVllO3GcjzD5PBbnwmPUc-3ip9ks5t6SJlgaJpZM4UZVUr>
.
|
No description provided.