Skip to content
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

Play silence by default #15

Merged
merged 1 commit into from May 23, 2018
Merged

Play silence by default #15

merged 1 commit into from May 23, 2018

Conversation

@ferjm
Copy link
Member

ferjm commented May 22, 2018

Quoting the spec:

Before a source is started (by calling start(), the source node MUST output silence (0). After a source has been stopped (by calling stop()), the source MUST then output silence (0).

@ferjm
Copy link
Member Author

ferjm commented May 22, 2018

@ferjm ferjm requested a review from Manishearth May 22, 2018
let need_data = move |app: &AppSrc, _bytes: u32| {
let mut buffer = gst::Buffer::with_size(buf_size).unwrap();
let need_data = move |app: &AppSrc, bytes: u32| {
let mut buffer = gst::Buffer::from_slice(&SILENCE[0..bytes as usize]).unwrap();

This comment has been minimized.

@sdroege

sdroege May 22, 2018

Contributor

Here you create a buffer with read-only memory ...

@@ -81,19 +55,16 @@ impl AudioSink for GStreamerAudioSink {
.mul_div_floor(gst::SECOND_VAL, rate as u64)
.unwrap()
.into();
let next_pts: gst::ClockTime = (sample_offset + n_samples)
let next_pts: gst::ClockTime = (sample_offset + WEBAUDIO_RENDER_QUANTUM)
.mul_div_floor(gst::SECOND_VAL, rate as u64)
.unwrap()
.into();
buffer.set_pts(pts);
buffer.set_duration(next_pts - pts);
let mut map = buffer.map_writable().unwrap();

This comment has been minimized.

@sdroege

sdroege May 22, 2018

Contributor

... and here the read-only memory would be copied into writeable memory so you can modify it.

It would be more efficient to create a buffer with writeable memory to begin with. with_size as before would do that, you only have to fill it with silence then e.g. with this

let need_data = move |app: &AppSrc, _bytes: u32| {
let mut buffer = gst::Buffer::with_size(buf_size).unwrap();
let need_data = move |app: &AppSrc, bytes: u32| {
let mut buffer = gst::Buffer::from_slice(&SILENCE[0..bytes as usize]).unwrap();

This comment has been minimized.

@sdroege

sdroege May 22, 2018

Contributor

Also you use the bytes as passed in by appsrc. In theory that might be more than 4096, in theory it might not be a multiple of the unit size (sample size * channels), and worst: the alignment of the silence Vec might not be correct (it needs to be aligned to the sample size). You probably want to allocate always the same buffer size here like before and not worry about anything :)

@ferjm ferjm force-pushed the ferjm:silence branch from f80eca5 to 708cbc5 May 22, 2018
@ferjm
Copy link
Member Author

ferjm commented May 22, 2018

Thank you for the feedback @sdroege! I just updated the patch.

rate,
);
sample_offset += n_samples;
AudioFormatInfo::from_format(gst_audio::AUDIO_FORMAT_F32).fill_silence(data);

This comment has been minimized.

@sdroege

sdroege May 22, 2018

Contributor

This works, but you probably want to simply keep around a AudioInfo for the format you produce later (which then also contains an AudioFormatInfo).

@ferjm ferjm force-pushed the ferjm:silence branch from 708cbc5 to 494b271 May 22, 2018
@Manishearth
Copy link
Member

Manishearth commented May 23, 2018

The render quantum is now called FRAMES_PER_BLOCK from #17.

This just needs .fill_silence(). r=me with those changes.

@Manishearth Manishearth force-pushed the ferjm:silence branch from 494b271 to 79cffbf May 23, 2018
@Manishearth
Copy link
Member

Manishearth commented May 23, 2018

Okay, so there are two separate issues here.

One is "what should happen if we don't connect a source node to the output". That should be fixed by fill_silence. I've made this commit do so.

The other is "source nodes should output silence when stopped". That should be fixed by having them directly output silence 😄 . We can do that once we have messaging.

@ferjm
Copy link
Member Author

ferjm commented May 23, 2018

The other is "source nodes should output silence when stopped". That should be fixed by having them directly output silence 😄 . We can do that once we have messaging.

I'm doing this as part of #21

@Manishearth Manishearth merged commit b3d4a0b into servo:master May 23, 2018
@ferjm ferjm deleted the ferjm:silence branch May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.