Last Comment Bug 717026 - small-shot.ogg seems to play twice (again)
: small-shot.ogg seems to play twice (again)
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
data:text/html,<video src="http://pea...
Depends on:
Blocks: 636894
  Show dependency treegraph
 
Reported: 2012-01-10 13:57 PST by Chris Pearce (:cpearce)
Modified: 2012-01-19 17:46 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (4.86 KB, patch)
2012-01-17 16:43 PST, Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2012-01-10 13:57:26 PST
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 2 Chris Pearce (:cpearce) 2012-01-17 16:43:36 PST
Created attachment 589350 [details] [diff] [review]
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] 2012-01-18 16:41:11 PST
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 5 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-01-19 02:58:03 PST
https://hg.mozilla.org/mozilla-central/rev/ece333215bde
Comment 6 Ed Morley [:emorley] 2012-01-19 17:45:10 PST
https://hg.mozilla.org/mozilla-central/rev/e5eaff41f775
Comment 7 Ed Morley [:emorley] 2012-01-19 17:46:27 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.