Add support for YUV422 IOSurface

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments)

Assignee

Description

3 years ago
The mac's media decoder currently use NV12 IOSurface. Which was an improvement over the original BGA32 use.

Playing the video from bug 1230641, timing of the decoding time indicates that the decoder takes a significant time (median=29ms, average=25ms) , which makes it impossible to play this video properly (60fps)

Chromium uses the YUV422 format (kCVPixelFormatType_422YpCbCr8).

Timing the decoding time shows that it gives much better performance: 5ms median with a 17ms average.

We should support the kCVPixelFormatType IOSurface format (see https://www.opengl.org/registry/specs/APPLE/ycbcr_422.txt, format is '2vuy' FOURCC code.
Given the performance improvement noted above this sounds really important.

Jya are you needing help from GFX to get this implement or does the media have this?
Flags: needinfo?(jyavenard)
Whiteboard: [gfx-noted]
Assignee

Comment 2

3 years ago
IT would be better if the gfx team did it, I had a shot with Matt but it appeared as if YUV wasn't converted to RGB (Apple OpenGL is supposed to allow mapping kCVPixelFormatType_422YpCbCr8 straight to an opengl texture and handle it as if it was RGB).

On an intel laptop, it appears that when using NV12 we have the need to for two CVPixelBufferPool : one for the decoder and one for the client : indicating that the VT decoder decode then converts / copy.

And once again, when compositing we play with the video. 

With 422YpCbCr8 all those intermediate steps are gone.

It doesn't fully explain on why there's such a massive difference, and I'm suspecting it's to do with the CVPixelBufferPool waiting for the compositor to release an IOSurface before recycling a new one.

I will track those issues separately in bug 1230641
Flags: needinfo?(jyavenard)
Assignee

Updated

3 years ago
Assignee: nobody → jyavenard
Comment on attachment 8722451 [details]
MozReview Request: Bug 1248323: P1. Add support for YUV422 IOSurface. r=nical

https://reviewboard.mozilla.org/r/36045/#review32667
Attachment #8722451 - Flags: review?(nical.bugzilla) → review+
Attachment #8722452 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8722452 [details]
MozReview Request: Bug 1248323: P2. Add readback code for converting YUV422 MacIOSurfaces into RGB. r=nical

https://reviewboard.mozilla.org/r/36047/#review32671

::: gfx/layers/MacIOSurfaceHelpers.cpp:86
(Diff revision 1)
> +    size_t cbCrWidth = cbCrStride;

nit: I would rather not declare cbCrWidth and use cbCrStride explicitly since we use it as a stride rather than as the actual width of the image.
Assignee

Comment 7

3 years ago
Will do..

The RGB conversion isn't so great. In the reftest Matt added in the bug adding NV12 support, it's a bright red video.
Safari and Chrome generate a perfect red when copying to a canvas, but we do not. I see red values between 253 and 255.

It's a pity we can't use OpenGL to perform this conversion so we get the same RGB conversion as the way it's rendered on screen.
I tried playing with CGIOSurfaceCreateContext but it always return a null CGImage out (was hoping to use a SourceSurfaceCG instead of software one)

Seeing that the CGcontext isn't available when this function is called, could we use an offscreen GLcontext and perform the conversion via OpenGL?
I think the naming of GetFormat/GetUploadFormat should be reversed here, and GetFormat should possibly be changed to 'GetReadFormat' or similar.

It makes sense to me that the 'upload' format in this case would be SurfaceFormat::YUV422, since that's the type of data that was uploaded to the texture. It's also what we'll get if we do readback and 'download' the data again.

The format that the GL shaders will see is SurfaceFormat::R8G8B8X8, so I think calling that the 'read' format (or 'shader' format?) is clearer.

Getting rid of the plain GetFormat and only having GetXXXFormat should help make things clearer too.
Assignee

Comment 9

3 years ago
(In reply to Nicolas Silva [:nical] from comment #6)
> Comment on attachment 8722452 [details]
> MozReview Request: Bug 1248323: P2. Add readback code for converting YUV422
> MacIOSurfaces into RGB. r?nical
> 
> https://reviewboard.mozilla.org/r/36047/#review32671
> 
> ::: gfx/layers/MacIOSurfaceHelpers.cpp:86
> (Diff revision 1)
> > +    size_t cbCrWidth = cbCrStride;
> 
> nit: I would rather not declare cbCrWidth and use cbCrStride explicitly
> since we use it as a stride rather than as the actual width of the image.

upon further look, we do differentiate between width and stride. It just happens that width == stride

However, just realised that the YUV->RGB converter is using SSE, so having a stride that isn't a multiple of 16 will seriously slow things down. on mac a malloc is always aligned on 16 bytes.

So we want a stride that is going to be a multiple of 16. That way all start of rows can be accessed using aligned movdqa
Assignee

Comment 12

3 years ago
Comment on attachment 8722451 [details]
MozReview Request: Bug 1248323: P1. Add support for YUV422 IOSurface. r=nical

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36045/diff/1-2/
Attachment #8722451 - Attachment description: MozReview Request: Bug 1248323: P1. Add support for YUV422 IOSurface. r?nical → MozReview Request: Bug 1248323: P1. Add support for YUV422 IOSurface. r=nical
Assignee

Comment 13

3 years ago
Comment on attachment 8722452 [details]
MozReview Request: Bug 1248323: P2. Add readback code for converting YUV422 MacIOSurfaces into RGB. r=nical

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36047/diff/1-2/
Attachment #8722452 - Attachment description: MozReview Request: Bug 1248323: P2. Add readback code for converting YUV422 MacIOSurfaces into RGB. r?nical → MozReview Request: Bug 1248323: P2. Add readback code for converting YUV422 MacIOSurfaces into RGB. r=nical
Assignee

Comment 14

3 years ago
reftest is disabled on windows due to localized try readback errors that can't be reproduced.

Review commit: https://reviewboard.mozilla.org/r/36509/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36509/
Attachment #8723380 - Flags: review?(jyavenard)
Assignee

Updated

3 years ago
Attachment #8723380 - Flags: review?(jyavenard) → review+
Assignee

Comment 15

3 years ago
Comment on attachment 8723380 [details]
MozReview Request: Bug 1248323: P3. Add test for NV12/YUV422 readback. r=jya

https://reviewboard.mozilla.org/r/36509/#review33069
Assignee

Updated

3 years ago
Blocks: 1230641
You need to log in before you can comment on or make changes to this bug.