Update rust mp4 parser to v0.2.1 when it's available.

Some WIP patches based on v0.2.0. Re-arrange the code into a proper module hierarchy within a single directory. This makes it more clear which rust file is part of what module.

Reflect change to positive error codes.

Comment on attachment 8712458 [details] [diff] [review] Part 4 - Flip error code sign in telemetry reporting Review of attachment 8712458 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libstagefright/binding/MP4Metadata.cpp @@ +145,2 @@ > rust_mp4parse_success == 0); > if (rust_mp4parse_success < 0) { This needs to be updated.

Comment on attachment 8712455 [details] [diff] [review] Part 1 - Move rust code into a shared directory Review of attachment 8712455 [details] [diff] [review]: ----------------------------------------------------------------- Excellent!

Oops, thanks for catching that. Updated. I think I found a bug in the track comparison telemetry too. We were only counting tracks when parsing failed.

Comment on attachment 8712463 [details] [diff] [review] Part 4 - Flip error code sign in telemetry reporting v2 Review of attachment 8712463 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libstagefright/binding/MP4Metadata.cpp @@ +142,5 @@ > mRustState.reset(mp4parse_new()); > int32_t rust_mp4parse_success = try_rust(mRustState, mSource); > Telemetry::Accumulate(Telemetry::MEDIA_RUST_MP4PARSE_SUCCESS, > + rust_mp4parse_success == MP4PARSE_OK); > + if (rust_mp4parse_success > 0) { Maybe make this != MP4PARSE_OK so we're consistent with using the error defines everywhere, then assert it's positive inside the test. @@ +179,4 @@ > #ifdef MOZ_RUST_MP4PARSE > uint32_t rust_total = 0; > const char* rust_track_type = nullptr; > + if (rust_mp4parse_success == MP4PARSE_OK && rust_tracks > 0) { Urgh. :-( Okay, *now* I want to get 0.2.1 landed, otherwise we're going to be waiting on getting good telemetry.

Update to 0.2.1 to pick up the string parsing fix.

Apply comments from review, carrying forward r=kinetik.

Random thoughts while refactoring some of the code in MP4Metadata.cpp: Now that I look more, try_rust is returning false for all of the buffer setup failures, which will coerce to the same value as MP4PARSE_OK, so we need to fix that too. Because it lives outside of mp4parse we don't have an MP4_PARSE_xxx error code suitable, but I think we want to report it somehow to work out when that code of chunk is causing the parser not to run at all. Using MP4PARSE_ERROR_EOF for this purpose might be okay, although potentially confusing if only reviewing the Rust code for places where it's returned. try_rust should probably also limit 'length' to avoid allocating a huge buffer and reading the entire stream. It probably makes sense to only report telemetry results for the track number comparison once per stream, rather than every time GetNumberTracks is called.

Refactoring work to clean up MP4Metadata. The last part of this addresses the issues I raised in my last comment.

This series applies on top of Ralph's changes in attachment 8712547 [details] [diff] [review].

Comment on attachment 8712550 [details] [diff] [review] 0001-Hide-MP4Metadata-behind-an-impl-pointer.patch Review of attachment 8712550 [details] [diff] [review]: ----------------------------------------------------------------- Do wrappers like this get optimized out by the compiler? Just curious, the overhead wouldn't matter for this object.

Comment on attachment 8712551 [details] [diff] [review] 0002-Move-mp4parse-rust-code-into-MP4MetadataRust-impl.patch Review of attachment 8712551 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing the refactoring. Idea is fine, but doesn't compile without MOZ_RUST_MP4PARSE. Patch also didn't apply cleanly for me. ::: media/libstagefright/binding/MP4Metadata.cpp @@ +155,5 @@ > uint32_t > MP4Metadata::GetNumberTracks(mozilla::TrackInfo::TrackType aType) const > { > + uint32_t numTracks = mStagefright->GetNumberTracks(aType); > + if (mRust) { Early return is better than large conditional blocks: if (mRust) { return numTracks; } Unfortunately this needs to be #ifdef MOZ_RUST_MP4PARSE as well. ::: media/libstagefright/binding/include/mp4_demuxer/MP4Metadata.h @@ +37,4 @@ > > private: > UniquePtr<MP4MetadataStagefright> mStagefright; > + UniquePtr<MP4MetadataRust> mRust; I think this needs to be #ifdef MOZ_RUST_MP4PARSE as well. UniquePtr<>'s dtor requires a complete type. Gcc warns about this, and there's a static assert at https://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#481

Comment on attachment 8712552 [details] [diff] [review] 0003-Remove-now-unnecessary-StagefrightPrivate-wrapper.patch Review of attachment 8712552 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libstagefright/binding/MP4Metadata.cpp @@ +227,5 @@ > > MP4MetadataStagefright::MP4MetadataStagefright(Stream* aSource) > + : mSource(aSource) > + , mMetadataExtractor(new MPEG4Extractor(new DataSourceAdapter(mSource))) > + , mCanSeek(mMetadataExtractor->flags() & MediaExtractor::CAN_SEEK) Nice use of initializer order.

This is temporary (until libstagefright is removed) and intended to make swapping between and comparing the results of the libstagefright and mp4parse-rust versions simpler.

Most of the interface is stubbed with asserts and only GetNumberTracks() is called on both libstagefright and mp4parse-rust variants. This also moves the libstagefright vs mp4parse-rust comparisons up into MP4Metadata.

Initializing in the constructor better matches libstagefright's behaviour (and avoids reading and copying the stream contents every time GetNumberTracks() is called). Also restricts the size of the buffer to 1MB. This will be handled in the future by passing the parser a DataSource-like interface for reading from the stream.

Verified all four patches in the series build with and without --enable-rust.

Comment on attachment 8712908 [details] [diff] [review] Part 8 - Move mp4parse-rust initialization into constructor and clean up try_rust. r?giles Review of attachment 8712908 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libstagefright/binding/MP4Metadata.cpp @@ +473,3 @@ > if (!rv || bytes_read != size_t(length)) { > MOZ_LOG(sLog, LogLevel::Warning, ("Error copying mp4 data")); > + return MP4PARSE_ERROR_EOF; MP4PARSE_ERROR_IO might be appropriate here too. It's probably what a real stream abstraction would return through the rust capi.

(In reply to Ralph Giles (:rillian) from comment #27) > MP4PARSE_ERROR_IO might be appropriate here too. It's probably what a real > stream abstraction would return through the rust capi. Yeah, that's definitely better, I'll change it before landing.

Scrubbed the last try due to a trivial compile error on OS X: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d47ddead0a

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc6ad5f9d8f https://hg.mozilla.org/integration/mozilla-inbound/rev/788a4f04a301 https://hg.mozilla.org/integration/mozilla-inbound/rev/71d2dfeccba4 https://hg.mozilla.org/integration/mozilla-inbound/rev/63bfa2308127 https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ea7e96ab01 https://hg.mozilla.org/integration/mozilla-inbound/rev/76030efe40a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/a40b8b886155 https://hg.mozilla.org/integration/mozilla-inbound/rev/e3212c974c62

https://hg.mozilla.org/mozilla-central/rev/8bc6ad5f9d8f https://hg.mozilla.org/mozilla-central/rev/788a4f04a301 https://hg.mozilla.org/mozilla-central/rev/71d2dfeccba4 https://hg.mozilla.org/mozilla-central/rev/63bfa2308127 https://hg.mozilla.org/mozilla-central/rev/a5ea7e96ab01 https://hg.mozilla.org/mozilla-central/rev/76030efe40a1 https://hg.mozilla.org/mozilla-central/rev/a40b8b886155 https://hg.mozilla.org/mozilla-central/rev/e3212c974c62

https://hg.mozilla.org/mozilla-central/rev/8bc6ad5f9d8f https://hg.mozilla.org/mozilla-central/rev/788a4f04a301 https://hg.mozilla.org/mozilla-central/rev/71d2dfeccba4 https://hg.mozilla.org/mozilla-central/rev/63bfa2308127 https://hg.mozilla.org/mozilla-central/rev/a5ea7e96ab01 https://hg.mozilla.org/mozilla-central/rev/76030efe40a1 https://hg.mozilla.org/mozilla-central/rev/a40b8b886155 https://hg.mozilla.org/mozilla-central/rev/e3212c974c62