Closed Bug 1248323 Opened 5 years ago Closed 5 years ago

Add support for YUV422 IOSurface

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

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]
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: 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.
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.
(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
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
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
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)
Attachment #8723380 - Flags: review?(jyavenard) → review+
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
Blocks: 1230641
You need to log in before you can comment on or make changes to this bug.