video aspect changes during playback
Categories
(Core :: Audio/Video: Playback, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox66 | --- | wontfix |
firefox67 | --- | verified |
firefox68 | --- | verified |
People
(Reporter: bzipitidoo, Assigned: bryce)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:65.0) Gecko/20100101 Firefox/65.0
Steps to reproduce:
Viewed the following video:
http://merlinchapin.6te.net/1962_Tripoli_winter.webm
ffprobe reports:
Input #0, matroska,webm, from '1962_Tripoli_winter.webm':
Metadata:
COMPATIBLE_BRANDS: qt
MAJOR_BRAND : qt
MINOR_VERSION : 537199360
ENCODER : Lavf57.83.100
Duration: 00:04:08.29, start: 0.000000, bitrate: 584 kb/s
Stream #0:0(eng): Video: vp9 (Profile 1), yuv444p(tv, progressive), 1280x720, SAR 1:1 DAR 16:9, 17 fps, 17 tbr, 1k tbn, 1k tbc (default)
Metadata:
HANDLER_NAME : Apple Alias Data Handler
ENCODER : Lavc57.107.100 libvpx-vp9
DURATION : 00:04:08.294000000
Actual results:
Between 7 and 8 seconds into playback, the aspect ratio suddenly changes. The video becomes very tall and narrow.
Expected results:
The video should have kept the size it was when playback started.
Updated•6 years ago
|
Comment 1•6 years ago
|
||
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b8fba4d3bec32495e45e8e9c850d300d4bd9d842&tochange=6b87d74e97c91155e6023d154aab1786f3518101
Regressed by: Bug 1482059
Assignee | ||
Comment 3•6 years ago
|
||
I see a different ffprobe output for the orignal file (different resolution), but regardless, see a similar issue. ffprobe output:
Input #0, matroska,webm, from '1962_Tripoli_winter.webm':
Metadata:
encoder : Lavf56.40.101
Duration: 00:04:08.29, start: 0.000000, bitrate: 237 kb/s
Stream #0:0(eng): Video: vp9 (Profile 1), yuv444p(tv), 960x720, SAR 1:1 DAR 4:3, 17 fps, 17 tbr, 1k tbn, 1k tbc (default)
This is being caused by the width and height being flipped. We go from 960x720 to 720x960. Can also observe other oddities:
- Once the change has taken place, seeking back to early in the video doesn't reset it.
- If observe the change, pause for awhile, then play again, it will reset to the original dimensions for a bit. Assume this is due to info being flushed until we hit the next keyframe.
Looks like our change monitor code is picking up resolution changes on the key frames. ffprobe doesn't think the keyframes have different resolutions based on the -show_frames
output: it shows a steady resolution throughout. However when we grab info from the VPX stream upon hitting key frames we appear to be consistently getting 720 width and 960 height.
Assignee | ||
Comment 4•6 years ago
|
||
Digging further into the bitstream, here's my working:
- Use MKVToolNix to open the file and get info on it.
- Inspect the cluster at offset 144779, this cluster has the keyframe just before 7 seconds that causes the shift.
- See that the first block contains the keyframe in question and that the frame starts at offset 144802.
- Dump the hex for that frame and walk it by hand
The relevant hex we need to walk is a2 49 83 42 00 07 7e 05 9e
and we can pull it apart with the bitstream spec.
- First byte (a2): frame marker = 0b10, version = 1, show exisiting frame = 0, frame type = 0 (keyframe), show frame = 1, error resilient mode = 0
- Second (49), third (83), and fourth (42) bytes are expected frame sync codes.
- Fifth byte (00): We end up reading 7 bits here. 3 bytes for color space = 0 (not CS_RGB which is 7), 1 for color range, then 1 for each subsampling x, subsampling y, and a reserved bit.
- Since we didn't read all of the fifth byte, and since we're about to read our size, the last bit of the fifth byte becomes part of our size. Size is two 16 bit values, first width, then height.
- Taking the hex starting at the fifth byte
00 07 7e 05 9e
and shifting it about so we're reading from the last bit of the fifth byte we get03 bf 02 cf
(trimming to 4 bytes). - If we convert each set of 16 bits to decimal we get 959 and 719. Our width and height minus 1.
Assignee | ||
Comment 5•6 years ago
|
||
This may suggest a bug in libvpx-vp9 if it's doing the encoding. Edit: this does not seem to be the issue.
Assignee | ||
Comment 6•6 years ago
|
||
Clearing the NI as I think I've confused myself by gazing at numbers. Will re add as appropriate once I've gazed some more.
Assignee | ||
Comment 7•6 years ago
|
||
Hopefully found our culprit this time: aInfo.mDisplay = gfx::IntSize(br.ReadBits(16) + 1, br.ReadBits(16) + 1);
and similar do not ensure that the first arg is evaluated before the second. The order of evaluation is unspecified, and I think this is why we're seeing the values being switched.
Will get a patch ready.
Assignee | ||
Comment 8•6 years ago
|
||
Function arguments do not evaluate in order, as such, this ensures width is read
before height from VP9 streams.
Assignee | ||
Updated•6 years ago
|
Ah, sorry, I forgot I had cut the video down to get it under the size limits. The original video is 8mm film, no audio, from which a video digitization service made a gigantic motion JPEG file, about 2G, and which I then converted to VP9 in a webm container. The ffprobe data is from a 17M webm version, and the video file I put on the website is only 7M. Firefox has the same problem on the 17M version, the aspect changes at about 7 seconds.
This is the command I used to create the 7M version, on Ubuntu Linux 16.04:
ffmpeg -i Chapin\ B_005\ \ 53ft.720p.mov -c:v libvpx-vp9 -b:v 240k -filter:v "crop=960:720:160:0" -pass 1 -f webm -y /dev/null && ffmpeg -i Chapin\ B_005\ \ 53ft.720p.mov -c:v libvpx-vp9 -b:v 240k -filter:v "crop=960:720:160:0" -pass 2 1962_Tripoli_IA_winter.webm
The md5sum of the 7M video file: 602cc6bf2a6ee9277d923603bd0ab95f
ffmpeg version 2.8.15-0ubuntu0.16.04.1 Copyright (c) 2000-2018 the FFmpeg developers
built with gcc 5.4.0 (Ubuntu 5.4.0-6ubuntu1~16.04.10) 20160609
Assignee | ||
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
Assignee | ||
Comment 12•6 years ago
|
||
Planning to nominate for beta once we've landed and had a little time to bake.
As a reminder to myself, this doesn't seem to affect all vp9 videos, which I would have expected. Worth clarifying why that is.
Assignee | ||
Comment 13•6 years ago
|
||
Looking into what's different between the file from this bug and something like this wiki commons video:
With the wiki video, the change monitor immediately picks up on the wrong dimensions and never detects any changes. This has the consequence that the monitor never thinks it needs to change resolution. I.e. the change mointor see's the resolutions [flipped, flipped, flipped...], so it never kicks of the code that alters changes resolution and creates a new decoder.
With the video in this bug, the change monitor picks up the wrong dimensions, but then keeps thinking that something has changed in the stream info and forcing creation of a new decoder with the dimensions. I think this is because of this check. I believe we have a &&
instead of an ==
in mSubSampling_x && aOther.mSubSampling_x
, which means if either this
or aOther
have mSubSampling_x
set to false, we fail this check.
So that would mean we'd only see this bug in videos without x subsampling (or ones with actual resolution changes).
Will raise a follow up bug and fix that.
Assignee | ||
Comment 14•6 years ago
|
||
Bug 1538253 created in response to comment 13.
Comment 15•6 years ago
|
||
bugherder |
Assignee | ||
Comment 16•6 years ago
|
||
Comment on attachment 9052603 [details]
Bug 1537675 - Ensure reads in VPXDecoder.cpp happen in correct order. r?jya!
Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: Bug 1482059
- User impact if declined: VP9 videos that do not use x subsampling will be presented to users with incorrect dimensions.
- Is this code covered by automated tests?: No
- 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 change is small and relatively simple (though the footgun it fixes is nasty).
- String changes made/needed: None
Comment 17•6 years ago
|
||
Comment 18•6 years ago
|
||
Comment on attachment 9052603 [details]
Bug 1537675 - Ensure reads in VPXDecoder.cpp happen in correct order. r?jya!
Fix for a video regression in 65, looks safe and has been on Nightly for a week with no reported regression, uplift approved for 67 beta 7, thanks.
Comment 19•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Comment 20•6 years ago
|
||
I have reproduced this issue using Firefox 68.0a1 (2019.03.20) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Firefox 67.0b7, 68.0a1 latest nightly build, on Ubuntu 16.04 x64, Windows 10 x64 and on macOS 10.14.
Description
•