Closed Bug 1303247 Opened 3 years ago Closed 3 years ago

Check mp4parse-rust TrackInfo generation against stagefright's answers

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fix-optional
firefox52 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(2 files, 2 obsolete files)

Right now we're only comparing the audio and video track counts between mp4parse-rust and stagefright.  This bug tracks adding additional comparisons.  We might want to roll these results into the MP4PARSE_TRACK_MATCH_{AUDIO,VIDEO} telemetry or add similar new fields, but for now I want something I can use locally to catch errors.
Attached patch wip v0 (obsolete) — Splinter Review
Compare the TrackInfo results between metadata parsers.

mp4parse-rust isn't returning the audio profile/extended profile results yet, so those checks are disabled.

Patch also fixes:
- missing initialization of AudioInfo's mTrackId field
- MP4MetadataRust::GetTrackInfo returning null for valid tracks due to track index mismatches
My initial thought is to turn mRustTestMode into a hidden pref test, so I can enable this for my various nightly dogfood builds.
Fix the GetTrackInfo index mismatch and AudioInfo initialization bugs.
Attachment #8791861 - Attachment is obsolete: true
Attachment #8792379 - Flags: review?(giles)
Add a dev-only (hidden, not in release) pref to crash when Rust and Stagefright disagree.
Attachment #8792380 - Flags: review?(giles)
Comment on attachment 8792380 [details] [diff] [review]
0002-Bug-1303247-wip-Treat-mismatches-between-Rust-and-St.patch

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

Good idea!
Attachment #8792380 - Flags: review?(giles) → review+
Comment on attachment 8792379 [details] [diff] [review]
0001-Bug-1303247-Fix-track-indexing-and-AudioInfo-initial.patch

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

::: media/libstagefright/binding/MP4Metadata.cpp
@@ +678,5 @@
> +        index += 1;
> +      }
> +      break;
> +    default:
> +      break;

Wow, this is confusing! So the 'mismatch' issue is that aTrackNumber is the index of the requested track _within_a_list_of_just_that_type_? So when we ask for track 2, we must see e.g. the third audio track, not the third track of any kind? If so, how did this ever work?

I think the code is hard to read because the important part is when index is _not_ bumped, complicated with the audio/video switch statement. Please add a comment describing what this block is doing and why. And if I'm misunderstood what you're doing, please let me see it again.
Attachment #8792379 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) needinfo me from comment #6)
> Wow, this is confusing! So the 'mismatch' issue is that aTrackNumber is the
> index of the requested track _within_a_list_of_just_that_type_? So when we
> ask for track 2, we must see e.g. the third audio track, not the third track
> of any kind?

Right.

> If so, how did this ever work?

It didn't.

> I think the code is hard to read because the important part is when index is
> _not_ bumped, complicated with the audio/video switch statement. Please add
> a comment describing what this block is doing and why. And if I'm
> misunderstood what you're doing, please let me see it again.

Yeah, it's not ideal.  I thought about improvements before submitting but nothing obviously better came to mind.  I'll take another crack at it.  At least splitting the index conversion out from GetTrackInfo would help a bit.
Hopefully somewhat clearer now, but please let me know if you have suggestions.
Attachment #8792379 - Attachment is obsolete: true
Attachment #8792698 - Flags: review?(giles)
Comment on attachment 8792698 [details] [diff] [review]
0001-Bug-1303247-Fix-track-indexing-and-AudioInfo-initial.patch

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

Thanks more clear. Thanks!
Attachment #8792698 - Flags: review?(giles) → review+
Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/516597939e1a
Fix track indexing and AudioInfo initialization in Rust metadata parser.  r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/85cacbef3243
Add a dev-only pref to treat mismatches between Rust and Stagefright as a fatal error.  r=rillian
Sorry had to backout this for GTest Assertion failures, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=36188203&repo=mozilla-inbound#L2132
Flags: needinfo?(kinetik)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44af49566fa3
Backed out changeset 85cacbef3243 for GTest Assertion failures
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8648997af90
Backed out changeset 516597939e1a
Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffe4592857f
Fix track indexing and AudioInfo initialization in Rust metadata parser.  r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6c334cf228
Add a dev-only pref to treat mismatches between Rust and Stagefright as a fatal error.  r=rillian
(In reply to Iris Hsiao [:ihsiao] from comment #11)
> Sorry had to backout this for GTest Assertion failures, e.g.,
> https://treeherder.mozilla.org/logviewer.html#?job_id=36188203&repo=mozilla-
> inbound#L2132

Thanks.  85cacbef3243 exposed the MOZ_ASSERT(infoRust) in cases where we'd previously skip it because mPreferRust was false.  I moved the assert to a more suitable place and repushed after checking the GTests are passing locally.
Flags: needinfo?(kinetik)
https://hg.mozilla.org/mozilla-central/rev/0ffe4592857f
https://hg.mozilla.org/mozilla-central/rev/5f6c334cf228
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1305596
Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.
Depends on: 1306432
You need to log in before you can comment on or make changes to this bug.