video aspect changes during playback

VERIFIED FIXED in Firefox 67

Status

()

defect
P2
normal
VERIFIED FIXED
2 months ago
2 months ago

People

(Reporter: bzipitidoo, Assigned: bryce)

Tracking

({regression})

65 Branch
mozilla68
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 wontfix, firefox67 verified, firefox68 verified)

Details

Attachments

(1 attachment)

Reporter

Description

2 months ago

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

2 months ago
Component: Untriaged → Audio/Video: Playback
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core

Comment 1

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

Comment 2

2 months ago

Taking a look.

Priority: -- → P2
Assignee

Comment 3

2 months 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

2 months 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 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)
Assignee

Comment 5

2 months 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

2 months 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.

Flags: needinfo?(jyavenard)
Assignee

Comment 7

2 months 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

2 months ago

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

Assignee

Updated

2 months ago
Assignee: nobody → bvandyk
Reporter

Comment 9

2 months 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

Comment 11

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

Comment 12

2 months 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

2 months 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

2 months ago

Bug 1538253 created in response to comment 13.

Comment 15

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee

Comment 16

2 months 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
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+

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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.