558761 - [OS/2] Update sydney_audio_os2

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rich Walsh]

Attachment: saos2 update

With the replacement of liboggplay by mozilla's own OGG decoder (Bug 531340), the buffering scheme used by sydney_audio_os2 no longer works well. This patch updates it and makes other changes to the audio backend.

While liboggplay allowed only minimal buffering in the backend, its replacement requires it for good performance.  saos2 now uses 2.5mb of low memory address space but only 480k of physical memory for typical audio streams (i.e. 2 channels @ 44.1/48k).  Normal system activity is far less likely to interrupt sound playback than previously.

The other major change is the elimination of event semaphores used to pass buffer counts between threads.  They've been replaced by atomic-operation functions found in [gcc]\include\386\builtin.h.  The patch also contains other changes to improve performance & reliability.

Comment 1

[Walter Meinl]

Since I'm off for a week review has to wait a bit, sorry. Dave do you have time to give it a try?

Comment 2

[Dave Yeo]

(In reply to comment #1)
> Since I'm off for a week review has to wait a bit, sorry. Dave do you have time
> to give it a try?

I'll try to stress test it here. Not really qualified to judge the code though.

Comment 3

[Rich Walsh]

Attachment: audio info v2 - for testing only

If anyone wants to see what sydney_audio_os2 is doing, apply this patch after 'saos2 update'.  It uses stderr to display hard errors as well as info msgs during a stream's startup & shutdown.  It can also display info whenever sa_stream_write() or sa_stream_get_position() is called.  Since each of these generates a _lot_ of output, the write stats are currently commented out.  If you enable them, comment-out the get_pos stats (which I found more useful for judging performance).

Comment 4

[Rich Walsh]

Attachment: saos2 update v2

Removed a debugging statement that produced an "unresolved external" at link time.

Comment 5

[Rich Walsh]

Attachment: audio info v2a - for testing only

Moved a debugging statement from the main patch to this one.

Comment 6

[Dave Yeo]

While I have been quite impressed on the smoothness of this version of sydney_audio_os2 and only SeaMonkey loading a large file with JavaScript formatting has made an audio stream stutter (video just paused) it is still better then audio players running at high priority.
I do see this in the logs,

entering sa_stream_create_pcm:  1 channels @ 48000 bps
new stream= 2910000
buffers:  cnt= 40  ms/write= 40  bufMin= 3840  bufSize= 8192
entering sa_stream_open - stream= 2910000
loop= 0  poll= 961070832  diff= -1 - add=     0  adj=     0  pos= 0
loop= 0  poll=  62  diff= 12 - add= 10496  adj=  1152  pos= 11648
loop= 0  poll=  78  diff= 41 - add=  4096  adj=  3936  pos= 18528
...

seems the first poll= is very large.
The other problem that I discovered today, when listening to http://www.cbc.ca/quirks/media/2009-2010/ogg/qq-2009-09-19_01.ogg I paused the playback and forgot about it.
When another app made a beep the ogg file would play for close to a second. Most noticeable with Thunderbird, also noticeable though shorter with a SDL game exiting, not noticeable at all with PMMail beeping. The worst was when being prompted to change folders in SeaMonkey newsgroups. This would only happen once after pausing the ogg file.
I also haven't been able to test streaming as the browser seems to want to download the whole file before playing and my connection isn't really up to streaming video.

Comment 7

[Rich Walsh]

(In reply to comment #6)
> I do see this in the logs,
> 
> loop= 0  poll= 961070832  diff= -1 - add=     0  adj=     0  pos= 0
> loop= 0  poll=  62  diff= 12 - add= 10496  adj=  1152  pos= 11648
> 
> seems the first poll= is very large.

I left the variable unintialized rather than testing it every time to see if it was unitialized - something that's only true the very first time.

FYI...
* poll - how long (in ms) since the upstream code last checked the current playback position.
* diff - how long (in ms) since the last 'buffer-empty' event
* adj - a guesstimate of how many additional bytes have been played since the last 'buffer-empty' event - this figure is based on "diff"
* loop - since the time & byte-count variables aren't protected by a mutex, the event thread could change them while this code is calculating the playback position.  To produce an accurate result, it checks to see if they've changed, then loops & recalculates if they have.

> When another app made a beep the ogg file would play for close to a second.

This is a problem with DART that I don't know how to work around.  After the other sound source finishes with the device, DART gives it back to the OGG stream which then resumes playing, even though it was in a paused state.

Comment 8

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

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/fdef1b2f86ad

Comment 9

[Walter Meinl]

(In reply to comment #8)
> Pushed to mozilla-central:
> http://hg.mozilla.org/mozilla-central/rev/fdef1b2f86ad

Chris, thanks for checking this in. Should we add the patch to the list in media/libsydneyaudio ?

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > Pushed to mozilla-central:
> > http://hg.mozilla.org/mozilla-central/rev/fdef1b2f86ad
> 
> Chris, thanks for checking this in. Should we add the patch to the list in
> media/libsydneyaudio ?

Oops, we totally should!

Comment 11

[Walter Meinl]

If that's ok, I'll try whether I can split up the patch here and add changes to sydney_os2_base.patch and sydney_os2_moz.patch. Then we won't have to change the readme and the update.sh script.

Comment 12

[Walter Meinl]

Attachment: patch for sydney_os2_base.patch

Comment 13

[Walter Meinl]

Attachment: patch for sydney_os2_moz.patch

Rich could you have please a look, whether I've split up your main patch correctly in basic and mozilla-specific parts?
I've patched with these two incremental patches those in the tree and used the new patches to recreate sydney_os2.c. Hg diff shows no differences, thus the sum of the patches works correctly.
Chris do we need reviews?

Comment 14

[Rich Walsh]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > 
> > Chris, thanks for checking this in. Should we add the patch to the list in
> > media/libsydneyaudio ?
> 
> Oops, we totally should!

The current revision should be appended to sydney_os2_moz.patch.  The original was designed to work with liboggplay and is appropriate for "upstreaming".  OTOH, this works great with Chris' creation but would fail if used with liboggplay (the extra buffering would cause liboggplay to stall).

Comment 15

[Walter Meinl]

Attachment: patch for sydney_os2_moz.patch

(In reply to comment #14)

> The current revision should be appended to sydney_os2_moz.patch.  The original
> was designed to work with liboggplay and is appropriate for "upstreaming". 
> OTOH, this works great with Chris' creation but would fail if used with
> liboggplay (the extra buffering would cause liboggplay to stall).
the patch attached updates sydney_os2_moz.patch to contain all the changes of attachment (id=439223). Because the new patch removes some of the stuff that was introduced with the old sydney_os2_moz.patch I found it would be cleaner to refresh the whole patch instead of simply appending the new lines to the old patch.

Comment 16

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

Comment on attachment 445406 [details] [diff] [review]
patch for sydney_os2_moz.patch

I'll take your word for it and check this in before I forget again.

Comment 17

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

(In reply to comment #15)
> Created an attachment (id=445406) [details]
> patch for sydney_os2_moz.patch

http://hg.mozilla.org/mozilla-central/rev/e4e69d7b6186

OS2 patches should now reflect what's in the tree...

Comment 18

[Walter Meinl]

(In reply to comment #17)
> 
> http://hg.mozilla.org/mozilla-central/rev/e4e69d7b6186

Hm, when I follow the link, there's a pushlog, but I don't see the patch, and checking out the tree doesn't change the patch either, only the changes you did for the tests in the same push. Did the patch got lost during checkin?

Comment 19

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

huh, so looking back at my `history`, and testing, I can see that an `hg import ...; hg qref -U... -m...` does not do an `hg qref`. Madness. I shall push again once Ehsan pushes his fix...

Comment 20

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

Pushed again, seemed to work this time.
http://hg.mozilla.org/mozilla-central/rev/b860cc41eb96

Comment 21

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

(In reply to comment #20)
> Pushed again, seemed to work this time.
> http://hg.mozilla.org/mozilla-central/rev/b860cc41eb96

Sorry, this chageset has my username as the patch author, there's something wrong with my mercurial, every time I `hg qref -U ... -m ...` it resets the changes, and every time I `hg qref` it resets the username. Weirdness.

Comment 22

[Walter Meinl]

(In reply to comment #14)

> The original was designed to work with liboggplay and is appropriate for "upstreaming". 
Um, what about upstreaming - should we?

(In reply to comment #21)
> Sorry, this chageset has my username as the patch author, there's something
Never mind (though creating a patch (that removes parts of another patch) for a patch was really quirky ;-)