integrate MooV track metadata parse written in rust

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: rillian, Assigned: ayang)

Tracking

(Depends on 2 bugs)

Trunk
Points:
---

Firefox Tracking Flags

(firefox40 affected)

Details

(URL)

Attachments

(2 obsolete attachments)

As an experiment we'd like to replace our remaining use of the stagefright mp4 parse with one written in rust.
I've been experimenting with rust code over at https://notabug.org/rillian/mp4parse-rust

Not much to show so far. Finding my way through the standard library and trying to pick a reasonable abstraction. Is Reader or borrowed ranges is a better idea?
Ideally integration would be in the MP4Metadata class. Not knowing rust and how it can integrate in our C++ code I don't know the feasibility of things.
Depends on: 1156689
Posted patch WIP - mp4parse-rust v0.0.7 (obsolete) — Splinter Review
Anthony, this is what the code looks like currently. Let me know if you have any suggestions.

NB this version doesn't build within gecko because of the byteorder dep.
Attachment #8620501 - Flags: feedback?(ajones)
Source for the byteorder crate dependency, under the MIT license. Hacked up so it can be built as a module rather than an external crate.

Upstream is https://crates.io/crates/byteorder
Comment on attachment 8620501 [details] [diff] [review]
WIP - mp4parse-rust v0.0.7

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

Nice work!

::: media/libstagefright/binding/MP4Metadata.rs
@@ +53,5 @@
> +    let size = match tmp_size {
> +        1 => src.read_u64::<BigEndian>().unwrap(),
> +        _ => tmp_size as u64,
> +    };
> +    assert!(size >= 8);

Note: jya landed a patch that relates to a valid 0 size which means "the rest of the file".

@@ +264,5 @@
> +             (u >>  8 & 0xffu32) as u8,
> +             (u & 0xffu32) as u8)
> +    };
> +    let name_bytes = u32_to_vec(name);
> +    String::from_utf8_lossy(&name_bytes).into_owned()

Notes for future: C++ supports a 4 byte character type (e.g. 'moov') in some compilers. Does rust have something similar? Do rust strings optimise as well as C++ in this instance?
Attachment #8620501 - Flags: feedback?(ajones) → feedback+
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #5)

> Note: jya landed a patch that relates to a valid 0 size which means "the
> rest of the file".

Yes. Hopefully that's only used for mdat though?

> Notes for future: C++ supports a 4 byte character type (e.g. 'moov') in some
> compilers. Does rust have something similar? Do rust strings optimise as
> well as C++ in this instance?

Rust doesn't have anything like Apple's fourcc literals extension for C/C++. There is a syntax extension mechanism which can do the equivalent of C++ custom literals, I think.

fourcc_to_string() necessarily allocates because it returns a String object wrapping its own copy of the 4 bytes. If inlined somewhere I think it could be optimized into a move, but the rust compiler isn't that sophisticated. I suspect the `match` ends up doing per-byte string compares too.

Anyway, I hadn't tried harder that this because dispatch isn't performance critical for metadata parsing, and this was easier to read than the direct numerical comparisions I used in the unit tests.
Depends on: 1174356
Depends on: 1175322
Comment on attachment 8620501 [details] [diff] [review]
WIP - mp4parse-rust v0.0.7

See bug 1175322.
Attachment #8620501 - Attachment is obsolete: true
Attachment #8620597 - Attachment is obsolete: true
Depends on: 1175746
Depends on: 1179885
Component: Audio/Video → Audio/Video: Playback
Depends on: 1215234
Depends on: 1215696
Depends on: 1217261
Depends on: 1219047
Depends on: 1219452
Depends on: 1220882
Depends on: 1220885
Depends on: 1221656
Depends on: 1229612
Depends on: 1229615
Depends on: 1231169
Depends on: 1238420
Depends on: vp9-in-mp4
Depends on: opus-in-mp4
Depends on: 1243234
Depends on: 1259282
Depends on: 1266792
Depends on: 1284589
Depends on: 1295666
Depends on: 1302027
(Assignee)

Updated

3 years ago
Depends on: 1302593
(Assignee)

Updated

3 years ago
Depends on: 1304254
(Assignee)

Updated

3 years ago
Depends on: 1306236
Depends on: 1306755
(Assignee)

Updated

3 years ago
Depends on: 1307045
(Assignee)

Updated

3 years ago
Depends on: 1309079
(Assignee)

Updated

3 years ago
Depends on: 1311627
(Assignee)

Updated

3 years ago
Depends on: 1311929
(Assignee)

Updated

3 years ago
Depends on: 1313556
Depends on: 1314460
(Assignee)

Updated

2 years ago
Depends on: 1317609
(Assignee)

Updated

2 years ago
Depends on: 1320026
(Assignee)

Updated

2 years ago
Depends on: 1323368
(Assignee)

Updated

2 years ago
Depends on: 1323390
(Assignee)

Updated

2 years ago
Depends on: 1328221
(Assignee)

Updated

2 years ago
Depends on: 1331330
(Assignee)

Updated

2 years ago
Depends on: 1340446
(Assignee)

Updated

2 years ago
Depends on: 1340447
(Assignee)

Updated

2 years ago
Depends on: 1340980
(Assignee)

Updated

2 years ago
Depends on: 1341221
(Assignee)

Updated

2 years ago
Depends on: 1341967
(Assignee)

Updated

2 years ago
Depends on: 1342852
(Assignee)

Updated

2 years ago
Depends on: 1343793
(Assignee)

Updated

2 years ago
Depends on: 1345382
(Assignee)

Updated

2 years ago
Depends on: 1347765
(Assignee)

Updated

2 years ago
Depends on: 1347834
(Assignee)

Updated

2 years ago
Depends on: 1349133
Depends on: 1350200
(Assignee)

Updated

2 years ago
Depends on: 1354963
(Assignee)

Updated

2 years ago
Depends on: 1356132
(Assignee)

Updated

2 years ago
Depends on: 1358024
(Assignee)

Updated

2 years ago
Depends on: 1358068
(Assignee)

Updated

2 years ago
Depends on: 1359331
Passing the tracking bug to Alfredo, who has been doing most of the work on lately.
Assignee: giles → ayang
(Assignee)

Updated

2 years ago
Depends on: 1363669
(Assignee)

Updated

2 years ago
Depends on: 1372838
(Assignee)

Updated

2 years ago
Depends on: 1374194
(Assignee)

Updated

2 years ago
Depends on: 1378607
(Assignee)

Updated

2 years ago
Depends on: 1381750
(Assignee)

Updated

a year ago
Depends on: 1408298
There are a few outstanding issues still, but this project is complete.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.