1161350 - integrate MooV track metadata parse written in rust

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

As an experiment we'd like to replace our remaining use of the stagefright mp4 parse with one written in rust.

Comment 1

[Ralph Giles (:rillian)]

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?

Comment 2

[Jean-Yves Avenard [:jya]]

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.

Comment 3

[Ralph Giles (:rillian)]

Attachment: WIP - mp4parse-rust v0.0.7

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.

Comment 4

[Ralph Giles (:rillian)]

Attachment: WIP - byteorder crate-as-module

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 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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?

Comment 6

[Ralph Giles (:rillian)]

(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.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8620501 [details] [diff] [review]
WIP - mp4parse-rust v0.0.7

See bug 1175322.

Comment 8

[Ralph Giles (:rillian)]

Passing the tracking bug to Alfredo, who has been doing most of the work on lately.

Comment 9

[Ralph Giles (:rillian)]

There are a few outstanding issues still, but this project is complete.