1906527 - YUV422P10 video playback inconsistencies

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

https://store.steampowered.com/app/1203620/Enshrouded/?l=french

This video (the game trailer) doesn't render on Android and renders incorrectly on macOS. It is yuv422p10. I haven't tested other platform just yet.

Comment 1

[Paul Adenot (:padenot)]

https://paul.cx/public/test-videos/pattern-1280x720-libvpx-vp9-yuv422p10.webm can easily repro this.

Comment 2

[Markus Stange [:mstange]]

On macOS, the red and blue parts look vertically stretched by 2x compared to the grayscale which looks correct.

Comment 3

[Markus Stange [:mstange]]

MacIOSurface::CreateNV12OrP010Surface uses kCVPixelFormatType_420YpCbCr10BiPlanarFullRange but I don't see it using kCVPixelFormatType_422YpCbCr10BiPlanarFullRange anywhere.

Comment 4

[Brad Werth [:bradwerth]]

(In reply to Markus Stange [:mstange] from comment #3)

MacIOSurface::CreateNV12OrP010Surface uses kCVPixelFormatType_420YpCbCr10BiPlanarFullRange but I don't see it using kCVPixelFormatType_422YpCbCr10BiPlanarFullRange anywhere.

Yes, presumably the fix will be adding the appropriate tag to the surface. I'll try to get a patch ready soon.

Comment 5

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

The severity field is not set for this bug.
:jimm, could you have a look please?

For more information, please visit BugBot documentation.

Comment 6

[Brad Werth [:bradwerth]]

Allowing kCVPixelFormatType_422YpCbCr10BiPlanarVideoRange in NativeLayerCA::ShouldSpecializeVideo is sufficient to get the video roughly correct, but the colors still look blocky. I'll see if I can get the video looking correct, as well.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1906527 Part 3: Add YUV422P10 and NV16 formats.

This seems harmless, but might have reprecussions due to current calling
patterns. In general, we want to return accurate results, so updating
this code to note when something is 10-bit color depth or uses the
YUV422 format is the right thing to do. If it causes problems, we'll
address those problems as the Bugs that they are.

Comment 8

[Brad Werth [:bradwerth]]

Attachment: WIP: Bug 1906527 Part 2: Change MacIOSurface::CreateYUV422Surface to allow specifying color depth.

YUV422 format video can be used in the same situations where YUV 420 is
used, so the construction methods should be just as expressive.

Comment 9

[Brad Werth [:bradwerth]]

Attachment: Bug 1906527 Part 4: Force all 10-bit video and multi-planar video to use macOS specialized video layers.

This includes YUV422 10-bit video. This has a side effect of no longer
using specialized video layers for YUV420 8-bit video in windowed mode.
That's fine for code simplicity purposes. We are trending to use
specialized video layers for all video as soon as we can solve the
issues that arise from the buffered nature of the specialized video
layers.

Comment 10

[Brad Werth [:bradwerth]]

(In reply to Paul Adenot (:padenot) from comment #0)

https://store.steampowered.com/app/1203620/Enshrouded/?l=french

This video (the game trailer) doesn't render on Android and renders incorrectly on macOS. It is yuv422p10. I haven't tested other platform just yet.

I'm curious how we're meant to detect that this is YUV422 and not YUV420. The color display problems, I think, are due to us treating this as 420 encoded video. When I grab all the video metadata using MediaInfo, I get:

Video
ID                                       : 1
Format                                   : VP9
Codec ID                                 : V_VP9
Duration                                 : 1 min 54 s
Width                                    : 854 pixels
Height                                   : 480 pixels
Display aspect ratio                     : 16:9
Frame rate mode                          : Constant
Frame rate                               : 48.000 FPS
Color space                              : YUV
Time code of first frame                 : 01:00:00:00
Time code source                         : Matroska tags
Writing library                          : Lavc60.3.100 libvpx-vp9
Default                                  : Yes
Forced                                   : No
Color range                              : Limited
Color primaries                          : BT.709
Transfer characteristics                 : BT.709
Matrix coefficients                      : BT.709
VENDOR_ID                                : FFMP

Is there something there that signifies 422?

Comment 11

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #10)

Is there something there that signifies 422?

Well, we do end up in FFmpegVideoDecoder::CreateImage with a pix_fmt of AV_PIX_FMT_YUV422P10LE, so we're detecting it correctly, somehow. The problem is that we later drop that knowledge and assume a 420 encoding. I'll try to fix this with an addition to the patches.

Comment 12

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #11)

The problem is that we later drop that knowledge and assume a 420 encoding. I'll try to fix this with an addition to the patches.

We're ignoring mChromaSubsampling in MacIOSurfaceRecycleAllocator::Allocate and instead making the decision of 420 vs 422 based on the value of the layers.iosurfaceimage.use-nv12 pref. I'll see if I can figure out what that pref was trying to accomplish and how we can handle these cases more correctly.

Comment 13

[Brad Werth [:bradwerth]]

This ended up being a much heavier lift than I anticipated, but I have a patch that seems to work.

Comment 14

[Brad Werth [:bradwerth]]

Attachment: Bug 1906527 Part 1: Rename SurfaceFormat::YUV422 to YUY2, and rename YUV to YUV420.

This is just a renaming patch, motivated by a need for clarity before
addding more YUV formats. Comments and log messages are updated as well,
and one function name in MacIOSurface.

Comment 15

[Brad Werth [:bradwerth]]

Attachment: Bug 1906527 Part 2: Rename swgl TextureFormat::YUV422 to YUY2.

This is a renaming patch, changing the kinda-generic YUV422 to the
more-specific YUY2. YUY2 is the Y0 Cb0 Y1 Cr0 single-planar 4:2:2
format that matches what Apple is doing with GL_RGB_RAW_422_APPLE.

The motivation for the renaming is also to match the enums used by
SurfaceFormat. YUV422 is a chroma sampling technique, not a format.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80c598e96f5a
Part 1: Rename SurfaceFormat::YUV422 to YUY2, and rename YUV to YUV420. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/570468403c66
Part 2: Rename swgl TextureFormat::YUV422 to YUY2. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/4dfdfd420e91
Part 3: Add YUV422P10 and NV16 formats. r=gfx-reviewers,ahale
https://hg.mozilla.org/integration/autoland/rev/2cd3a78c64be
Part 4: Force all 10-bit video and multi-planar video to use macOS specialized video layers. r=mac-reviewers,spohl

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/80c598e96f5a
https://hg.mozilla.org/mozilla-central/rev/570468403c66
https://hg.mozilla.org/mozilla-central/rev/4dfdfd420e91
https://hg.mozilla.org/mozilla-central/rev/2cd3a78c64be

Comment 18

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

The patch landed in nightly and beta is affected.
:bradwerth, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox130 to wontfix.

For more information, please visit BugBot documentation.

Comment 19

[Brad Werth [:bradwerth]]

I think this will benefit from a full Nightly cycle to check for any breakage. Let's not uplift.