Closed Bug 1331330 Opened 3 years ago Closed 3 years ago

Support index table from rust parser to gecko

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(1 file)

No description provided.
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
Assignee: nobody → ayang
Blocks: 1302027
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 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.
Attachment #8830614 - Flags: review?(kinetik) → review+
Blocks: 1161350
No longer blocks: 1302027
Blocks: 1340446
Blocks: 1340447
Blocks: 1341221
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47b318d87004
compare rust parser and stagefright sample table. r=kinetik
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=79261624&repo=autoland&lineNumber=1369
Flags: needinfo?(ayang)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a1aabfac756
Backed out changeset 47b318d87004 for bustage
Flags: needinfo?(ayang)
(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.
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b69cf392d0a
compare rust parser and stagefright sample table. r=kinetik
https://hg.mozilla.org/mozilla-central/rev/7b69cf392d0a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Can someone point out if there's a missing defined value I need to set up to fix
bug 1342291?
(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?
(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.
(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?
(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.
You need to log in before you can comment on or make changes to this bug.