1237809 - Incorrect aspect ratio for 16:9 videos

Status: RESOLVED FIXED

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

Description

[rctgamer3]

Loaded an MPEG-4 video in Firefox. Instead of the video being shown in 16:9, it's being output with aspect ratio 4:3. It displays just fine on both OS X and Linux (Arch Linux).

Due to copyright and file size, I cannot share this video. However, I can share MediaInfo output below. Let me know if you need any further information to help determine the root cause.

Video: MPEG4 Video (H264) 1436x1076 (1436:807) 29.97fps 5411kbps [V: English [eng] (h264 high L4.0, yuv420p, 1436x1076, 5411 kb/s)]

General
Format                         : MPEG-4
Format profile                 : Base Media / Version 2
Codec ID                       : mp42
Overall bit rate mode          : Variable
Overall bit rate               : 5 542 Kbps

Video
ID                             : 2
Format                         : AVC
Format/Info                    : Advanced Video Codec
Format profile                 : High@L4
Format settings, CABAC         : Yes
Format settings, ReFrames      : 2 frames
Codec ID                       : avc1
Codec ID/Info                  : Advanced Video Coding
Bit rate mode                  : Variable
Bit rate                       : 5 412 Kbps
Maximum bit rate               : 768 Kbps
Width                          : 1 436 pixels
Height                         : 1 076 pixels
Display aspect ratio           : 16:9

Comment 1

[Jean-Yves Avenard [:jya]]

Please retry with beta.. This should be fixed.

Comment 2

[Jean-Yves Avenard [:jya]]

Re-opened if it's not fixed.

Comment 3

[rctgamer3]

I'm already running Aurora/DevEdition 45.0a2 (2016-01-06). Tested with the latest m-c/Nightly build ( 46.0a1 (2016-01-07)), same incorrect result.

Comment 4

[Jean-Yves Avenard [:jya]]

Aspect ratio is calculated as per the SPS found in the file, not what is found and described in the container.

So if the SPS is wrong, aspect ratio won't be good.

As such, without a sample, there's no much we can do...

If you could provide a private link, I can take it further...

Comment 5

[Jean-Yves Avenard [:jya]]

Just the metadata would be sufficient (moov atom). If the moov is located at the beginning (progressive mp4) then giving the first 1MB will be more than enough for debugging purposes

Comment 6

[Jean-Yves Avenard [:jya]]

Weird..

The video is 1436x1076, and when calling VideoData::Create or VideoData::CreateFromImage we do pass gfx::IntRect(0, 0, 1436, 1076) and a VideoInfo.mDisplay containing (1914, 1076) as it should be.

On mac when using IOSurface, it displays as 1436x1076, if I force to use YUVBuffers instead, then it displays fine.

Same on windows, but that's for another reason. The dimensions passed to the constructor are wrong

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: [h264] P1. Ensure correct video dimensions are passed to the decoder.

Dimensions are extracted from the SPS found in metadata if present.

Images are still displayed incorrectly on Windows despite the correct dimensions being provided.

Comment 8

[Jean-Yves Avenard [:jya]]

Matt, could you have a look and let me know how the image should be created so that it is displayed with the proper aspect ratio?

this is where it's created: https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/wmf/WMFVideoMFTManager.cpp#489

using software decoder.

going to try the HW decoder shortly

Comment 9

[rctgamer3]

Ran mozregression, ended up with

LOG: MainThread Bisector INFO Narrowed inbound regression window from [39af5c53, b4401440] (3 revisions) to [86d1b3a8, b4401440] (2 revisions) (~1 steps left)
LOG: MainThread main INFO Oh noes, no (more) inbound revisions :(
LOG: MainThread Bisector INFO Last good revision: 86d1b3a8aecb978a7e656e8c8018e227333d9ae5
LOG: MainThread Bisector INFO First bad revision: b4401440ad833c2b6cea09e3ef589fba28cc2f0a
LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=86d1b3a8aecb978a7e656e8c8018e227333d9ae5&tochange=b4401440ad833c2b6cea09e3ef589fba28cc2f0a

Comment 10

[Jean-Yves Avenard [:jya]]

It is this commit that caused the regression:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4401440ad83

I can confirm that with the patch attached, aspect ratio is fixed when using HW decoding and D3D Image.

But displays incorrectly when using the windows SW decoder with IMFYCbCrImage.

Comment 11

[Jean-Yves Avenard [:jya]]

rctgamer3 was kind enough to test on 43 with media.hardware-video-decoding.enabled set to false, and the aspect ratio is broken there too (likely always has been broken only realising now)

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8705669 [details] [diff] [review]
[h264] P1. Ensure correct video dimensions are passed to the decoder.

Approval Request Comment
[Feature/regressing bug #]: 1195094
[User impact if declined]: Wrong aspect ratio 
[Describe test coverage new/current, TreeHerder]: manual test
[Risks and why]: Very low, partially redoing what bug 1195094 changed.
[String/UUID change made/needed]: Low

Note that aspect ratio is still wrong under some circumstances, but this hasn't changed over 43.

Comment 13

[Jean-Yves Avenard [:jya]]

[Tracking Requested - why for this release]: regression

Comment 14

[Jean-Yves Avenard [:jya]]

Matt, this isn't resolved for IMFYCbCrImage nor IOSurface, they are displayed incorrectly ignoring the aspect ratio provided.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Jean-Yves, is that normal that the patch didn't land in m-c/46?

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1f479d8623

Comment 17

[Jean-Yves Avenard [:jya]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #15)
> Jean-Yves, is that normal that the patch didn't land in m-c/46?

was waiting on the review...

the fix is small, will be very safe to uplift (in fact it's almost going back to the original code before the regression was introduced)

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Expose picture rect on IMFYCbCrImage

Hard to do much without a testcase, but it looks like we're ignoring the picture rect.

This patch should fix that, does it work?

We'd need to do something similar for MacIOSurfaceImage too if so.

Comment 19

[Jean-Yves Avenard [:jya]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> Created attachment 8706675 [details] [diff] [review]
> Expose picture rect on IMFYCbCrImage
> 
> Hard to do much without a testcase, but it looks like we're ignoring the
> picture rect.
> 
> This patch should fix that, does it work?

 1:43.91 c:\mozilla-central\gfx\layers\IMFYCbCrImage.h(27): error C2039: 'mGetPictureRect': is not a member of 'mozilla::layers::PlanarYCbCrData'

I see no mPictureRect there either. The only related member is mSize

There also no gfx::IntRect IMFYCbCrImage::GetPictureRect() member either

Comment 20

[Jean-Yves Avenard [:jya]]

this is still the wrong aspect ratio.
GetPictureRect() returns {0,0,1436,1076} this is the size of the picture, not the size it should be displayed at (1914,1076)

Comment 21

[Matt Woodrow (:mattwoodrow)]

(In reply to Jean-Yves Avenard [:jya] from comment #20)
> this is still the wrong aspect ratio.
> GetPictureRect() returns {0,0,1436,1076} this is the size of the picture,
> not the size it should be displayed at (1914,1076)

It's not really clear what the right behaviour is then.

The webpage specifies the size of the video element, and the video specifies of the picture rect to sample from. The combination of those two sizes constrains the aspect ratio, so we don't easily get to factor an external one in.

Are we supposed to leave blank space within the area specified by the video element? At the bottom, or evenly distributed between bottom and top?

We have a ScaleMode [1] type which could potentially request this sort of behaviour, but it isn't implemented anywhere.

How does this work on any backend?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayersTypes.h#159

Comment 22

[Jean-Yves Avenard [:jya]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> (In reply to Jean-Yves Avenard [:jya] from comment #20)
> > this is still the wrong aspect ratio.
> > GetPictureRect() returns {0,0,1436,1076} this is the size of the picture,
> > not the size it should be displayed at (1914,1076)
> 
> It's not really clear what the right behaviour is then.
> 
> The webpage specifies the size of the video element, and the video specifies
> of the picture rect to sample from. The combination of those two sizes
> constrains the aspect ratio, so we don't easily get to factor an external
> one in.

That's not how aspect ratio works.

All is defined in the picture returned by the decoder, the webpage doesn't define anything.

So this image is 1436x1076 pixels; but an aspect ratio is defined: e.g. the pixels aren't square: so it's to be displayed as 1914x1076 (that would be the visual aspect if the pixels had been square), or if you want to be anal, displayed with 1436x1076 pixels with each pixel being 1.77x1

The D3D/DXVA image is displayed as it should, and so is the plain YUV buffer.

> 
> Are we supposed to leave blank space within the area specified by the video
> element? At the bottom, or evenly distributed between bottom and top?

neither due to the above.

> 
> We have a ScaleMode [1] type which could potentially request this sort of
> behaviour, but it isn't implemented anywhere.
> 
> How does this work on any backend?

I don't know.. but it does (except IMFYUV and IOSurface) :)
On mac you can easily test by changing https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/apple/AppleVDADecoder.cpp#53 mUseSoftwareImage from false to true

you'll find that the image is displayed correctly with mUseSoftwareImage set to true (which uses a VideoData::YCbCrBuffer as image)

Comment 23

[Jean-Yves Avenard [:jya]]

Attachment: aspect_test.mp4

Took a while to craft that one as the file ffmpeg generated always presented properly.

It turns out, the original video is incorrect really, in that the display size set in the container is showing as 1436x1076.
Though, it theory it shouldn't make a difference as we retrieve the dimensions from the SPS NAL.

Comment 24

[Jean-Yves Avenard [:jya]]

The file above is incorrectly encoded in that the video track header width/height are invalid.

From ISO 14496-12:
"width and height specify the track's visual presentation size as fixed-point 16.16 values. These need not be the same as the pixel dimensions of the images, which is documented in the sample description(s); all images in the sequence are scaled to this size, before any overall transformation of the track represented by the matrix. The pixel dimensions of the images are the default values."

so it should be 1914x1076 here, not 1436x0176. To follow, the same file with the right data set in the moov atom.

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: aspect_test_ok.mp4

Comment 26

[Jean-Yves Avenard [:jya]]

very weird, the VideoConfig passed to the decoder is identical under both instance. so it should make zero difference what the container contains

Comment 27

[Jean-Yves Avenard [:jya]]

I just realised what was happening, I was concentrating on the decoder side of things.

But here, the difference is the reader and the MediaDecoderStateMachine when ReadMetadata is called would get a different resolution.

Under all circumstances, the MediaDataDecoder does return a 1436x1076 image with a display size of 1914x1076.

In the mp4 showing the bad AR, ReadMetadata would have returned 1436x1076 and in the one where it works: 1914x1076.

I can read the SPS if known directly in the MP4Demuxer so we always return a size consistent with what the decoder will be getting that will get around the problem.

But that still shows that regardless of the size returned in ReadMetadata, the decoded image isn't displayed properly (and worse it's displayed differently depending on the system); it's as if the display size set in the VideoData was ignored.

Assuming that the container contained bad data and was marked with a display size of 1436x1076, 
attempting to display a 1436x1076 *image* with a 16:10 AR should be displayed as a 1436 pixels wide image, but with black bars on top and bottom to preserve the 16:10 aspect ratio, making it effectively a 1436x807 image.

I guess this could be taken in a follow up bug, as for something like AVC3 we still wouldn't have the proper resolution until we decoded the first frame, so the work around described above wouldn't work

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: P2. Detect change of display size.

This is a similar problem as HE-AAC where the container information diverge with what it actually contains.

Comment 29

[Jean-Yves Avenard [:jya]]

Attachment: P3. Ensure element dimensions are up to date.

The VideoContainer contains more up to date video size information than the MediaInfo received by MediaDecoder::MetadataLoaded

We have a racy situation in regards to what the actual dimensions of the video are.
It goes on to something like this:

MDSM taskqueue:
-MDSM read the metadata and notify MediaDecoder::MetadataLoaded
-MDSM request decoding the first frame, receive a frame and update the video sink with the dimensions that could be different from what the metadata contained
-MDSM notify that the MediaDecoder first frame has been decoded.

Main Thread:
-MediaDecoder::MetadataLoaded gets called with a MediaInfo (from the original metadata).
-MediaDecoder::Invalidate() is called, which reads the dimensions of the container (those are the ones of the first frame). This set the element dimensions to the right ones as it has VideoFrameContainer::mIntrinsicSizeChanged is true.
-mOwner->MetadataLoaded() is called with the old MediaInfo, which set the element dimensions back to the original one.

-MediaDecoder::FirstFrameLoaded is called.
-MediaDecoder::Invalidate() is called, but this time VideoFrameContainer::mIntrinsicSizeChanged is false as it has already processed the change above.

So overall, the element is set to the dimensions set in the metadata and not of the actual content: we should have had a second resize event but it is never fired.

This simple patch simply ensure that Invalid() is called after MediaDecoderOwner::MetadataLoaded so the most up to date dimensions are used.

We could probably handle this differently, in particular using a Mirror on the MediaInfo, so we always process it in the proper order.

Test page: http://people.mozilla.org/~jyavenard/tests/aspect.html

this one liner ensure that the video is always displayed in the proper aspect ratio.

Comment 30

[Jean-Yves Avenard [:jya]]

(In reply to Jean-Yves Avenard [:jya] from comment #29)

> We could probably handle this differently, in particular using a Mirror on
> the MediaInfo, so we always process it in the proper order.

I'll handle this in a follow up bug. I wanted something dead and simple for any chance of uplifting there are some left

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e1f479d8623

Comment 32

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8706847 [details] [diff] [review]
P3. Ensure element dimensions are up to date.

Review of attachment 8706847 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoder.cpp
@@ +875,5 @@
>    if (aEventVisibility != MediaDecoderEventVisibility::Suppressed) {
>      mFiredMetadataLoaded = true;
>      mOwner->MetadataLoaded(mInfo, nsAutoPtr<const MetadataTags>(aTags.forget()));
>    }
> +  Invalidate();

The order is tricky. It might be worth it to add some comments.

Comment 33

[JW Wang [:jwwang] [:jw_wang]]

https://hg.mozilla.org/mozilla-central/file/5acc2a44834ce0614f98466475e674517daf0041/dom/media/VideoFrameContainer.cpp#l147

Btw, we might want to set mIntrinsicSizeChanged to false only when mElement->UpdateMediaSize() really changes mMediaInfo.mVideo.mDisplay so 'resize' event won't be missed.

Comment 34

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #33)
> https://hg.mozilla.org/mozilla-central/file/
> 5acc2a44834ce0614f98466475e674517daf0041/dom/media/VideoFrameContainer.
> cpp#l147
> 
> Btw, we might want to set mIntrinsicSizeChanged to false only when
> mElement->UpdateMediaSize() really changes mMediaInfo.mVideo.mDisplay so
> 'resize' event won't be missed.

I don't see how that would have helped here.
mElement->UpdateMediaSize() did change to the final dimensions. It's MediatadaLoaded that overwrote them

Comment 35

[JW Wang [:jwwang] [:jw_wang]]

like

bool HTMLMediaElement::UpdateMediaSize(const nsIntSize& aSize)
{
  if (IsVideo() && mReadyState != HAVE_NOTHING &&
      mMediaInfo.mVideo.mDisplay != aSize) {
    DispatchAsyncEvent(NS_LITERAL_STRING("resize"));
    mMediaInfo.mVideo.mDisplay = aSize;
    mWatchManager.ManualNotify(&HTMLMediaElement::UpdateReadyStateInternal);
    return true;
  }
  return false;
}

to return true only when mMediaInfo.mVideo.mDisplay is really updated.

Comment 36

[Jean-Yves Avenard [:jya]]

Oh, I perfectly understand what you meant (returning a boolean), however, I don't see how not setting mIntrinsicSizeChanged when UpdateMediaSize return false, would help to have resize fired.

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jean-Yves, bug 1195094 shows Fx44 status as "fixed". Why does this need to be uplifted to Beta44? Or the bug # a typo? Just wondering.

Comment 38

[Jean-Yves Avenard [:jya]]

(In reply to Ritu Kothari (:ritu) from comment #37)
> Jean-Yves, bug 1195094 shows Fx44 status as "fixed". Why does this need to
> be uplifted to Beta44? Or the bug # a typo? Just wondering.

because the form ask to enter the regression bug number. So that's what I entered: bug 1195094 is what introduced the regression.
P1 is the fix of that regression.
P3 is the "proper" fix. I would uplift (if possible) both P1 and P3. I feel that they are very safe.

Comment 39

[Jean-Yves Avenard [:jya]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd4a6fbc817

amazing something changing MediaDecoder not introducing regression :D

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a72fd5871157
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0c58e53473

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a72fd5871157
https://hg.mozilla.org/mozilla-central/rev/ed0c58e53473

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8705669 [details] [diff] [review]
[h264] P1. Ensure correct video dimensions are passed to the decoder.

Taking in aurora as it is a regression but I am pretty sure that we will reject it in beta as it is very late in the cycle.

Comment 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8705669 [details] [diff] [review]
[h264] P1. Ensure correct video dimensions are passed to the decoder.

While this is a valid issue I do not think a significant portion of our end-users are affected by this. I am denying this based on the more stringent Beta44 uplift criteria that allows only for fixing critical (recent) regressions, sec and stability issues. To me this fix, does not meet that bar. Sorry!

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Do all three patches from here need to be uplifted to aurora?

Comment 45

[Jean-Yves Avenard [:jya]]

Yes.

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b64485dcf3d7
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b5e456f2848e
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5fc729fa21d1