update rust mp4parse to v0.2.1

RESOLVED FIXED in Firefox 47

Status

()

P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(8 attachments, 9 obsolete attachments)

188.19 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
686 bytes, patch
kinetik
: review+
Details | Diff | Splinter Review
45.21 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
2.02 KB, patch
rillian
: review+
Details | Diff | Splinter Review
12.00 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
12.04 KB, patch
rillian
: review+
Details | Diff | Splinter Review
7.34 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
7.20 KB, patch
rillian
: review+
Details | Diff | Splinter Review
Update rust mp4 parser to v0.2.1 when it's available.
(Assignee)

Updated

3 years ago
Assignee: nobody → giles
Blocks: 1161350
(Assignee)

Comment 1

3 years ago
Created attachment 8712455 [details] [diff] [review]
Part 1 - Move rust code into a shared directory

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

Comment 2

3 years ago
Created attachment 8712456 [details] [diff] [review]
Part 2 - Update import script for v0.2.0
(Assignee)

Comment 3

3 years ago
Created attachment 8712457 [details] [diff] [review]
Part 3 - Update rust mp4parse to v0.2.0
(Assignee)

Comment 4

3 years ago
Created attachment 8712458 [details] [diff] [review]
Part 4 - Flip error code sign in telemetry reporting

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.
Attachment #8712458 - Flags: review-
Comment on attachment 8712455 [details] [diff] [review]
Part 1 - Move rust code into a shared directory

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

Excellent!
Attachment #8712455 - Flags: review+
(Assignee)

Comment 7

3 years ago
Created attachment 8712463 [details] [diff] [review]
Part 4 - Flip error code sign in telemetry reporting v2

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.
Attachment #8712458 - Attachment is obsolete: true
Attachment #8712463 - Flags: review?(kinetik)
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.
Attachment #8712463 - Flags: review?(kinetik) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8712542 [details] [diff] [review]
Part 2 - Update import script for v0.2.1

Update to 0.2.1 to pick up the string parsing fix.
Attachment #8712456 - Attachment is obsolete: true
Attachment #8712542 - Flags: review?(kinetik)
Created attachment 8712546 [details] [diff] [review]
Part 3 - Update rust mp4parse to v0.2.1
Attachment #8712457 - Attachment is obsolete: true
Attachment #8712546 - Flags: review?(kinetik)
Created attachment 8712547 [details] [diff] [review]
Part 4 - Flip error code sign in telemetry reporting v3

Apply comments from review, carrying forward r=kinetik.
Attachment #8712463 - Attachment is obsolete: true
Attachment #8712547 - Flags: review+
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.
Created attachment 8712550 [details] [diff] [review]
0001-Hide-MP4Metadata-behind-an-impl-pointer.patch

Refactoring work to clean up MP4Metadata.  The last part of this addresses the issues I raised in my last comment.
Attachment #8712550 - Flags: review?(giles)
Created attachment 8712551 [details] [diff] [review]
0002-Move-mp4parse-rust-code-into-MP4MetadataRust-impl.patch
Attachment #8712551 - Flags: review?(giles)
Created attachment 8712552 [details] [diff] [review]
0003-Remove-now-unnecessary-StagefrightPrivate-wrapper.patch
Attachment #8712552 - Flags: review?(giles)
Created attachment 8712553 [details] [diff] [review]
0004-Move-mp4parse-rust-initialization-into-constructor-a.patch

This series applies on top of Ralph's changes in attachment 8712547 [details] [diff] [review].
Attachment #8712553 - Flags: review?(giles)
Attachment #8712542 - Flags: review?(kinetik) → review+
Attachment #8712546 - Flags: review?(kinetik) → 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.
Attachment #8712550 - Flags: review?(giles) → review+
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
Attachment #8712551 - Flags: review?(giles)
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.
Attachment #8712552 - Flags: review?(giles) → review+
Comment hidden (obsolete)
Attachment #8712862 - Attachment description: Move mp4parse-rust code into MP4MetadataRust impl. r=giles → 0002-Move mp4parse-rust code into MP4MetadataRust impl. r=giles
Attachment #8712862 - Attachment is obsolete: true
Created attachment 8712905 [details] [diff] [review]
Part 5 - Hide MP4Metadata behind an impl pointer.

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.
Attachment #8712905 - Flags: review+
Attachment #8712550 - Attachment is obsolete: true
Created attachment 8712906 [details] [diff] [review]
Part 6 - Move mp4parse-rust code into MP4MetadataRust impl.  r?giles

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.
Attachment #8712906 - Flags: review?(giles)
Attachment #8712551 - Attachment is obsolete: true
Created attachment 8712907 [details] [diff] [review]
Part 7 - Remove now-unnecessary StagefrightPrivate wrapper.
Attachment #8712907 - Flags: review+
Attachment #8712552 - Attachment is obsolete: true
Created attachment 8712908 [details] [diff] [review]
Part 8 - Move mp4parse-rust initialization into constructor and clean up try_rust.  r?giles

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.
Attachment #8712908 - Flags: review?(giles)
Attachment #8712553 - Attachment is obsolete: true
Attachment #8712553 - Flags: review?(giles)
Verified all four patches in the series build with and without --enable-rust.
Blocks: 1240413
Blocks: 1240412
(Assignee)

Updated

3 years ago
Attachment #8712906 - Flags: review?(giles) → review+
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.
Attachment #8712908 - Flags: review?(giles) → review+
(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
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.