1303247 - Check mp4parse-rust TrackInfo generation against stagefright's answers

Status: RESOLVED FIXED

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

Description

[Matthew Gregan [:kinetik]]

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.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: wip v0

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

Comment 2

[Matthew Gregan [:kinetik]]

My initial thought is to turn mRustTestMode into a hidden pref test, so I can enable this for my various nightly dogfood builds.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: 0001-Bug-1303247-Fix-track-indexing-and-AudioInfo-initial.patch

Fix the GetTrackInfo index mismatch and AudioInfo initialization bugs.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: 0002-Bug-1303247-wip-Treat-mismatches-between-Rust-and-St.patch

Add a dev-only (hidden, not in release) pref to crash when Rust and Stagefright disagree.

Comment 5

[Ralph Giles (:rillian)]

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!

Comment 6

[Ralph Giles (:rillian)]

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.

Comment 7

[Matthew Gregan [:kinetik]]

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

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: 0001-Bug-1303247-Fix-track-indexing-and-AudioInfo-initial.patch

Hopefully somewhat clearer now, but please let me know if you have suggestions.

Comment 9

[Ralph Giles (:rillian)]

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!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Iris Hsiao [:ihsiao]]

Sorry had to backout this for GTest Assertion failures, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=36188203&repo=mozilla-inbound#L2132

Comment 12

[Pulsebot]

Backout by [e-mail]:
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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Matthew Gregan [:kinetik]]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ffe4592857f
https://hg.mozilla.org/mozilla-central/rev/5f6c334cf228

Comment 16

[Gerry Chang [:gchang]]

Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.