Closed Bug 1243234 Opened 5 years ago Closed 5 years ago

update rust mp4parse to v0.2.1

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(8 files, 9 obsolete files)

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: nobody → giles
Blocks: 1161350
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.
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+
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+
Update to 0.2.1 to pick up the string parsing fix.
Attachment #8712456 - Attachment is obsolete: true
Attachment #8712542 - Flags: review?(kinetik)
Attachment #8712457 - Attachment is obsolete: true
Attachment #8712546 - Flags: review?(kinetik)
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.
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)
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+
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
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
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
Attachment #8712552 - Attachment is obsolete: true
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.
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.