1331330 - Support index table from rust parser to gecko

Status: RESOLVED FIXED

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

Comment 1

[Alfredo Yang (:alfredo)]

We may pass the raw data pointer of rust vector to Index [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/media/libstagefright/binding/Index.cpp#236

Comment 2

[Alfredo Yang (:alfredo)]

https://github.com/mozilla/mp4parse-rust/pull/59

Comment 3

[Alfredo Yang (:alfredo)]

There will be several PRs for this bug and that's the final piece for mp4parser.

I plan to have another gecko patch which comparing to the stagefright result. And then we can check the result from test case.

That's the first step to turn on this module officially.

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: Bug 1331330 - compare rust parser and stagefright sample table.

Review commit: https://reviewboard.mozilla.org/r/107358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107358/

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8830614 [details]
Bug 1331330 - compare rust parser and stagefright sample table.

https://reviewboard.mozilla.org/r/107358/#review109496

::: media/libstagefright/binding/MP4Metadata.cpp:371
(Diff revision 1)
> -#ifdef MOZ_RUST_MP4PARSE
> -  if (mRust && mPreferRust && mRust->ReadTrackIndex(aDest, aTrackID)) {
> -    return true;
> +  bool ret = mStagefright->ReadTrackIndex(aDest, aTrackID);
> +
> +#ifndef RELEASE_OR_BETA
> +  if (mRustTestMode && ret && mRust) {
> +    mp4parse_byte_data data = {};
> +    mRust->ReadTrackIndice(&data, aTrackID);

Handle an error return from ReadTrackIndice here.

::: media/libstagefright/binding/MP4Metadata.cpp:407
(Diff revision 1)
>      indice.end_composition = s_indice.end_composition - aMediaTime;
>      indice.start_decode = s_indice.start_decode;
>      indice.sync = s_indice.sync;
>      // FIXME: Make this infallible after bug 968520 is done.
>      MOZ_ALWAYS_TRUE(aDest.AppendElement(indice, mozilla::fallible));
> +    MOZ_LOG(sLog, LogLevel::Debug, ("s_o: %lld, e_o: %lld, s_c: %lld, e_c: %lld, s_d: %lld, sync: %d\n",

%lld expects signed but these fields are u64 in Rust.  Also the base type can be long or long long on different platforms, so you need to use PRIu64 rather than encoding the format string directly.

Comment 6

[Alfredo Yang (:alfredo)]

https://github.com/mozilla/mp4parse-rust/pull/61

Comment 7

[Alfredo Yang (:alfredo)]

https://github.com/mozilla/mp4parse-rust/pull/64

Comment 8

[Alfredo Yang (:alfredo)]

Comment on attachment 8830614 [details]
Bug 1331330 - compare rust parser and stagefright sample table.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107358/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47b318d87004
compare rust parser and stagefright sample table. r=kinetik

Comment 10

[Iris Hsiao [:ihsiao]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=79261624&repo=autoland&lineNumber=1369

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a1aabfac756
Backed out changeset 47b318d87004 for bustage

Comment 12

[Alfredo Yang (:alfredo)]

(In reply to Iris Hsiao [:ihsiao] from comment #10)
> sorry had to back this out for bustage like
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=79261624&repo=autoland&lineNumber=1369

Hmm... I should land bug 1340980 first and then this one. It should be ok if they build together.
Anyway, I'll land it again.

Comment 13

[Alfredo Yang (:alfredo)]

Comment on attachment 8830614 [details]
Bug 1331330 - compare rust parser and stagefright sample table.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107358/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b69cf392d0a
compare rust parser and stagefright sample table. r=kinetik

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b69cf392d0a

Comment 16

[Edmund Wong (:ewong)]

Can someone point out if there's a missing defined value I need to set up to fix
bug 1342291?

Comment 17

[Alfredo Yang (:alfredo)]

(In reply to Edmund Wong (:ewong) from comment #16)
> Can someone point out if there's a missing defined value I need to set up to
> fix
> bug 1342291?

Have you tried clear build?

Comment 18

[Edmund Wong (:ewong)]

(In reply to Alfredo Yang (:alfredo) from comment #17)
> (In reply to Edmund Wong (:ewong) from comment #16)
> > Can someone point out if there's a missing defined value I need to set up to
> > fix
> > bug 1342291?
> 
> Have you tried clear build?

While the macs aren't clobbered, the linux builders are always clobbered so
every build is done in a clean directory.

Comment 19

[Edmund Wong (:ewong)]

(In reply to Alfredo Yang (:alfredo) from comment #17)
> (In reply to Edmund Wong (:ewong) from comment #16)
> > Can someone point out if there's a missing defined value I need to set up to
> > fix
> > bug 1342291?
> 
> Have you tried clear build?

Is something like bug 1342291 avoided with adding MOZ_RUST_MP4PARSE=1,
as in bug 1219530?

Comment 20

[Alfredo Yang (:alfredo)]

(In reply to Edmund Wong (:ewong) from comment #19)
> (In reply to Alfredo Yang (:alfredo) from comment #17)
> > (In reply to Edmund Wong (:ewong) from comment #16)
> > > Can someone point out if there's a missing defined value I need to set up to
> > > fix
> > > bug 1342291?
> > 
> > Have you tried clear build?
> 
> Is something like bug 1342291 avoided with adding MOZ_RUST_MP4PARSE=1,
> as in bug 1219530?

I saw your patch in bug 1342291, do your build target for firefox browser?
The definition in "browser/confvars.sh" is for browser.