1243234 - update rust mp4parse to v0.2.1

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

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

Comment 1

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 2

[Ralph Giles (:rillian)]

Attachment: Part 2 - Update import script for v0.2.0

Comment 3

[Ralph Giles (:rillian)]

Attachment: Part 3 - Update rust mp4parse to v0.2.0

Comment 4

[Ralph Giles (:rillian)]

Attachment: Part 4 - Flip error code sign in telemetry reporting

Reflect change to positive error codes.

Comment 5

[Matthew Gregan [:kinetik]]

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 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8712455 [details] [diff] [review]
Part 1 - Move rust code into a shared directory

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

Excellent!

Comment 7

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 8

[Matthew Gregan [: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.

Comment 9

[Ralph Giles (:rillian)]

Attachment: Part 2 - Update import script for v0.2.1

Update to 0.2.1 to pick up the string parsing fix.

Comment 10

[Ralph Giles (:rillian)]

Attachment: Part 3 - Update rust mp4parse to v0.2.1

Comment 11

[Ralph Giles (:rillian)]

Attachment: Part 4 - Flip error code sign in telemetry reporting v3

Apply comments from review, carrying forward r=kinetik.

Comment 12

[Matthew Gregan [: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.

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 14

[Matthew Gregan [:kinetik]]

Attachment: 0002-Move-mp4parse-rust-code-into-MP4MetadataRust-impl.patch

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: 0003-Remove-now-unnecessary-StagefrightPrivate-wrapper.patch

Comment 16

[Matthew Gregan [:kinetik]]

Attachment: 0004-Move-mp4parse-rust-initialization-into-constructor-a.patch

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

Comment 17

[Ralph Giles (:rillian)]

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 18

[Ralph Giles (:rillian)]

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 19

[Ralph Giles (:rillian)]

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.

Comment 20

[Matthew Gregan [:kinetik]]

Attachment: 0002-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.

Comment 21

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 22

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 23

[Matthew Gregan [:kinetik]]

Attachment: Part 7 - Remove now-unnecessary StagefrightPrivate wrapper.

Comment 24

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 25

[Matthew Gregan [:kinetik]]

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

Comment 26

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15ca7a6b9fc1

Comment 27

[Ralph Giles (:rillian)]

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.

Comment 28

[Matthew Gregan [:kinetik]]

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

Comment 29

[Matthew Gregan [:kinetik]]

Scrubbed the last try due to a trivial compile error on OS X:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d47ddead0a

Comment 30

[Pulsebot]

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

Comment 31

[Carsten Book [:Tomcat]]

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

Comment 32

[Carsten Book [:Tomcat]]

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