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

RESOLVED FIXED in Firefox 45

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kinetik, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Once bug 1229612 lands, we can call mp4parse_get_track_info and compare the results with what we get from libstagefright.
(Reporter)

Comment 1

2 years ago
Created attachment 8694522 [details] [diff] [review]
Part 2 - Compare track counts with stagefright

First attempt.
Blocks: 1161350
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.
Attachment #8694522 - Flags: review+
Created attachment 8694891 [details] [diff] [review]
Part 1 - Don't reject files without edit lists

Backport your fix from upstream.
Attachment #8694891 - Flags: review?(kinetik)
Created attachment 8694892 [details] [diff] [review]
Part 3 - Conditionalize mp4parse.h export

Fix up the export to follow the same conditional as the build.
Attachment #8694892 - Flags: review?(kinetik)
Attachment #8694522 - Attachment description: bug1229615_wip_v0.patch → Part 2 - Compare track counts with stagefright
(Reporter)

Updated

2 years ago
Attachment #8694891 - Flags: review?(kinetik) → review+
(Reporter)

Comment 5

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

Oops.
Attachment #8694892 - Flags: review?(kinetik) → review+

Comment 6

2 years ago
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
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
Flags: needinfo?(giles)
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?
Flags: needinfo?(giles) → needinfo?(acrichton)
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
Flags: needinfo?(acrichton)
Created attachment 8695041 [details] [diff] [review]
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.
Attachment #8695041 - Flags: review?(kinetik)
(Reporter)

Comment 11

2 years ago
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.
Attachment #8695041 - Flags: review?(kinetik) → review+

Comment 12

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

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.