Closed Bug 1537675 Opened 5 years ago Closed 5 years ago

video aspect changes during playback

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
65 Branch
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- wontfix
firefox67 --- verified
firefox68 --- verified

People

(Reporter: bzipitidoo, Assigned: bryce)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1537675 - Ensure reads in VPXDecoder.cpp happen in correct order. r?jya!
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
Details | Review

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.

status-firefox66: --- → wontfix
status-firefox67: --- → fix-optional
status-firefox68: --- → affected
status-firefox-esr60: --- → unaffected
Component: Untriaged → Audio/Video: Playback
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b8fba4d3bec32495e45e8e9c850d300d4bd9d842&tochange=6b87d74e97c91155e6023d154aab1786f3518101

Regressed by: Bug 1482059

Blocks: 1482059
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true

Taking a look.

Priority: -- → P2

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.

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 get 03 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.
Flags: needinfo?(jyavenard)

This may suggest a bug in libvpx-vp9 if it's doing the encoding. Edit: this does not seem to be the issue.

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.

Flags: needinfo?(jyavenard)

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.

Function arguments do not evaluate in order, as such, this ensures width is read
before height from VP9 streams.

Assignee: nobody → bvandyk

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

Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a95055f3ccf9
Ensure reads in VPXDecoder.cpp happen in correct order. r=jya

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.

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.

https://hg.mozilla.org/mozilla-central/rev/a95055f3ccf9

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox68: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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
Attachment #9052603 - Flags: approval-mozilla-beta?

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.

Attachment #9052603 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+

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.

Status: RESOLVED → VERIFIED
status-firefox67: fixed → verified
status-firefox68: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: