Call rust mp4parser from MP4Metadata

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Anthony tells me sooner is better than later, so we should land my hacked-up buffer-copy patch calling the rust parser on init segments as soon as it's safe.
Posted patch Call rust mp4 parser v1 (obsolete) — Splinter Review
Here's my WIP hack. Needs conditionals, and seems to crash on quit.

Anthony, you can apply this and compile with `ac_add_options --enable-rust` in your mozconfig. Then:

./mach run bruce_vs_ironman_frag.mp4
[...]
rust parser found 2 tracks
Flags: needinfo?(ajones)
Depends on: 1219530
Pushed a version with MOZ_RUST conditionals (needs bug 1219530) to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fb0897f1f98

I don't see the shutdown crash on Linux x64 debug with rustc 1.3.0... it'd be good to track that down.
The crash happened when I killed firefox (ctrl-C on the terminal) while a video was playing. Doesn't seem to happen if I quit from the gui.
Depends on: 1220754
Depends on: 1219983
Posted patch Call rust mp4 parser v2 (obsolete) — Splinter Review
Let's get something landed.
Attachment #8679721 - Attachment is obsolete: true
Attachment #8682104 - Flags: review?(kinetik)
Comment on attachment 8682104 [details] [diff] [review]
Call rust mp4 parser v2

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

r+ with assert converted to an error return.

::: media/libstagefright/binding/MP4Metadata.cpp
@@ +118,5 @@
> +    fprintf(stderr, "Couldn't get source length\n");
> +    return false;
> +  }
> +  fprintf(stderr, "Source length %d bytes\n", (long long int)length);
> +  MOZ_ASSERT(length > 0);

My earlier try run showed this failing; Length() can return -1 for unknown stream lengths I guess.
Attachment #8682104 - Flags: review?(kinetik) → review+
Convert assert to error return per review comment. Carrying forward r=kinetik.
Attachment #8682104 - Attachment is obsolete: true
Attachment #8682169 - Flags: review+
This patch will be functional once bug 1219530 lands. Until then you need to #define MOZ_RUST_MP4PARSE in media/libstagefright/binding/MP4Metadata.cpp.
Flags: needinfo?(ajones)
Blocks: 1220882
Blocks: 1220885

Comment 9

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a48d8551417
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.