Closed Bug 1481512 Opened 2 years ago Closed Last year

make libvorbis compile on aarch64 windows

Categories

(Core :: Audio/Video, enhancement, P3)

ARM64
Windows
enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Surprisingly, all that needs to be changed is this bit:

https://searchfox.org/mozilla-central/source/media/libvorbis/lib/os.h#148-150

which assumes that _WIN64 implies x86-64, which isn't true for aarch64.

I don't know whether it'd be reasonable to try and define an optimized path for aarch64; that could be done as a followup in any event.
Rank: 25
Priority: -- → P3
We're using tremor on aarch64 aren't we ? Why are we compiling libvorbis at all then? Maybe I'm missing something.
Flags: needinfo?(nfroyd)
(In reply to Paul Adenot (:padenot) from comment #1)
> We're using tremor on aarch64 aren't we ? Why are we compiling libvorbis at
> all then? Maybe I'm missing something.

We use tremor on Android or on arm (32-bit) platforms.  I assume AArch64 would have decent enough support for floats that vorbis would be more appropriate than tremor?
Flags: needinfo?(nfroyd)
I see now that this is Windows, sorry about the noise. When this is working well enough, we can profile and see whether it's worth it to use integer paths, it's more or less a single #define, as far as the media stack is concerned.
(In reply to Paul Adenot (:padenot) from comment #3)
> I see now that this is Windows, sorry about the noise. When this is working
> well enough, we can profile and see whether it's worth it to use integer
> paths, it's more or less a single #define, as far as the media stack is
> concerned.

I dunno, maybe it'd be worth it to use tremor just in case there are fewer OS dependencies.  I have no idea what the policy on accepting code into libvorbis is (github?  mail list?  xiph bugtracker?  all of these look pretty dead) and I don't know whether we permit local patches.
...rather than all 64-bit Windows implementations.

Not really sure who the correct person for this is, please redirect if
necessary!
Attachment #9005234 - Flags: review?(padenot)
Comment on attachment 9005234 [details] [diff] [review]
limit libvorbis x86-64 assembly to x86-64 windows

Review of attachment 9005234 [details] [diff] [review]:
-----------------------------------------------------------------

This would work, but the process is to put a patch file in media/libvorbis, add it to update.sh with a `patch -pX < asdasd.patch` command at the bottom, and to commit the patch file, the modification to os.h, and the modification to update.sh.
Attachment #9005234 - Flags: review?(padenot)
Thanks for the explanation, I should have looked for update.sh in the first
place.
Attachment #9005247 - Flags: review?(padenot)
Attachment #9005234 - Attachment is obsolete: true
Comment on attachment 9005247 [details] [diff] [review]
limit libvorbis x86-64 assembly to x86-64 windows

Review of attachment 9005247 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #9005247 - Flags: review?(padenot) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7f2727ad4f
limit libvorbis x86-64 assembly to x86-64 windows; r=padenot
Assignee: nobody → nfroyd
https://hg.mozilla.org/mozilla-central/rev/eb7f2727ad4f
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.