Closed Bug 920992 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
ProductDemo
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: tommy.cvkk, Assigned: laszio.bugzilla)

References

Details

(Whiteboard: [Flatfish only])

Attachments

(2 files, 1 obsolete file)

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

Would you mind to upload the sample ogv or ogg file here??
Attached video It's a normal ogv file.
Depends on: flatfish
Boot2Gecko is based on Android 4.2 version.
Blocks: flatfish
No longer depends on: flatfish
We can play it in Inari with 20130925172950, b2g27. It looks like device dependent bug.
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]
==================================
Marco, could you help on this?
Assignee: nobody → mchen
blocking-b2g: --- → koi+
Whiteboard: [Flatfish only]
Blocks: 923437
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 ?
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.
( 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)
Blocks: 922972
(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.
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)
No longer blocks: 922972
Blocks: 924015
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
After adding ".align 2", it works fine, thanks~
Hi Ting-Yuan,

Thanks for your help.
And may I know you will summit a patch for ".align 4"?
Flags: needinfo?(thuang)
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.
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.
Flags: needinfo?(thuang)
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.
(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?
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.
I think you are looking at this GAS bug:

http://lists.gnu.org/archive/html/bug-binutils/2011-06/msg00199.html
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.
(In reply to jbowler@acm.org 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.
>we can't make changes to the whole B2G toolchain just [to fix a bug in the toolchain]

Oh well.
(In reply to Timothy B. Terriberry (:derf) from comment #22)
> (In reply to jbowler@acm.org 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.
> 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"
(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.
Attached patch alignments.patch (obsolete) — Splinter Review
"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.
Attachment #824484 - Flags: review?(tterribe)
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.
Attachment #824484 - Flags: review?(tterribe) → review+
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
+
Attachment #824484 - Attachment is obsolete: true
Attachment #824526 - Flags: review+
See Also: → 832390
Nitpick: The "last mod" dates in the patched files should probably be updated before checkin.
https://hg.mozilla.org/mozilla-central/rev/08826e162b8a
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee: mchen → thuang
Duplicate of this bug: 923437
You need to log in before you can comment on or make changes to this bug.