717026 - small-shot.ogg seems to play twice (again)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

When played in Firefox, content/media/test/small-shot.ogg seems to play twice per play(). Every other player I play small_shot.ogg in plays it once, so either we are wrong, or everybody else it.

I seem to remember having this problem before...

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=372eca6704c2&tochange=919f15d71153

This is a regression from bug 636894; http://hg.mozilla.org/mozilla-central/rev/635bb4ffe6ad

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

So we have two problems here:
1. We use minWriteFrames wrong. The code assumes that any individual call to nsAudioStream::Write() which writes less than minWriteFrames frames won't end up with all frames pushed to the hardware. On Windows this isn't the case. Audio is queued up in blocks of size minWriteFrames, once a block is filled, it gets written to hardware. So at any time if we've not written an even multiple of minWriteFrames frames, we'll have audio which has been queued but not written to hardware. This is what's happening while playing small-shot.ogg; we're writing minWriteFrames+N frames, and some of the audio data is overlapping and not completely filling the last block and so is not played, and then we're underrunning, causing the stuttery playback.
2. sa_stream_get_min_write isn't returning a value which is an even multiple of num_channels * bytes_per_sample, so the calculation in nsNativeAudioStream::GetMinWriteSize() is rounding down, and so our predictions of how much silence we need to write to ensure the last block of audio gets played are off by a few frames anyway.

Solution:
1. Instead of pushing silence if the *last* write pushed less than minWriteFrames frames, push enough silence to ensure the number of frames we've pushed is a multiple of minWriteFrames, so all blocks are flushed.
2. Move the brackets around in sa_stream_create_pcm to ensure the block size is a multiple of num_channels*bytes_per_sample. 

Greenish on Try, modulo known randoms:
https://tbpl.mozilla.org/?tree=Try&rev=d41ed5f7ae98

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 589350 [details] [diff] [review]
Patch v1

Review of attachment 589350 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +782,5 @@
>        ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +      PRInt64 unplayedFrames = audioDuration % minWriteFrames;
> +      if (minWriteFrames > 1 && unplayedFrames > 0) {
> +        // Sound is written by libsydneyaudio to the hardware in blocks of
> +        // frames, of size minWriteFrames. So if the number of frames we've

Extra comma after frames.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ece333215bde

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/ece333215bde

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e5eaff41f775

Comment 7

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #6)
> https://hg.mozilla.org/mozilla-central/rev/e5eaff41f775

This cset belongs in bug 713381, wrong bug number in commit.