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

RESOLVED FIXED in Firefox 28, Firefox OS v1.2

Status

Firefox OS
Gaia::Video
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tommy_chiu, Assigned: Ting-Yuan Huang)

Tracking

(Blocks: 2 bugs)

unspecified
ProductDemo
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [Flatfish only])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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??
(Reporter)

Comment 2

4 years ago
Created attachment 810934 [details]
It's a normal ogv file.
(Reporter)

Updated

4 years ago
Depends on: 903304
(Reporter)

Comment 3

4 years ago
Boot2Gecko is based on Android 4.2 version.

Updated

4 years ago
Blocks: 903304
No longer depends on: 903304
We can play it in Inari with 20130925172950, b2g27. It looks like device dependent bug.
(Reporter)

Comment 5

4 years ago
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+

Updated

4 years ago
Whiteboard: [Flatfish only]
(Reporter)

Updated

4 years ago
Blocks: 923437
(Reporter)

Comment 7

4 years ago
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

4 years ago
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.
(Reporter)

Comment 9

4 years ago
( 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)

Updated

4 years ago
Blocks: 922972

Comment 10

4 years ago
(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.
(Reporter)

Comment 11

4 years ago
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)

Updated

4 years ago
No longer blocks: 922972

Updated

4 years ago
Blocks: 924015
(Assignee)

Comment 12

4 years ago
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
(Reporter)

Comment 13

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

Comment 14

4 years ago
Hi Ting-Yuan,

Thanks for your help.
And may I know you will summit a patch for ".align 4"?
Flags: needinfo?(thuang)

Comment 15

4 years ago
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.
(Assignee)

Comment 16

4 years ago
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)

Comment 17

4 years ago
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.
(Assignee)

Comment 18

4 years ago
(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

4 years ago
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

4 years ago
I think you are looking at this GAS bug:

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

Comment 21

4 years ago
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.

Comment 23

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

Oh well.
(Assignee)

Comment 24

4 years ago
(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.
(Assignee)

Comment 25

4 years ago
> 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.
(Assignee)

Comment 27

4 years ago
Created attachment 824484 [details] [diff] [review]
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.
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+
(Assignee)

Comment 29

4 years ago
Created attachment 824526 [details] [diff] [review]
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
+
Attachment #824484 - Attachment is obsolete: true
Attachment #824526 - Flags: review+
(Assignee)

Comment 30

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0b7f714bcd8a
https://tbpl.mozilla.org/?tree=Try&rev=44cd432aa7b2
Keywords: checkin-needed

Updated

4 years ago
See Also: → bug 832390
Nitpick: The "last mod" dates in the patched files should probably be updated before checkin.
https://hg.mozilla.org/integration/b2g-inbound/rev/08826e162b8a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08826e162b8a
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7f8180acfc33
status-b2g-v1.2: --- → fixed
status-firefox26: --- → wontfix
status-firefox27: --- → wontfix
status-firefox28: --- → fixed

Updated

4 years ago
Assignee: mchen → thuang

Updated

4 years ago
Duplicate of this bug: 923437
You need to log in before you can comment on or make changes to this bug.