Closed Bug 449754 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
OpenSolaris
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: ginnchen+exoracle, Assigned: eagle.lu)

References

Details

(Keywords: verified1.9.1)

Attachments

(4 files, 8 obsolete files)

Some header file config issues.
The major problem is there's no Sun Audio support for media/liboggplay_audio.
Attached patch WIP (obsolete) — Splinter Review
Blocks: 449757
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.
Attached patch add sunaudio support (obsolete) — Splinter Review
Attachment #336438 - Flags: review?(chris.double)
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.
Sun audio support is added by updating to libsydneyaudio in upstream. See bug 459765. The libsydneyaudio maintainers added the sunaudio support from this patch.
Attachment #336438 - Flags: review?(chris.double)
Thanks!

There's still something to followup, I will post patches here later.
Attached patch updated patch (obsolete) — Splinter Review
Attachment #332920 - Attachment is obsolete: true
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
Attachment #336438 - Attachment is obsolete: true
Depends on: 462409
Trac Ticket 434 has landed in mozilla-central via bug 462409
(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.
Yes, I haven't landed the liboggz change yet. I assume that's the one you're referrring too.
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.
I posted a new patch at https://trac.xiph.org/ticket/1453
You probably didn't mean to raise that ticket (1453) as it's a Mozilla specific file. Probably attach it here.
Trac ticket 433 has landed in mozilla-central by updating to libsydneyaudio in this commit:

http://hg.mozilla.org/mozilla-central/rev/05b6f4f2101e
Attachment #346386 - Flags: review?(chris.double)
Attachment #346386 - Flags: review?(chris.double) → review+
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.
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.
(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.
Attachment #346386 - Flags: superreview?(roc)
I posted a patch at https://trac.xiph.org/ticket/1454
Attachment #346386 - Flags: superreview?(roc) → superreview+
Keywords: checkin-needed
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #347224 - Flags: review?(chris.double)
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.
Attached file patch (obsolete) —
Assignee: nobody → brian.lu
Attachment #347224 - Attachment is obsolete: true
Attachment #347249 - Attachment is obsolete: true
Attachment #347250 - Flags: review?(chris.double)
Attachment #347224 - Flags: review?(chris.double)
Attachment #346386 - Flags: approval1.9.1b2?
Keywords: checkin-needed
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
Attachment #346386 - Flags: approval1.9.1b2?
Attachment #347250 - Flags: review?(chris.double) → review+
Attachment #347250 - Flags: superreview?(roc)
Attached patch patch for liboggz (obsolete) — Splinter Review
Attachment #350307 - Flags: review?(chris.double)
Attachment #350307 - Flags: review?(chris.double) → review+
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.
Attached patch include actual changes (obsolete) — Splinter Review
Attachment #350419 - Flags: superreview?(roc)
Comment on attachment 347250 [details] [diff] [review]
patch including README_MOZILLA and update.sh

Flip the #ifndef around so it's #ifdef.
Comment on attachment 350419 [details] [diff] [review]
include actual changes

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

Drop the extra space between "defined" and "(SOLARIS)"
Attachment #350741 - Flags: superreview?(roc)
Attachment #350742 - Flags: superreview?(roc)
Attachment #350741 - Flags: superreview?(roc) → superreview+
Attachment #350742 - Flags: superreview?(roc) → superreview+
Attachment #347250 - Attachment is obsolete: true
Attachment #347250 - Flags: superreview?(roc)
Attachment #350419 - Attachment is obsolete: true
Attachment #350419 - Flags: superreview?(roc)
Keywords: checkin-needed
Attachment #350307 - Attachment is obsolete: true
Attachment #345069 - Attachment is obsolete: true
Comment on attachment 346386 [details] [diff] [review]
Don't use asm() on solaris
[Checkin: Comment 35]

http://hg.mozilla.org/mozilla-central/rev/917509259a36
Attachment #346386 - Attachment description: Don't use asm() on solaris → Don't use asm() on solaris [Checkin: Comment 35]
Comment on attachment 350741 [details] [diff] [review]
libogg revised patch
[Checkin: Comment 36]

http://hg.mozilla.org/mozilla-central/rev/534ba368b351
Attachment #350741 - Attachment description: libogg revised patch → libogg revised patch [Checkin: Comment 36]
Comment on attachment 350742 [details] [diff] [review]
revised patch for liboggz
[Checkin: Comment 37]

http://hg.mozilla.org/mozilla-central/rev/d503a9f90157
Attachment #350742 - Attachment description: revised patch for liboggz → revised patch for liboggz [Checkin: Comment 37]
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #363845 - Flags: approval1.9.1?
Comment on attachment 363845 [details] [diff] [review]
patch for 1.9.1 branch

a191=beltzner
Attachment #363845 - Flags: approval1.9.1? → approval1.9.1+
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.
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.
Fix verified.
Although bug 487197 is blocking the compiling on Solaris with both mozilla-central and mozilla-1.9.1.
Status: RESOLVED → VERIFIED
Wouldn't using stdint.h and uint{16,32}_t everywhere be a more upstreamable change ?
This doesn't need to go upstream, because libogg uses configure script rather than static header.
And Visual Studio doesn't have stdint.h
(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.
You need to log in before you can comment on or make changes to this bug.