1229615 - Use the expanded mp4parse C API to compare results with the libstagefright parser

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Matthew Gregan [:kinetik]]

Once bug 1229612 lands, we can call mp4parse_get_track_info and compare the results with what we get from libstagefright.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: Part 2 - Compare track counts with stagefright

First attempt.

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 8694522 [details] [diff] [review]
Part 2 - Compare track counts with stagefright

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

Seems to work. Let's get this in tree.

Comment 3

[Ralph Giles (:rillian)]

Attachment: Part 1 - Don't reject files without edit lists

Backport your fix from upstream.

Comment 4

[Ralph Giles (:rillian)]

Attachment: Part 3 - Conditionalize mp4parse.h export

Fix up the export to follow the same conditional as the build.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8694892 [details] [diff] [review]
Part 3 - Conditionalize mp4parse.h export

Oops.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7bf9994e1b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/80aa7ecf9456
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa313047a6de

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee27d317a68 for crashtest failures: https://treeherder.mozilla.org/logviewer.html#?job_id=18174303&repo=mozilla-inbound

Comment 8

[Ralph Giles (:rillian)]

Fails on dom/media/test/crashtests/0-timescale.html

We're hitting an illegal instruction, seemingly in the rust panic! handler, so it's not being caught by the isolation thread.

In the linux64 debug build build, it dies at `libxul.so!rust_eh_personality_catch + 0x132` which disassembles to:

 30d7742:       0f 0b                   ud2    (https://pastebin.mozilla.org/8853666)

According to nagisa, this is is often used as an intrinsic to mark unreachable code. Question is, how did it get there?

On mac, the result is similar: EXC_BAD_INSTRUCTION at `XUL!rust_panic + 0x137`

Alex, any idea what's up here?

Comment 9

[Ralph Giles (:rillian)]

So what's going on here is that rust code is panic!()ing, but we don't do anything to catch that, which kills the process. We the capi.rs wraps the call to read_mp4 in a std::thread::spawn to catch issues like this, but we didn't do the same for the new getters on the parser context structure. So we either need to do that, for be a lot more careful eliminating panics.

The actual crash is a divide by zero in track_time_to_ms. If I add an assert!():

./mach crashtest dom/media/test/crashtests/0-timescale.html
[...]
 0:09.52 thread '<unnamed>' panicked at 'assertion failed: scale.0 > 0', /home/giles/mozilla/firefox/media/libstagefright/binding/MP4Metadata.rs:1055
 0:09.58 fatal runtime error: Could not unwind stack, error = 5

Comment 10

[Ralph Giles (:rillian)]

Attachment: Part 4 - Catch panics in mp4parse_get_track_info

This just papers over the underlying bug, but will let us reland and get more testing like the one which found this one.

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8695041 [details] [diff] [review]
Part 4 - Catch panics in mp4parse_get_track_info

I disagree with fixing it this way, but w/e.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3af84d7471c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d036b1daeb0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1d2251afd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/822004606576

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3af84d7471c8
https://hg.mozilla.org/mozilla-central/rev/d036b1daeb0b
https://hg.mozilla.org/mozilla-central/rev/7f1d2251afd6
https://hg.mozilla.org/mozilla-central/rev/822004606576