511038 - Fix Ogg/Theora on ARM

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Our current ogg-related code does not compile on ARM.  It should.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: liboggplay cpu detection code

The cpu detection code in liboggplay tries to compile x86-only code on all platforms.  This puts ifdefs around the relevant code block.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: liboggz unaligned access fixes

Curiously, someone fixed one part of this a while back in http://lists.xiph.org/pipermail/ogg-dev/2009-May/001394.html but didn't fix the other 3 macros that were in the same spot, doing the exact same kind of bad cast.  And noone mentioned it.

Comment 3

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

If this is as trivial as it looks I think it should block Fennec 1.0; the playback won't be fantastic, I don't expect, but it'll show progress and commitment to open video on mobile devices.

Comment 4

[[e-mail]]

Comment on attachment 394987 [details] [diff] [review]
liboggz unaligned access fixes

>+static ogg_int64_t
>+int64_le_at (unsigned char *c)
>+{
>+  ogg_int32_t a = c [0] | (c [1] <<  8) | (c [2] <<  16) | (c [3] <<  24);
>+  ogg_int32_t b = c [4] | (c [5] <<  8) | (c [6] <<  16) | (c [7] <<  24);
>+
>+  return (((ogg_int64_t)b) << 32) | a;
> }
Since ogg_int32_t is signed, a will get sign extended to 64 bits which is not what you want. (b doesn't care because it gets shifted anyway.)

Comment 5

[Makoto Kato [:m_kato]]

About cpu.c, I believe that it is already fixed on upstream by http://git.xiph.org/?p=liboggplay.git;a=commitdiff;h=b908ed69267c001310cc234628fb4c7dc41ced76

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: liboggz unaligned access fixes, v2

Good catch -- now with more uint32_t.

Comment 7

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

(In reply to comment #6)
> Created an attachment (id=395755) [details]
> liboggz unaligned access fixes, v2

We maintain changes to liboggz as patches which we apply to a liboggz release/changeset with the media/liboggz/update.sh script. So your patch also needs to add these changes to a patch in media/liboggz, and you need to edit media/liboggz/update.sh and media/liboggz/README_MOZILLA similar to how we've done it for the other patches in media/liboggz.

Comment 8

[Conrad Parker]

patch v2 (id=395755) applied in upstream liboggz master branch (and seek-rewrite etc. are rebased on top of master):

commit 20609d34c41fa611fe3dbb4853d4f328bd0a6185
Author: Conrad Parker <[e-mail]>
Date:   Fri Aug 21 15:21:21 2009 +0900

    Mozilla #511038: Fix Ogg/Theora on ARM
    
    "liboggz unaligned access fixes v2" patch by Vladimir Vukicevic (:vlad)

Comment 9

[cajbir (:cajbir)]

With Conrad pushing the fixes upstream the patch is obviously fine. Needs the changes outlined in comment 7 before we can push to trunk - or we need to update to liboggz tip and then we don't need this patch.

Added wiking, the liboggplay maintainer, to take a look at the liboggplay patch.

Comment 10

[cajbir (:cajbir)]

Comment on attachment 394985 [details] [diff] [review]
liboggplay cpu detection code

r+ with update to README_MOZILLA, addition of a .patch file to be applied so it doesn't get lost when we do a liboggplay update, and adding the patch application to update.sh.

Comment 11

[Viktor Gal]

(In reply to comment #9)
> With Conrad pushing the fixes upstream the patch is obviously fine. Needs the
> changes outlined in comment 7 before we can push to trunk - or we need to
> update to liboggz tip and then we don't need this patch.
> 
> Added wiking, the liboggplay maintainer, to take a look at the liboggplay
> patch.

i've just pushed my patch to oggplay repo. the only diff between that and the one by Vlad is that i've added  defined(_M_AMD64) macro to the macro condition, as otherwise it'll have problems when compiling it on Win64.

see commit 9c7a748

Comment 12

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

I'm working on updating the ogg libraries, which will include the changes in these patches, so these changes will be in those patches.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ok, great -- I'll hold off on landing these then.  Are we going to take those updates on 1.9.2 as well?

Comment 14

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

Fixed on m-c by bug 512327.

Comment 15

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

Bug 512327 was backed out to see if it caused a near permanent orange on Linux, so reopening this bug until that lands again.
[ http://hg.mozilla.org/mozilla-central/rev/b722d9fdf154 ]

Comment 17

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

Fixed on m-c by bug 512327...

Comment 18

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

We still need liboggplay changeset 9c7a74803736d45c10c40e4f15f899246bbd7f9f before we can mark this fixed.

Comment 19

[Matt Brubeck (:mbrubeck)]

bug 512328 includes liboggplay changeset 9c7a74803736d45c10c40e4f15f899246bbd7f9f