1695033 - Webm files fail to play if they have metadata with dimensions greater than those derived form in band video information

Status: RESOLVED FIXED

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

Description

[Leo Smith]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

1. Installed firefox version 86 for Linux via Mint repository
2. attempted to connect to webm/vorbis streaming video (TV)
3. got error message Video can't be played because the file is corrupt
4. downgraded to previous version
5. stream plays correctly

Actual results:

got error message "Video can't be played because the file is corrupt"

Expected results:

stream plays correctly

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Audio/Video: Playback' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Leo Smith]

This is erroneously reported as a version 85 bug but it applies to version 86 <i>only</i>

Comment 3

[Leo Smith]

The streams in question are on a private server that streams live UK TV so are not legally accessible to non UK TV license holders. I can provide temporary login details to anyone working on the problem however.

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

I think I have a good handle what's going on here. The issue is the result of a few things:

• The site is sending a VP8 stream in a webm where the webm metadata and the VP8 stream data differ. The webm says the stream will be 544x576 but the vp8 stream says 362x384. This alone is not enough to cause a problem.
• However, bug 1637658 introduced some changes to how we handle the above case. This change overlooked the need to reset the image rectangle when resolutions change (as we do in other such cases[0]). This means we end up with frames that are mostly treated as 362x384, but which will eventually fail when we calculate the scaled image rectangle[1]. More specifically, the scaled image rect will be 544x576 and will fail and overflow check later where those dimensions are too large for the smaller 362x384 image[2].

Tests and fix incoming.

[0] https://searchfox.org/mozilla-central/rev/eeb8cf278192d68b3977d0adb4d43f1463439269/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#193
[1] https://searchfox.org/mozilla-central/rev/eeb8cf278192d68b3977d0adb4d43f1463439269/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp#701
[2] https://searchfox.org/mozilla-central/rev/eeb8cf278192d68b3977d0adb4d43f1463439269/dom/media/MediaData.cpp#192-193

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1695033 - Add test cases for vp8 container stream dimension mismatch. r?alwu

This patch

• adds several test files derived from bipbop.mp4. These files are re-encodes
  that used vp8 and opus. One reference file is added with metadata that is
  correct, and several others are added with bad metadata (it doesn't match the
  stream resolution).
• adds these files to the gPlayTests to ensure we can play them.

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1695033 - Log error message in OnAudioNotDecoded + OnVideoNotDecoded. r?alwu

This makes debugging decode failures using logs a little nicer when we hit these
cases.

Depends on D107723

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1695033 - Reset image rectangle in the MediaChangeMonitor when changing resolutions. r?alwu

This avoids an issue where we end up with invalid sizes if the metadata in a
webm file has a greater resolution than the in band video data.

Depends on D107724

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Bumping severity of this. This could have a pretty nasty consequences. The regressing bug intended to stop us incorrectly displaying videos where the initial metadata disagrees with the in band data. In that bug the initial data had smaller dimensions than the in band data and that worked okay, howwever, as seen here, if the initial data has bigger dimensions it's fatal. It's not clear to me how widespread sites doing this are (hopefully not), but playback/WebRTC would be broken for these sites.

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Try run in preparation for landing.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/459745537b9b
Add test cases for vp8 container stream dimension mismatch. r=alwu
https://hg.mozilla.org/integration/autoland/rev/a74c36f659b6
Log error message in OnAudioNotDecoded + OnVideoNotDecoded. r=alwu
https://hg.mozilla.org/integration/autoland/rev/db7158dfb86d
Reset image rectangle in the MediaChangeMonitor when changing resolutions. r=alwu

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/459745537b9b
https://hg.mozilla.org/mozilla-central/rev/a74c36f659b6
https://hg.mozilla.org/mozilla-central/rev/db7158dfb86d

Comment 12

[Julien Cristau [:jcristau]]

Given comment 8 do we want this in 87? We build the last beta today.

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 9207904 [details]
Bug 1695033 - Reset image rectangle in the MediaChangeMonitor when changing resolutions. r?alwu

Beta/Release Uplift Approval Request

• User impact if declined: Some videos will be fatally broken without a workaround. Affect media is VP8 (and likely VP9) videos contained in a WebM container where that container specifies greater dimensions than are in the actual video stream.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is small and and enforces a mechanism that already exists in other code paths (i.e. it's not introducing a major new media path). 'Reset image rectangle in the MediaChangeMonitor when changing resolutions.' is the only patch strictly needed, the others contain tests and log changes and do not need uplift if we wish to reduce the code volume. But similarly, they're unlikely to cause issue since they're not functional changes seen by end users.
• String changes made/needed:

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9207902 [details]
Bug 1695033 - Add test cases for vp8 container stream dimension mismatch. r?alwu

Thanks Bryce. Approved for 87.0b9

Comment 15

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/001e2f9d026c
https://hg.mozilla.org/releases/mozilla-beta/rev/d1ecd7e40cf7
https://hg.mozilla.org/releases/mozilla-beta/rev/0a445fa9981e