920992 - [flatfish] can't play ogg video (.ogv)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect, P1)

Description

[tommy_chiu]

If there is any .ogv or .ogg file at /mnt/sdcard/Movies, the video player can't be lunched sucessfully.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi tommy,

Would you mind to upload the sample ogv or ogg file here??

Comment 2

[tommy_chiu]

Attachment: It's a normal ogv file.

Comment 3

[tommy_chiu]

Boot2Gecko is based on Android 4.2 version.

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

We can play it in Inari with 20130925172950, b2g27. It looks like device dependent bug.

Comment 5

[tommy_chiu]

I got the error log.
==================================
I/Gecko   ( 1270):
I/Gecko   ( 1270): ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv
I/Gecko   ( 1270):
I/Gonk    ( 1270): Setting nice for pid 1629 to 1
I/Gonk    ( 1270): Changed nice for pid 1629 from 18 to 1.
I/GeckoDump( 1270): Crash reporter : Can't fetch app.reportCrashes. Exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/shell.js :: shell_reportCrash :: line 120"  data: no]
==================================

Comment 6

[Steven Yang [:styang]]

Marco, could you help on this?

Comment 7

[tommy_chiu]

Hi, All:

Let me explain the issue. After lunching the video player, it will scan the /mnt/scard/Movies/* . If it find the (.ogv) files, it will read it's ogv meta data in the gecko layer. In the function  oc_dec_headerin(), it will call the assembly function call "oc_pack_read", and then the video player crash ! The root cause is the assembly function call.

I changed the gcc version to 4.4.x (arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gcc) and re-build the libtheora folder only. And the video player works fine!

Is it a good solution ?

Comment 8

[Marco Chen [:mchen]]

Hi,

May I know your original gcc version?

In Gonk-ICS version, we used 4.4.3.
In Gonk-JB MR2 version, we used 4.7.

Thanks.

Comment 9

[tommy_chiu]

( 4.4.3 )
./arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gcc -v

Using built-in specs.
Target: arm-linux-androideabi
Configured with: /tmp/android-build-bb7e003d31d08f72cabc269a652912b7/src/build/../gcc/gcc-4.4.3/configure --prefix=/usr/local --target=arm-linux-androideabi --host=i686-linux-gnu --build=i686-linux-gnu --with-gnu-as --with-gnu-ld --enable-languages=c,c++ --with-gmp=/tmp/android-build-bb7e003d31d08f72cabc269a652912b7/obj/temp-install --with-mpfr=/tmp/android-build-bb7e003d31d08f72cabc269a652912b7/obj/temp-install --disable-libssp --enable-threads --disable-nls --disable-libmudflap --disable-libgomp --disable-sjlj-exceptions --disable-shared --disable-tls --with-float=soft --with-fpu=vfp --with-arch=armv5te --enable-target-optspace --disable-hosted-libstdcxx --enable-cxx-flags='-fexceptions -frtti' --with-gcc-version=4.4.3 --with-binutils-version=2.20.1 --with-gmp-version=4.2.4 --with-mpfr-version=2.4.1 --with-gdb-version=7.1.x --with-arch=armv5te --with-sysroot=/tmp/android-build-bb7e003d31d08f72cabc269a652912b7/arm-linux-androideabi-4.4.x/sysroot --with-prefix=/tmp/android-build-bb7e003d31d08f72cabc269a652912b7/arm-linux-androideabi-4.4.x --with-gold-version=20100303 --enable-gold=both/gold --program-transform-name='s,^,arm-linux-androideabi-,'
Thread model: posix
gcc version 4.4.3 (GCC)

Comment 10

[Marco Chen [:mchen]]

(In reply to tommy_chiu from comment #9)
> ( 4.4.3 )

Hi, 
in comment 7, you said this issue can be fixed by gcc 4.4.x and the log you post is to make sure 4.4.x mean 4.4.3.

My question is what gcc version you used for crash issue? Thanks.

Comment 11

[tommy_chiu]

The crash gcc version is 4.6

prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/arm-linux-androideabi-gcc -v
Using built-in specs.
COLLECT_GCC=prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/arm-linux-androideabi-gcc
COLLECT_LTO_WRAPPER=/pj/Allwinner/B2G/a31_f1/a31-b2g/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../libexec/gcc/arm-linux-androideabi/4.6.x-google/lto-wrapper
Target: arm-linux-androideabi
Configured with: /tmp/android-8532/src/build/../gcc/gcc-4.6/configure --prefix=/usr/local --target=arm-linux-androideabi --host=x86_64-linux-gnu --build=x86_64-linux-gnu --with-gnu-as --with-gnu-ld --enable-languages=c,c++ --with-gmp=/tmp/android-8532/obj/temp-install --with-mpfr=/tmp/android-8532/obj/temp-install --with-mpc=/tmp/android-8532/obj/temp-install --without-ppl --without-cloog --disable-libssp --enable-threads --disable-nls --disable-libmudflap --disable-libgomp --disable-libstdc__-v3 --disable-sjlj-exceptions --disable-shared --disable-tls --disable-libitm --with-float=soft --with-fpu=vfp --with-arch=armv5te --enable-target-optspace --with-gcc-version=4.6 --with-binutils-version=2.21 --with-gmp-version=4.2.4 --with-mpfr-version=2.4.1 --with-gdb-version=7.3.x --with-arch=armv5te --with-sysroot=/tmp/android-8532/install/sysroot --with-prefix=/tmp/android-8532/install --with-gold-version=2.21 --enable-gold --program-transform-name='s&^&arm-linux-androideabi-&' --enable-gold=default
Thread model: posix
gcc version 4.6.x-google 20120106 (prerelease) (GCC)

Comment 12

[Ting-Yuan Huang]

It appears that some of the handwritten assembly codes are not aligned correctly. ARM codes should be word aligned. The crashing function oc_pack_read_arm starts at 0x40e8c022. Not sure why it is the case but, after adding ".align 4" it works correctly.

Dump of assembler code for function oc_pack_read_arm:
   0x40e8c022 <+0>:     add     r12, r0, #8
   0x40e8c026 <+4>:     ldm     r12, {r2, r3}
   0x40e8c02a <+8>:     subs    r3, r3, r1
   0x40e8c02e <+12>:    blt     0x40e8c04a <oc_pack_read_refill>
   0x40e8c032 <+16>:    rsb     r0, r1, #32
   0x40e8c036 <+20>:    lsr     r0, r2, r0
   0x40e8c03a <+24>:    lsl     r2, r2, r1
   0x40e8c03e <+28>:    stm     r12, {r2, r3}
   0x40e8c042 <+32>:    mov     pc, lr

Comment 13

[tommy_chiu]

After adding ".align 2", it works fine, thanks~

Comment 14

[Marco Chen [:mchen]]

Hi Ting-Yuan,

Thanks for your help.
And may I know you will summit a patch for ".align 4"?

Comment 15

[William Liang]

From the previous description, I think Ting-Yuan was right. I guess .align 4 was just a typo. Word alignment needs to use .align 2, which makes the following instructions start to aligned at 2^2 == 4 bytes boundry.

Comment 16

[Ting-Yuan Huang]

The older toolchain aligns texts to instruction size boundaries while the newer doesn't. It looks like kind of toolchain or linker script problem. I've no idea if we should adjust the linker script or explicitly specify alignments in assembly codes. Can somebody tell?

I have an alignment patch ready for review if we all agree.

By the way, the same problem should also exist in filter_neon.S in libpng.

Comment 17

[William Liang]

In case that thumb2 mode is used, both 32bit ARM instruction and 16bit instruction can be mixed, which may make it look like the alignment is not required. Back to the issue described in this bug, if the assembly part is just a handmade code segment, I think creating a patch on it directly could be feasible. FYR.

Comment 18

[Ting-Yuan Huang]

(In reply to William Liang from comment #17)
> In case that thumb2 mode is used, both 32bit ARM instruction and 16bit
> instruction can be mixed, which may make it look like the alignment is not
> required.

Then it could still be a problem in toolchain. Why generating codes that jump into (possibly) mixed codes in arm mode?

Comment 19

[William Liang]

Oh, since ARMv6, it provides thumb2 16 & 32 bit mixed instruction execution. The tool-chain, while doing the compilation, should be able to generate proper code to change the processor mode and make it run without any trouble. The problem here is that the code segment is hand-written, and the compiler won't be able modify what the developer intended to do in his/her assembly code. 

Of course, another possibility could be that, the hand-written code may just follow a few words of data area (not .data section, but declared as .byte/.halfword or something) which is put after some instructions, causing these hand-written instructions been started at half-word boundary. It's quite common to see that the .align is used in such cases to fix the alignment issue. (These code is typically reached by a branch instruction before the data area.) 

PS. Yes, with sophisticated design over the section arrangement, the linker script could be used to avoid the problem. But, I think .align is the simplest solution for this case.

Comment 20

[u153838]

I think you are looking at this GAS bug:

http://lists.gnu.org/archive/html/bug-binutils/2011-06/msg00199.html

Comment 21

[u153838]

I.e. in newer versions of gas the .text segment starts out with an alignment requirement of 1 (16-bit) but the instruction mode starts out as .arm/.code32.  In mixed Thumb/ARM assembler a .arm(.code32) or .thumb(.thumb,.thumbx,code16 as appropriate) directive is required to announce the instruction set.  .arm results in an implicit .align 2 and this bumps the total alignment requirement of the .text subsection.

In non-mixed ARM32 code no .arm is required, however that means gas has to bump the alignment as soon as it sees *anything* that puts stuff into the .text (not just an instruction, even random bytes of data).  It looks like GNU didn't get that right in some versions of gas.

It's inappropriate to work round a bug like this since it affects most of the ARM assembler that's out there - it needs to be fixed upstream.

Comment 22

[Timothy B. Terriberry (:derf)]

(In reply to [e-mail] from comment #21)
> It's inappropriate to work round a bug like this since it affects most of
> the ARM assembler that's out there - it needs to be fixed upstream.

While it definitely does need to be fixed upstream, we can't make changes to the whole B2G toolchain just for this. The current translation script in media/libtheora/lib/arm/arm2gnu.pl adds an .align 2 directive for .data and .rdata sections already. It should be simple enough to add one for .text (e.g., around line 91). That should fix this issue for all of the libtheora ASM. I'm happy to review a patch to that effect if someone can test it.

Comment 23

[u153838]

>we can't make changes to the whole B2G toolchain just [to fix a bug in the toolchain]

Oh well.

Comment 24

[Ting-Yuan Huang]

(In reply to Timothy B. Terriberry (:derf) from comment #22)
> (In reply to [e-mail] from comment #21)
> > It's inappropriate to work round a bug like this since it affects most of
> > the ARM assembler that's out there - it needs to be fixed upstream.
> 
> While it definitely does need to be fixed upstream, we can't make changes to
> the whole B2G toolchain just for this.

Maybe we can do both. The alignment directive just provides redundant information and shall not hurt.

I scanned the object files and there is only one problematical site in addition: filter_neon.S in libpng
find objdir-gecko/ -name "*.o" | xargs arm-linux-androideabi-readelf -S -W | grep " \.data" | grep "1$" | sed -e "/0000000000000000/d"

Unless more assemblies are added, I thinks this workaround should be enough for B2G.


> I'm happy to review a patch to that effect if someone can test it.

May I know how to test it? I submitted an alignment patch to try server and everything looked fine.

Comment 25

[Ting-Yuan Huang]

> find objdir-gecko/ -name "*.o" | xargs arm-linux-androideabi-readelf -S -W |
> grep " \.data" | grep "1$" | sed -e "/0000000000000000/d"

sorry, typo: s/.data/.text

find objdir-gecko/ -name "*.o" | xargs arm-linux-androideabi-readelf -S -W | grep " \.text" | grep "1$" | sed -e "/0000000000000000/d"

Comment 26

[Timothy B. Terriberry (:derf)]

(In reply to Ting-Yuan Huang from comment #24)
> May I know how to test it? I submitted an alignment patch to try server and
> everything looked fine.

Well, if you can reproduce this bug without the patch, and not with the patch, that's a pretty good test.

Comment 27

[Ting-Yuan Huang]

Attachment: alignments.patch

"ALIGN" is added into four assembly files instead of modifying arm2gnu.pl. This allows thumb codes in the future. Please let me know if you think a single change in arm2gnu.pl is better. I'm OK with both ways.

and yes, confirmed a dozen times that w/w.o. this patch turns proper alignments (observed using gdb) on/off and fixes crashes.

Comment 28

[Timothy B. Terriberry (:derf)]

Comment on attachment 824484 [details] [diff] [review]
alignments.patch

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

LGTM, but could you add a comment with a link to this bug in each file? That way someone will have a clue why this is here after we backport this patch upstream.

Comment 29

[Ting-Yuan Huang]

Attachment: Specifying alignments explicitly in assembly codes. r=derf

Added comments:

+   ; Explicitly specifying alignment here because some versions of
+   ; gas don't align code correctly. See
+   ; http://lists.gnu.org/archive/html/bug-binutils/2011-06/msg00199.html
+   ; https://bugzilla.mozilla.org/show_bug.cgi?id=920992
+   ALIGN
+

Comment 30

[Ting-Yuan Huang]

https://tbpl.mozilla.org/?tree=Try&rev=0b7f714bcd8a
https://tbpl.mozilla.org/?tree=Try&rev=44cd432aa7b2

Comment 31

[Glenn Randers-Pehrson]

Nitpick: The "last mod" dates in the patched files should probably be updated before checkin.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/08826e162b8a

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08826e162b8a

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7f8180acfc33