449754 - Ogg Theora backend for HTML5 video element failed to compile/work on Solaris

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Ginn Chen]

Some header file config issues.
The major problem is there's no Sun Audio support for media/liboggplay_audio.

Comment 1

[Ginn Chen]

Attachment: WIP

Comment 2

[cajbir (:cajbir)]

Please note that some of the files you will be modifying for Solaris support are files from an upstream 3rd party library. 

If the original libraries actually build ok on Solaris from their source distribution then I may just need to tweak the way I configure the build for Solaris (the changes made in update.sh and defines in the Makefile.in).

If the original's don't build, you might like to confirm if they already have Solaris support in their svn and I can pull patches against particular svn revisions.

I see that there are some changes for Solaris in their trac for example:

https://trac.xiph.org/search?q=solaris

The Xiph track contains details on libogg, libvorbis and libtheora. The Annodex trac contains details on the remaining media libraries:

http://trac.annodex.net

liboggplay_audio was originally the audio from browser_plugins in the Annodex svn. You may want to check with them to see what the status of Solaris support is - or contribute a port of the audio implementation for them.

Comment 3

[Boying Lu]

Attachment: add sunaudio support

Comment 4

[Ginn Chen]

Chris,

libogg, libvorbis, liboggz can compile on Solaris.

ogg/config_types.h is a generated file, on Solaris, it has
typedef uint16_t ogg_uint16_t;
typedef uint32_t ogg_uint32_t;

libvorbis have a generated config.h which has
#define HAVE_ALLOCA_H     1
on Solaris.

iboggz/include/oggz/oggz_off_t_generated.h uses
typedef off64_t oggz_off_t;
on Solaris.
My patch was incorrect about this.

The change to liboggplay and liboggplay_audio should go upstream.

Comment 5

[cajbir (:cajbir)]

Sun audio support is added by updating to libsydneyaudio in upstream. See bug 459765. The libsydneyaudio maintainers added the sunaudio support from this patch.

Comment 6

[Ginn Chen]

Thanks!

There's still something to followup, I will post patches here later.

Comment 7

[Boying Lu]

Attachment: updated patch

Comment 8

[Boying Lu]

Comment on attachment 336438 [details] [diff] [review]
add sunaudio support

The patch has been upstreamed.

I posted a new patch at 
http://trac.annodex.net/ticket/433

Comment 9

[Boying Lu]

and 
http://trac.annodex.net/ticket/434

Comment 10

[cajbir (:cajbir)]

Trac Ticket 434 has landed in mozilla-central via bug 462409

Comment 11

[Boying Lu]

(In reply to comment #10)
> Trac Ticket 434 has landed in mozilla-central via bug 462409

The patch for the second file is not landed.

Comment 12

[cajbir (:cajbir)]

Yes, I haven't landed the liboggz change yet. I assume that's the one you're referrring too.

Comment 13

[cajbir (:cajbir)]

Expanding on comment 12, trac ticket 434 was marked under the liboggplay component but included a patch for liboggz. I only picked up the change for the component that the ticket was filed for, sorry. I'll look at the liboggz change soon.

Comment 14

[Boying Lu]

I posted a new patch at https://trac.xiph.org/ticket/1453

Comment 15

[cajbir (:cajbir)]

You probably didn't mean to raise that ticket (1453) as it's a Mozilla specific file. Probably attach it here.

Comment 16

[cajbir (:cajbir)]

Trac ticket 433 has landed in mozilla-central by updating to libsydneyaudio in this commit:

http://hg.mozilla.org/mozilla-central/rev/05b6f4f2101e

Comment 17

[Boying Lu]

Attachment: Don't use asm() on solaris [Checkin: Comment 35]

Comment 18

[Ralph Giles (:rillian)]

The libtheora configure script only tries to compile the inline asm if it detects gcc; one solution is to add a similar check in the mozilla build. Note that we have separate asm for x86 gcc, and x86 msvc; the latter would give a significant performance improvement if they were added to that build.

I gather Sun Studio support the gcc syntax for inline assembly, but there's a bug that prevents it from compiling the theora code? Perhaps adding specific blocks, like we have for gcc <= 3.1 would help there.

Cross reference with https://trac.xiph.org/ticket/1453 which is the upstream bug.

Comment 19

[Ginn Chen]

I think detection of GCC is more accurate here (for Solaris platform).

For Sun Studio, it doesn't recognize code like this
"movq "OC_I(3)",%%mm2\n\t"
Concatenate several strings with calculation (3*16 here) doesn't work here.
I think it would need a few lines changes.
Also, if we build this code with mmx or sse option with Sun Studio, the shared library will have MMX or SSE hardware cap tag.
If we want to use the same binary on non-mmx machine, we need somehow isolate the code that uses mmx.
It's doable. See: http://blogs.sun.com/alblog/entry/ridding_or_modifying_hardware_capabilities

I don't want to solve this issue here.
I think it would be better if we use the system libvorbis library instead on Linux and Solaris distros.

Comment 20

[Boying Lu]

(In reply to comment #19)
> I think detection of GCC is more accurate here (for Solaris platform).
> 
Since this file is a firefox specific file and SunStduio 12 is the only compiler we use to build firefox, detect OS_ARCH shouldn't be a problem.

Comment 21

[Boying Lu]

I posted a patch at https://trac.xiph.org/ticket/1454

Comment 22

[Ginn Chen]

(In reply to comment #20)
> (In reply to comment #19)
> > I think detection of GCC is more accurate here (for Solaris platform).
> > 
> Since this file is a firefox specific file and SunStduio 12 is the only
> compiler we use to build firefox, detect OS_ARCH shouldn't be a problem.

Somebody may want to build Firefox with GCC on Solaris.
But building Firefox with GCC on Solaris was broken for quite a while (> 2 years?).

If somebody really cares, he can fix this later.

Comment 23

[Boying Lu]

Attachment: patch

I can build libogg-1.1.3 on OpenSolaris without any problem. the generated file 
config_types.h is different with the one used in firefox trunk.

So I guess the patch had better to be posted here.

Comment 24

[cajbir (:cajbir)]

Needs update.sh and README_MOZILLA changed to document the change so the they don't get lost when I refresh to a new version of libogg. See media/liboggz/update.sh and media/liboggz/README_MOZILLA for examples.

Comment 25

[Boying Lu]

Attachment: patch

Comment 26

[Boying Lu]

Attachment: patch including README_MOZILLA and update.sh

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346386 [details] [diff] [review]
Don't use asm() on solaris
[Checkin: Comment 35]

Clearing blocking request as it's on an out of date patch

Comment 28

[Boying Lu]

Attachment: patch for liboggz

Comment 29

[cajbir (:cajbir)]

Comment on attachment 350307 [details] [diff] [review]
patch for liboggz

For both these patches you'll also need to include the actual change itself. eg. in this case, the results of applying oggz_off_t.patch.

update.sh only gets run when updating to a new version of the upstream library so your change won't get included by just applying this patch.

r+=me with that change.

Comment 30

[Boying Lu]

Attachment: include actual changes

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 347250 [details] [diff] [review]
patch including README_MOZILLA and update.sh

Flip the #ifndef around so it's #ifdef.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 350419 [details] [diff] [review]
include actual changes

+#if defined(__APPLE__) || defined (SOLARIS)

Drop the extra space between "defined" and "(SOLARIS)"

Comment 33

[Boying Lu]

Attachment: libogg revised patch [Checkin: Comment 36]

Comment 34

[Boying Lu]

Attachment: revised patch for liboggz [Checkin: Comment 37]

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 346386 [details] [diff] [review]
Don't use asm() on solaris
[Checkin: Comment 35]

http://hg.mozilla.org/mozilla-central/rev/917509259a36

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 350741 [details] [diff] [review]
libogg revised patch
[Checkin: Comment 36]

http://hg.mozilla.org/mozilla-central/rev/534ba368b351

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 350742 [details] [diff] [review]
revised patch for liboggz
[Checkin: Comment 37]

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

Comment 38

[Ginn Chen]

Attachment: patch for 1.9.1 branch

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 363845 [details] [diff] [review]
patch for 1.9.1 branch

a191=beltzner

Comment 40

[Ginn Chen]

Pushed to 1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/63a008740297

Comment 41

[cajbir (:cajbir)]

The patch pushed in comment 40 seems to be missing the change to media/libvorbis/README_MOZILLA that was included in the patch originally pushed to mozilla-central. Comparing the README_MOZILLA file in mozilla-central and mozilla-1.9.1 shows the difference. Please do a followup patch with the change , thanks.

Comment 42

[Ginn Chen]

Fixed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e5f8bcc606fe

Comment 43

[Tony Chung [:tchung]]

can someone on a solaris platform confirm this is fixed and working on 1.9.1 trunk and branch?   Please revert status and keyword after doing so.  thanks.

Comment 44

[Ginn Chen]

Fix verified.
Although bug 487197 is blocking the compiling on Solaris with both mozilla-central and mozilla-1.9.1.

Comment 45

[Mike Hommey [:glandium]]

Wouldn't using stdint.h and uint{16,32}_t everywhere be a more upstreamable change ?

Comment 46

[Ginn Chen]

This doesn't need to go upstream, because libogg uses configure script rather than static header.
And Visual Studio doesn't have stdint.h

Comment 47

[Conrad Parker]

(In reply to comment #34)
> Created an attachment (id=350742) [details]
> revised patch for liboggz

This doesn't need to go upstream, because liboggz generates the oggz_off_t type based on GNU autoconf. This patch is specific to Mozilla's static header.