448918 - Make MOZ_MEDIA work on OS/2

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[Peter Weilbacher]

Attachment: Working build with stub functions

For the moment --disable-ogg should work to get a build going, but if we want to get the new content stuff working we need to port some stuff or at least add dummies to get it through the build. I attach a patch with some stubs in the new file modules/liboggplay_audio/sydney_audio_os2wave.c that need to be filled for MMOS2. I'm not too sure about the semaphore definitions, either, but I had to add something to modules/liboggplay/src/liboggplay/std_semaphore.h...

Comment 1

[Dave Yeo]

Attachment: oggz_off_t_generated.h fixed for OS/2

Finally tried building Firefox without --disable-ogg and the first problem is that we need to patch media/liboggz/include/oggz/oggz_off_t_generated.h or change the build system to create off_t_generated.h.
The problem is that I don't know where to send the patch. The file was checked in in bug #442538, https://bugzilla.mozilla.org/attachment.cgi?id=330960.
Building a standalone liboggz creates a correct off_t_generated.h and it does not exist in liboggz svn.
I notice in bug #449066 that FreeBSD needs a similar patch.

Comment 2

[Dave Yeo]

Now I feel stupid, looking again at your patch I notice that you came across the same issue.
Still the off_t_generated patch should probably be separated as various other platforms based on the FreeBSD headers will need a similar patch.

Comment 3

[Peter Weilbacher]

Thanks for confirming that part of my patch, Dave. :-) Before we proceed any further, we need info where to send the patches so that they get included "upstream".

I got the hint from KO Myung-Hun that libdart is the library that we need to look at to fill liboggplay_audio/sydney_audio_os2wave.c with useful content. The only problem is that at the moment it is LGPL only. I have asked the contributors to get it tri-licensed. Otherwise we could still make libdart a (dynamic?) dependency, although I don't like that.

Comment 4

[Peter Weilbacher]

OK, the main authors seem to be the same for the three (at the moment) involved packages (liboggz, liboggplay, liboggplay_audio, although for the latter I don't find an AUTHORS file). They are mainly Shane Stevens and Conrad Parker, as listed e.g. in http://svn.annodex.net/liboggplay/trunk/AUTHORS. I guess that's where the OS/2 patches should be sent.

Comment 5

[Dave Yeo]

I've already filed a couple of bugs against liboggz (a include sys/types.h and as usual, a setmode()). Generally the libs compile fine on OS/2, it is just the test programs and tools that often need minor patching, usually just a setmode() or two.
In the case of off_t_generated.h, it is automatically generated during the build using gcc -E and the Mozilla version seems to of been added by Chris Double so the patch needs to be applied directly to the Mozilla tree, or generated by the build process. 

Comment 6

[Peter Weilbacher]

Dave, did you have time to do anything more on this?

I have gotten permission from Andrew Zabolotny and Alex Strelnikov to tri-license libdart, I copy the relevant parts of their reply emails below (I have their current email addresses, I just didn't want to paste them here because they might not want them to be publically listed). Both are no longer using OS/2, so no extra help there...

But we can use that library now without _legal_ problems, at least in the version that is present in the wvgui SVN repository (I see it here http://svn.netlabs.org/wvgui/browser/trunk/plugins/). KO Myung-Hun thinks his contribution and hence his newer version in his MPlayer (source) package is irrelevant, because he only changed the interface.


Date: Fri, 8 Aug 2008 01:20:38 +0400
From: Andrew Zabolotny <...>
To: Peter Weilbacher <[e-mail]>
Subject: Re: libdart licensing for Mozilla-OS/2 (fwd)
Message-ID: <[e-mail]>

From Thu, 7 Aug 2008 16:58:37 +0200 (CEST)
Peter Weilbacher <[e-mail]> wrote:

> > I saw that you contributed to the OS/2 library "libdart" in the past.
> > It has
> >    Copyright (C) 1998 by Andrew Zabolotny <[e-mail]>
> > in the header. It is currently licensed LGPL, but I wanted to ask if
> > you give me permission to change it to tri-license (GPL/LGPL/MPL).
Sure, no problem here.
[...]

Message-ID: <[e-mail]>
Date: Thu, 07 Aug 2008 21:28:17 +0400
From: lelik <...>
To: Peter Weilbacher <[e-mail]>

Peter Weilbacher wrote:
[...]
> I saw that you contributed to the OS/2 library "libdart". It has
>    Copyright (C) by Alex Strelnikov
> in the header. It is currently licensed LGPL, but I wanted to ask if you
> give me permission to change it to tri-license (GPL/LGPL/MPL). Then I
> would be able to use the code for the new media framework (the <audio>
> and <video> tags) in Mozilla applications on OS/2, mainly Firefox.
[...]
If there is my copyright, feel free to change it to tri-license (GPL/LGPL/MPL).
[...]

Comment 7

[Dave Yeo]

I haven't had much time to look into any of this. It is good to get libdart under a compatible licence, as this gives us options.
I was having problems finding the upstream for liboggplay_audio which doesn't seem to exist. Closest I have found is the browser_plugin for liboggplay which contains the sydney_audio_* files.
The source is available via svn at
svn co http://svn.annodex.net/browser_plugin/trunk browser_plugin
Hopefully I can find the time to get the plugin built with your stub files as a start, which means building xul-runner to get the gecko-sdk and I don't know what else :)
One other idea that occurred to me was to use the uniaud sdk to port the alsa drivers. This looks the simplest as the uniaud headers are based on the alsa headers. Of course the problem is that then audio will only work with uniaud, while a majority of users do have uniaud installed I'm sure a sizeable minority don't.
To port libdart to the browser_plugin will take someone more knowledgable then me. Perhaps at one point will need to make a general request for help.

Comment 8

[Peter Weilbacher]

I looked a bit more at implementing the sydney_audio_* file for OS/2 but I can't make sense of what exactly the functions are supposed to do. I didn't find docs about that and just trying to guess from the Windows and Linux equivalents doesn't help me.

Also, I still haven't understood where in the codebase the actual Ogg decoding is done. I would have expected that when building with stubs that always return OK I would at least get a display of where on the page the audio or video should appear, but I see no effect at all...

Comment 9

[Dave Yeo]

The actual Ogg decoding is done by liboggplay, http://svn.annodex.net/liboggplay/trunk, which depends on libogg, libvorbis, libtheora, optionally libspeex. Most of these build ok and KO has uploaded some of them to Hobbes.

Comment 10

[Peter Weilbacher]

But with the exception of libspeex they are all in the media/ subdir and indeed build correctly (with the patches here). So I would have expected that just audio wouldn't work but that videos do show up. (Why would we need libspeex but other platforms not?)
Is KMH using additional patches for his versions that we need in the mozilla-tree?

Do we need to specifically set other defines than just MOZ_MEDIA? Or add XP_OS2 somewhere else in the tree?

Chris, perhaps you can give us a few hints...

Comment 11

[cajbir (:cajbir)]

I've disabled libspeex support in all the libraries and not included libspeex. I only got permission to include the vorbis and theora codecs. We'd need to investigate speex before including it.

Each of the directories under media has a README_MOZILLA that explains the origin of the library. If something is missing, making it hard to track down the original, let me know and I'll fix.

The liboggplay_audio originally came from the annodex browser_plugin, but now has been spun into a separate library called libsydneyaudio. Bug 459765 recently committed to trunk moves us to this library in media/libsydneyaudio. Documentation is limited but if you're stuck feel free to ask. I did the Alsa implementation (based on the others).

You also need MOZ_OGG to be defined by the way. MOZ_MEDIA and MOZ_OGG should be defined by default when configure is run.

Comment 12

[Peter Weilbacher]

Thanks, Chris, for the comments. Will take a look at libsydneyaudio some time next week.

What is the procedure for getting patches, like attachment 332668 [details] [diff] [review] into the Mozilla tree? Do we have to mail them to the package maintainers and then tell you to update the sources? Or can we simultaneously push them to mozilla-central?

Comment 13

[cajbir (:cajbir)]

Since some of the files in media/ are generated, or modified to fit our build system, it's best to add the patch to the bug and add me as a reviewer. I'll take a look, and then follow the process to get it upstreamed, or modify what's needed in the build, etc.

Comment 14

[Rich Walsh]

Attachment: audio/video support for OS/2

includes sydney_audio_os2.c, patches to supporting files, and an implementation of posix semaphores

Comment 15

[Rich Walsh]

Some aspects of sydney_audio_os2 have been tailored to accommodate the ..umm.. imperfect state of the upstream code which feeds it (e.g. it leaks streams).  As that code matures, I'll update s_a_os2 accordingly.  Meanwhile, don't try to open more than one audio stream at a time:  the browser is guaranteed to crash.

Comment 16

[Peter Weilbacher]

Rich, sorry that it took me so long to comment on your patch. I only partly read it so far, but the stuff that I understood in passing make sense to me. :-) In the SM build that I created using it I saw a few more crashes than usual, mostly in GKLAYOUT.DLL. (I don't think I was trying to open more streams than one.)

I noticed this comment:
  /* the current design of nsAudioStream prevents
   * us from returning an honest answer */
Can you give a bit more detail of how you determined that? (I guess Mozilla-specific stuff should not be added into the sydneyaudio code.) A few more hints about the leaking streams would be appreciated, too. I didn't notice any such leaks on Linux, and probably the leak-testing would have uncovered them. If you explain, perhaps Chris can comment.

Comment 17

[Rich Walsh]

(In reply to comment #16)

> A few more hints about the leaking streams would be appreciated

When I play a file, sa_stream_destroy() is called after sa_stream_drain();  when I play an http: stream, it isn't.  Consequently, the device is left open and none of the allocated resources (buffers, semaphores, etc.) are freed.

> I noticed this comment:
>   /* the current design of nsAudioStream prevents
>    * us from returning an honest answer */
> Can you give a bit more detail of how you determined that?

1) nsAudioStream::Write() tries to keep sa_stream_write() from blocking the thread by determining how many bytes can be written without blocking, then buffering the rest.  Unfortunately, its buffering scheme doesn't scale well and imposes no limits on itself.  While it may be suitable for holding a few hundred ms. of sound, errors downstream which keep the device from accepting data for longer periods will cause it to drive (my) CPU usage to 100% as it tries to buffer far more data than it can handle efficiently.

In cases where the stream is creating data faster than it can be consumed, it should be throttled back or blocked.  If instead, additional buffering is considered preferable, then it should happen in sydney_audio where it's accessible to the hardware and generates much lower overhead..

2) nsAudioStream::Available() ignores the return code from sa_stream_get_write_size().  All implementations perform some action that could fail & would be cause for destroying the stream (although, in truth, few of the others actually check for errors and none return anything other than SA_SUCCESS).  In earlier, buggier versions of sydney_audio_os2, attempts to return an "honest" answer (zero bytes available & an error code because of a semaphore time-out) just caused nsAudio's buffering scheme to run amok.

3) When playing a file, "admitting" that there's a full second of buffer space available causes repeated buffer underruns.  The upstream code just doesn't start refilling them soon enough.  Returning a value equivalent to 250ms or less produces an uninterrupted stream.  I'd guess that the problem is in nsFileStreamStrategy or the like, not nsAudioStream.

> I saw a few more crashes than usual, mostly in GKLAYOUT.DLL.

All of the media stuff is in gklayout.  When you see crashes there, could you check its map file to determine what function, or at least, what module it occurred in?  BTW... trying to play a Flash video while an audio stream is playing is instant death (SYS3175 in FLASHWIN.DLL).

Comment 18

[Rich Walsh]

Attachment: improved performance in sydney_audio_os2

This revision of sydney_audio_os2.c ensures that audio and video remain in sync after a buffer underrun.  Other files in the patch are unchanged.

Comment 19

[Rich Walsh]

Attachment: debug patch

This adds info about nsAudioStream's overflow buffer to sydney_audio_os2's console messages.  In particular, it demonstrates the effect of a hardware buffer underrun on the overflow buffer's size.

Comment 20

[Rich Walsh]

Attachment: improved & debugged sydney_audio_os2

Removed some obsolete code in sa_stream_drain() which caused errors during end-of-stream processing.

Comment 21

[Peter Weilbacher]

Chris, could you comment on the points that Rich raised in comment 17? (Rich, I take it that even with your newest patch they still represent a problem?)

Comment 22

[cajbir (:cajbir)]

(In reply to comment #17)
> When I play a file, sa_stream_destroy() is called after sa_stream_drain(); 
> when I play an http: stream, it isn't.  Consequently, the device is left open
> and none of the allocated resources (buffers, semaphores, etc.) are freed.

sa_stream_destroy() is called for me on both http and file streams. Can you provide more information? Can you provide steps to reproduce?

For the other issues I suggest raising bugs for them so they don't get lost and can be investigated/fixed.

Comment 23

[Peter Weilbacher]

Rich, I finally had a go at testing your patch again. And on my current setup it works fine (in a debug build of SeaMonkey from comm-central). Do you not think that we should get this in now and then deal with issues as they come up afterwards? I also tried to play several videos at the same time, in multiple windows and tabs and even from the same document. Seemed to use high CPU at times but no crashes or other bad effects.

Comment 24

[Peter Weilbacher]

Actually, that was a mistake in my MMOS2 setup that caused sound not to play. And thought it was because I had turned the speakers down... It does crash if more than one video with sound is played at the same time. I have to look at this in more detail.

Comment 25

[Rich Walsh]

(In reply to comment #24)
> It does crash if more than one video with sound is played at the same time.
> I have to look at this in more detail.

As noted in comment 15, it's supposed to crash.  This was intended as a temporary workaround to deal with the http stream leakage I saw when listening to http://217.150.130.173:8000/jazzradio56.ogg

After every track, sa_stream_drain()is called but the stream is never closed.  Network activity continues, but AFAICT, no function in sydney_audio_os2 is ever called again.  You have to close the tab to destroy the stream & halt the download.

I just tested both the Windows & Mac versions of Firefox 3.5b4 and saw what I believe is the exact same behavior.  Perhaps this is related to Bug 455165 "Firefox fails on chained ogg stream" (is there anyone who can confirm this?).

Clearly, this workaround has to be removed before the OS/2 code is committed.  However, users should be warned that tabs with ogg content should be closed after playback ends.

Comment 26

[cajbir (:cajbir)]

Please raise a bug for the stream leakage with details, thanks.

Comment 27

[Rich Walsh]

(In reply to comment #26)
> Please raise a bug for the stream leakage with details, thanks.

See Bug 494504

Comment 28

[Peter Weilbacher]

Thanks, Rich. My stupid comments from last week are a perfect example what happens, if one looks at a bug late at night, thinking one knows the history, but indeed misremembers the state of a patch...

As you found out that the leak is a cross-platform problem, could you create a new patch without that crashing code and so that Chris can review and get into the tree?

Comment 29

[Rich Walsh]

Attachment: sydney_audio_os2 update

Thanks to greatly improved upstream code, I've been able to remove the kludges used in the last version to keep audio & video in sync.  It also buffers far less than before, reducing a stream's footprint by almost a 1mb.  A downside is that it's more subject to hiccups due to heavy CPU usage, but it seems to recover much quicker.

Loading multiple streams is possible but performance is unreliable unless you explicitly pause existing streams before starting a new one (and then pause that before resuming another).  I'm going to work on adding some mechanism to coordinate the streams so they can peacefully co-exist.

BTW... you may see CPU usage spikes and/or 100% CPU usage on occasion.  I believe this is a bug in the upstream code.  See Bug 495352 for details.

Comment 30

[Rich Walsh]

Attachment: workaround for 100% CPU usage (Bug 495352)

This is a kludgy-but-effective workaround that keeps the upstream code from going crazy.  Every time the write() routine finds it's been called 16 consecutive times with no data, it yields the rest of its time-slice.  I tested it with a bunch of video clips and it doesn't seem to affect video playback or sync.

Comment 31

[cajbir (:cajbir)]

For the final patch to be reviewed could you split this up into separate patches for the various upstream libraries. (ie. for liboggz, liboggplay, and libsydneyaudio).

Please also add a .patch file in the directory (media/liboggz, media/liboggplay and media/libsydneyaudio) with the patch that can be applied to our repository to make these changes, and update README_MOZILLA noting the patch name, this bug, and what the patch is for (adding OS/2 support). Once we upstream the code this won't be needed but until then it's needed so the changes don't get lost when we refresh to new upstream code.
 
Also note that some of the changes to upstream code refers to Mozilla code. For example:

+  /* nsAudioStream ignores this function's errors, so return a non-zero
+   * value here & let it bomb-out in sa_stream_write() instead */

Given that this is added to a third party library with no dependancy on Mozilla code, they won't know what nsAudioStream is.

Comment 32

[Rich Walsh]

(In reply to comment #31)
> Please also add a .patch file in the directory (media/liboggz,
> media/liboggplay, and media/libsydneyaudio) with the patch that can
> be applied to our repository to make these changes

I'm not sure I fully understand.  For, say media/liboggplay, should the patch I post here contain:
 A) all the patches needed to update the contents of its subdirectories,
    *plus* a new file in media/liboggplay containing said patches, or
 B) just the new file in media/liboggplay that someone will later use
    to update the subdirectories

Also, should my mozilla-specific patches (i.e. changes to makefile.in's) be incorporated in that top-level patch file?

> note that some of the changes to upstream code refers to Mozilla code.

I've made the comments more generic and changed the code slightly to eliminate any hint of Bug 495352.  If that isn't resolved in due course, I'll change my workaround patch into something that can be added to the media/libsydneyaudio directory.

Comment 33

[Rich Walsh]

(In reply to comment #29)
> I'm going to work on adding some mechanism to coordinate the streams
> so they can peacefully co-exist.

I tried this at the libsydneyaudio level & it just doesn't work.  It has to happen at a much higher level so that this coordinating code can, in effect, push the pause button for an existing stream when a new one becomes active.  Unfortunately, I have no idea where such code belongs.

Comment 34

[Peter Weilbacher]

(In reply to comment #32)
> I'm not sure I fully understand.  For, say media/liboggplay, should the patch I
> post here contain:
>  A) all the patches needed to update the contents of its subdirectories,
>     *plus* a new file in media/liboggplay containing said patches, or
>  B) just the new file in media/liboggplay that someone will later use
>     to update the subdirectories
> 
> Also, should my mozilla-specific patches (i.e. changes to makefile.in's) be
> incorporated in that top-level patch file?

I am not Chris, but seeing how the process works in the media dirs and elsewhere when foreign code is involved, your item (A) sounds correct. And you should collect the Makefile.in collect them in an extra patch. (Looking at e.g. http://hg.mozilla.org/mozilla-central/rev/057648c9f7e1 I think it would also help Chris, if you added a line to the README_MOZILLA files.)

Comment 35

[Rich Walsh]

Attachment: add OS/2 support to liboggz

Comment 36

[Rich Walsh]

Attachment: add OS/2 support to liboggplay

Comment 37

[Rich Walsh]

Attachment: add OS/2 support to libsydneyaudio

Comment 38

[Rich Walsh]

I hope I got this right...

Each patch updates its respective library's tree and adds a patch file to the top-level directory.  That file contains only changes to the upstream versions of these libraries (i.e. Mozilla-specific changes are not included).

media/libsydneyaudio has an additional patch file named "sydney_os2_moz.patch".  This contains changes to sydney_audio_os2.c that are Mozilla-specific and not appropriate for a general-purpose version of the library.  Unlike the other patches, this one is "permanent" and should be applied every time the upstream code is refreshed.

Comment 39

[Rich Walsh]

Attachment: adds debugging output to sydney_audio_os2.c

This patch is not intended to be committed.  It just sends a lot of debug and informational messages to the console for those who are interested.  It's useful but 100% optional.

Comment 40

[Peter Weilbacher]

Chris, are you aware that we have been waiting for your review for more than a month now? The non-OS/2 part of the patches is really small, so it shouldn't take too much of your time...

Comment 41

[cajbir (:cajbir)]

Yes, I am aware, thanks.

Comment 42

[Peter Weilbacher]

Great, thanks for the review, Chris. I pushed the three patches to trunk:

http://hg.mozilla.org/mozilla-central/rev/8c0b5a34583d
http://hg.mozilla.org/mozilla-central/rev/cc7766d63a13
http://hg.mozilla.org/mozilla-central/rev/57f2315358ef

Comment 43

[Peter Weilbacher]

As these patches are essentially OS/2-only and haven't caused any problems (that I know of) on trunk, and we really want to finally enable media for Firefox 3.5.3, I'm going to ask for approval for these patches and the tiny OS/2 patch in bug 506434.

Chris, if you disagree, please let me know.

Comment 44

[Peter Weilbacher]

Comment on attachment 383099 [details] [diff] [review]
add OS/2 support to liboggz

OS/2-only change in cross-platform file.

Comment 45

[Peter Weilbacher]

Comment on attachment 383100 [details] [diff] [review]
add OS/2 support to liboggplay

Small OS/2-only changes in cross-platform files, and a few added OS/2-only files.

Comment 46

[Peter Weilbacher]

Comment on attachment 383101 [details] [diff] [review]
add OS/2 support to libsydneyaudio

Small OS/2-only changes in cross-platform files, and an added OS/2-only file; all of this recorded in the patch against the external library.

Comment 47

[Peter Weilbacher]

Actually, for attachment 390628 [details] [diff] [review] from bug 506434 I don't need to ask for approval. It's fully contained in the OS/2-only file, so I can just commit it, together with the relevant patch update, when I push these patches to 1.9.1 (if approved).

Comment 48

[Samuel Sidler (old account; do not CC)]

Comment on attachment 383101 [details] [diff] [review]
add OS/2 support to libsydneyaudio

All three of these patches, approved for 1.9.1.3. a=ss

In the future, can you please provide one roll-up patch with all changes?

Comment 49

[Rich Walsh]

(In reply to comment #48)
> In the future, can you please provide one roll-up patch with all changes?

Perhaps I misunderstood the request to split this up made in comment #31, but I think this was a special case because 3 separate 3rd-party libraries were being patched in addition to the Mozilla code.

Comment 50

[cajbir (:cajbir)]

The split was at my request as mentioned in comment 49 to make upstreaming easier.

Comment 51

[Peter Weilbacher]

Yeah, would have been easier to make a combined patch for 1.9.1 checkin. Thanks for the extra approval efforts!

Landed it just now:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/29db404861ac
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/39f062f2866b
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5d00ea92d877

(Yay! We'll get 3.5.3 with media in the default build. :-))

Comment 52

[Conrad Parker]

(In reply to comment #35)
> Created an attachment (id=383099) [details]
> add OS/2 support to liboggz

Just confirming that this does not need to be applied to upstream liboggz, as the file is generated there using an autoconf macro.

Comment 53

[Peter Weilbacher]

(In reply to comment #52)
> Just confirming that this does not need to be applied to upstream liboggz, as
> the file is generated there using an autoconf macro.

Then why was this not correct in liboggz from the very beginning? And what has now changed upstream so that you get the correct list of #ifdefs?

Comment 54

[Conrad Parker]

My understanding is that was correct in liboggz upstream from the very beginning, as liboggz upstream uses an autoconf macro to determine off_t; the file is generated during the build process. Mozilla instead directly distributes a copy of the generated version of that file, and that copy did not include the correct OS/2 support -- perhaps it was generated with an incorrect version of autoconf. The patch in this bug fixes the generated copy in the Mozilla tree.

As for "what has now changed upstream": perhaps autoconf now includes the correct OS/2 support. If that is not correct, and there is also a bug in the upstream version then please let me know. I will gladly apply such a patch upstream for that issue.

Comment 55

[cajbir (:cajbir)]

Conrad's understanding is correct. We don't run autoconf for the media libraries. Instead we modify things to work with our build system and any patches to the  generated file shouldn't go upstream.

Comment 56

[Peter Weilbacher]

OK, I think I understand now. Upstream liboggz is only prepared to build for one platform at a time, so include/oggz/oggz_off_t_generated.h.in only contains
   typedef @TYPEOF_OGGZ_OFF_T@ oggz_off_t;
and no #ifdefs at all.

As the Mozilla update.sh script does not copy oggz_off_t_generated.h from the liboggz tree, it is basically private. So the patch for that file is superfluous and should be removed from the Mozilla tree?

Comment 57

[cajbir (:cajbir)]

include/oggz/oggz_off_t_generated.h.in isn't in the Mozilla tree (or shouldn't be) so if there's a patch there for it it should be remobed.

Comment 58

[Peter Weilbacher]

Read my comment again: oggz_off_t_generated.h (without .in) _is_ in the tree. There is no patch against the file with .in.