MacIOSurfaceImage: Add 3 planes YUV (YV12/YUV420) and 2 planes (NV12) support

VERIFIED FIXED in Firefox 41

Status

()

defect
P2
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: jya, Assigned: mattwoodrow)

Tracking

Trunk
mozilla43
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 wontfix, firefox41 fixed, firefox42 fixed, firefox43 verified)

Details

Attachments

(9 attachments, 2 obsolete attachments)

20.22 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
4.68 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
11.29 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
5.31 KB, patch
nical
: review+
Details | Diff | Splinter Review
3.05 KB, patch
jya
: review+
Details | Diff | Splinter Review
4.50 KB, patch
nical
: review+
Details | Diff | Splinter Review
11.48 KB, patch
nical
: review+
Details | Diff | Splinter Review
11.55 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
12.46 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
The default image coming of the VideoToolbox hardware decoder is a NV12 image (this is at least what's occurring with an Intel HD5000 and ATI 6970M GPUs).

The compositor currently only supports RGBA IOSurface image.

It is possible to configure the VideoToolbox decoder to return an RGBA IOSurface, however the conversion appears to be performed in CPU as the CPU usage during playback is much greater than when using NV12.

The compositor/MacIOSurfaceImage should be able to handle 3 planes and 2 planes YUV images.
(Reporter)

Updated

5 years ago
Blocks: 1059066
(Reporter)

Comment 1

5 years ago
Posted patch output YUV, display as-is (obsolete) — Splinter Review
This patch make the VideoToolbox decoder output a YUV frame, and will display it as-is, (giving a nice negative film effect)
(Reporter)

Updated

5 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
Assignee: jyavenard → nobody
(Reporter)

Updated

5 years ago
Status: ASSIGNED → NEW
Cool!

Roughly what we need to do is:

* Make CGLTexImageIOSurface2D take a plane parameter and then use IOSurfaceGetPixelFormat to compute GL formats for the given plane.
* Maybe make MacIOSurface::GetWidth use IOSurfaceGetWidthOfPlane and add a plane parameter defaulting to 0. Same for all the other functions that have OfPlane equivalents.


For 3-plane YUV:

* Make MacIOSurfaceTextureHostOGL construct 3 MacIOSurfaceTextureSourceOGL's as a linked list (see BufferTextureHost::Upload for an example) with one corresponding to each of the 3 planes.

* Make MacIOSurfaceTextureHostOGL::GetFormat return SurfaceFormat::YUV

For 2 plane YUV:

* Rename SurfaceFormat::YUV to something more specific that describes 3-plane
* Add a new enum value to SurfaceFormat for 2-plane/NV12
* Make MacIOSurfaceTextureHostOGL construct 2 TextureSources and return the new enum value for GetFormat
* Add a new Effect subclass for NV12 and create it in CreateTexturedEffect when we see the new enum value.
* Handle the new Effect type in CompositorOGL::DrawQuad
* Write the new shader for this effect in ShaderProgramOGL (I assume it's basically the same as 3-plane, except we've only got 2 textures bound and we're reading .rb from the second instead of .r from the second and third).
(Reporter)

Updated

5 years ago
Blocks: 1077277
(Reporter)

Updated

4 years ago
Blocks: 1133633
Posted patch mac-io-yuv (obsolete) — Splinter Review
Attachment #8482661 - Attachment is obsolete: true

Updated

4 years ago
Priority: -- → P2
Attachment #8565260 - Attachment is obsolete: true
Assignee: nobody → matt.woodrow
Attachment #8569419 - Flags: review?(bgirard)
Attachment #8569419 - Attachment description: Bug 1061525 - Part 1: Add support for planar MacIOSurfaces → Part 1: Add support for planar MacIOSurfaces
Attachment #8569423 - Flags: review?(bgirard) → review?(nical.bugzilla)
Comment on attachment 8569419 [details] [diff] [review]
Part 1: Add support for planar MacIOSurfaces

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

::: gfx/layers/LayersLogging.cpp
@@ +309,5 @@
>    case SurfaceFormat::R8G8B8X8:  aStream << "SurfaceFormat::R8G8B8X8"; break;
>    case SurfaceFormat::R5G6B5:    aStream << "SurfaceFormat::R5G6B5"; break;
>    case SurfaceFormat::A8:        aStream << "SurfaceFormat::A8"; break;
>    case SurfaceFormat::YUV:       aStream << "SurfaceFormat::YUV"; break;
> +  case SurfaceFormat::NV12:       aStream << "SurfaceFormat::NV12"; break;

optional nit: extra spacing
Attachment #8569419 - Flags: review?(bgirard) → review+
Comment on attachment 8569422 [details] [diff] [review]
Part 3: Add shaders for NV12 to OGLShaderProgram

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

This code isn't my strong suite. Maybe get another pair of eye on it if you want. But it looks reasonable.

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +243,5 @@
>    const char *texture2D = "texture2D";
>  
>    if (aConfig.mFeatures & ENABLE_TEXTURE_RECT) {
>      fs << "uniform vec2 uTexCoordMultiplier;" << endl;
> +    fs << "uniform vec2 uCbCrTexCoordMultiplier;" << endl;

should this be under 'if (aConfig.mFeatures & ENABLE_TEXTURE_NV12) {'?

@@ +276,5 @@
>  
>    if (!(aConfig.mFeatures & ENABLE_RENDER_COLOR)) {
>      fs << "vec4 sample(vec2 coord) {" << endl;
>      fs << "  vec4 color;" << endl;
> +    if (aConfig.mFeatures & ENABLE_TEXTURE_YCBCR ||

IMO this would be simpler as:
if (a) {
} else if (b) {
} else if (c) {
} else if (d) {
}

@@ +326,5 @@
>        fs << "    color = vec4(onBlack, alphas.a);" << endl;
>        fs << "  else" << endl;
>        fs << "    color = alphas;" << endl;
>      } else {
> +      if (aConfig.mFeatures & ENABLE_TEXTURE_RECT) {

I don't understand why this was moved around. Please make sure this was intentional.
Attachment #8569422 - Flags: review?(bgirard) → review+
(Reporter)

Comment 13

4 years ago
Comment on attachment 8569424 [details] [diff] [review]
Part 5: Make the OSX video decoders output NV12 format MacIOSurfaces

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

r=me with comments addressed.

::: dom/media/fmp4/apple/AppleVDADecoder.cpp
@@ +473,5 @@
>      CFNumberCreate(kCFAllocatorDefault,
>                     kCFNumberSInt32Type,
>                     &PixelFormatTypeValue);
>  
> +  const void* outputKeys[] = { kCVPixelBufferIOSurfacePropertiesKey ,

nit: space before ,

@@ +478,4 @@
>                                 kCVPixelBufferPixelFormatTypeKey,
>                                 kCVPixelBufferOpenGLCompatibilityKey };
> +  const void* outputValues[] = { IOSurfaceProperties ,
> +                                 PixelFormatTypeNumber ,

same here, space before ,

::: dom/media/fmp4/apple/AppleVTDecoder.cpp
@@ +352,5 @@
>    const void* extensionValues[] =
>      { kCVImageBufferChromaLocation_Left,
>        kCVImageBufferChromaLocation_Left,
>        atoms,
> +      kCFBooleanFalse };

We don't need this.
Remove from extensionKeys AppleCMLinker::skPropFullRangeVideo and remove from extensionValues
the last element
Attachment #8569424 - Flags: review?(jyavenard) → review+
Comment on attachment 8569421 [details] [diff] [review]
Part 2: Make MacIOSurfaceTextureHostOGL understand planar MacIOSurfaces

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

Looks good to me.
Attachment #8569421 - Flags: review?(jmuizelaar) → review+
Attachment #8569423 - Flags: review?(nical.bugzilla) → review+
Attachment #8569427 - Flags: review?(nical.bugzilla) → review+
Attachment #8569430 - Flags: review?(nical.bugzilla) → review+
Can we land this?
Flags: needinfo?(matt.woodrow)
(Reporter)

Comment 16

4 years ago
What's the status with this ?

Any progress? Would be great to have...
(Reporter)

Comment 17

4 years ago
When VT is configured to output YUV420 and with the patches provided here (once rebased). I can no longer reproduce the issue.

Matt, could we hurry that one ?

In the mean time I will disable the use of IOSurface and RGBA32 texture
Blocks: 1168552, 1187103
It looks like we have multiple threads waiting on VTDecompressionSessionWaitForAsynchronousFrames (2 in m-gl, 6 in m-2) while the main thread is trying to shut down.

I have no idea how asking the decoder for NV12 would cause that, jya?
Flags: needinfo?(matt.woodrow) → needinfo?(jyavenard)
(Reporter)

Comment 21

4 years ago
Neither do I :(

And reading the little doc available for that API it shouldn't be blocking long either..

Can you reproduce the problem locally?
Flags: needinfo?(jyavenard)
I can't, but it's 10.6 only, so that's probably not surprising.

Should we just use NV12 for 10.7/8 and up?
(Reporter)

Comment 23

4 years ago
What about using NV12 for 10.7 and later, and use software backed image for the rest?

We know that using RGBA can cause kernel panics, even in newer OS, can only be worse in 10.6
That sounds good to me, can you write the patch for that?

Note that we only have 10.6 and 10.10 tests, so we don't know for sure that it's fixed in 10.7 (or 10.8/9 either).
(Reporter)

Comment 25

4 years ago
Ill do this patch today hopefully.

lots on my plate, but I'm really keen on getting rid of those changes.

In 10.6 none of those framework actually officially exist, not even in 10.7. They were introduced in 10.8

So that it works it pure luck already. However, we use VDA by default whenever available if < 10.9 ; so it should be fine. (i hope)
(Reporter)

Comment 26

4 years ago
Using NV12 IOSurface cause some hangs.
Attachment #8644182 - Flags: review?(matt.woodrow)
Comment on attachment 8644182 [details] [diff] [review]
Part 7: Use software backed NV12 images on 10.6

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

::: dom/media/platforms/apple/AppleVDADecoder.cpp
@@ +270,5 @@
> +  if (mUseSoftwareImages) {
> +    size_t width = CVPixelBufferGetWidth(aImage);
> +    size_t height = CVPixelBufferGetHeight(aImage);
> +#ifdef DEBUG
> +    size_t planes = CVPixelBufferGetPlaneCount(aImage);

You can just do 'DebugOnly<size_t> planes' and avoid needing #ifdef DEBUG.
Attachment #8644182 - Flags: review?(matt.woodrow) → review+
Looks like that didn't fix the problem :(
(Reporter)

Comment 30

4 years ago
Do you have a link to the try hang?

It's surprising that it doesn't fix it, as this is pretty much what we used to do early on, before going to use MacIOSurface
(Reporter)

Comment 31

4 years ago
If you're referring to my try run, I included other changes from inbound known to fail at the time.

I'll re-run one.

It works here in my 10.6 VM
(Reporter)

Comment 32

4 years ago
On 2nd thought, those 10.6 machines are 2010 mac mini with a nvidia card; it would be using VDA.
So it's unlikely to be VTDecompressionSessionWaitForAsynchronousFrames that is blocking (and that it works on my VM confirms it as VM would be using VT).

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09fd9cf838a0
(Reporter)

Comment 33

4 years ago
So that didn't work.

I looked up back to when we used software images at all time ; and the configuration type was never created, instead it was set to "NULL"

Let see if that works any better:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35d4da026f0a
(Reporter)

Comment 34

4 years ago
Ok.

I made it so NV12 IOSurface aren't even used:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=614e49e82912

and I get the same failure on 10.6
So the issue has nothing to do with the VDA or VT decoder ; but some changes in the IOSurface code.
(Reporter)

Updated

4 years ago
Flags: needinfo?(matt.woodrow)
(Reporter)

Comment 35

4 years ago
this is with part5 (that makes use of NV12 IOSurface) removed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40308a41df73
(Reporter)

Comment 36

4 years ago
I can reproduce locally (10.10) when I step inside the VT callback. This is the same as the various try failures ([Parent 1108] WARNING: Couldn't pass frame to decoder: file /builds/slave/try-m64-d-00000000000000000000/build/src/dom/media/platforms/apple/AppleVTDecoder.cpp, line 245)

as soon as I hit CVPixelBufferLockBaseAddress ; I see the Apple VT internal decode thread throwing undocumented error like: [22:56:27.775] vtDecompressionDuctDecodeSingleFrame signalled err=268451843 (err) (VTVideoDecoderDecodeFrame returned error) at /SourceCache/CoreMedia_frameworks/CoreMedia-1562.235/Sources/VideoToolbox/VTDecompressionSession.c line 3241


I can find no reference to that 268451843 error . If I ignore that specific error code everything appears okay.

A race maybe and it doesn't like CVPixelBufferLockBaseAddress ? Removing it causes crashes YCbCrImageDataSerializer::CopyData due to null deref.

Back when we were using NV12 software images, we didn't use async decoding. That could explain why it worked then.

Software NV12 only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b7c85245af9

Software NV12 with async decoding disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3e462bd1b93

Software NV12, ignore error 268451843:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03c2793b15f5

Hardware NV12, ignore error 268451843:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab3cba6ce808
(Reporter)

Updated

4 years ago
Depends on: 1192675
(Reporter)

Updated

4 years ago
Flags: needinfo?(matt.woodrow)
Jean-Yves, do you think this fix is safe enough to uplift to Beta 41? I verified it fixes bug 1168552 and bug 1187103 in Nightly 42.0a1 (2015-08-11).
Status: RESOLVED → VERIFIED
Flags: needinfo?(jyavenard)
(Reporter)

Comment 41

4 years ago
I believe so yes..

This patch had been pending for months... Unfortunate that it wasn't checked in earlier.

Alternatively, we could just uplift and set it to return YUV software images instead. This is what patch #7 does. https://hg.mozilla.org/mozilla-central/rev/f57e11edbde2

It would be far less intrusive with less chance of regressiong, and revert to the behaviour we had a year ago.

This was the primary reason I made it originally, and pushed it even though it's currently unused
Flags: needinfo?(jyavenard)
The changeset's commit message says "Currently unused." Is that because the other changes obsolete patch #7, but uplifting just patch #7 would fix the crash/hang? Would it (returning YUV software images) regress video performance on OS X?

I think we should uplift that fix to Beta 41 since the crash/hang was a regression in 40. Do you mind uplifting?
Flags: needinfo?(jyavenard)
(Reporter)

Comment 43

4 years ago
(In reply to Chris Peterson [:cpeterson] from comment #42)
> The changeset's commit message says "Currently unused." Is that because the
> other changes obsolete patch #7, but uplifting just patch #7 would fix the
> crash/hang? Would it (returning YUV software images) regress video
> performance on OS X?

This bug added support for NV12 IOSurface ; part 7 added a software path returning software YUV420 frames.
this is the part that is unused.

should we apply just that patch, it will also fix the kernel crash at the expense of slightly higher CPU usage (being software only).

> 
> I think we should uplift that fix to Beta 41 since the crash/hang was a
> regression in 40. Do you mind uplifting?

it wasn't a regression in 40.
The bug is in Apple VideoToolbox framework. It's been there since at least 10.6.

The bug was exposed in bug 1059066 which made the use of RGB IOSurface
Flags: needinfo?(jyavenard)
Depends on: 1193977
Comment on attachment 8644182 [details] [diff] [review]
Part 7: Use software backed NV12 images on 10.6

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Firefox 41 will continue to have reproducible OS X video crashes. This crash is a long-standing Apple bug, not a Firefox regression.

[Describe test coverage new/current, TreeHerder]: Patch #7 will switch to a software video compositor that we used in Firefox 36 and 37.

[Risks and why]: We don't need to uplift all seven patches on this bug. We can uplift just patch #7 to switch from the crashing hardware video compositor to a software video compositor that we used in Firefox 36 and 37. Patch #7 can't be applied as-is on Beta 41. It must be modified to enable the software code path (mUseSoftwareImages = true).

[String/UUID change made/needed]: None.
Attachment #8644182 - Flags: approval-mozilla-beta?
See Also: → 1149814
Thanks Chris for making the uplift request. Can you also request uplift to Aurora? I would like to uplift to Aurora first and let it stabilize for 2-3 days and then uplift to Beta to ensure there is no unexpected fallout.
Flags: needinfo?(cpeterson)
Comment on attachment 8644182 [details] [diff] [review]
Part 7: Use software backed NV12 images on 10.6

Approval Request Comment: As above.
Attachment #8644182 - Flags: approval-mozilla-aurora?
Flags: needinfo?(cpeterson)
Comment on attachment 8644182 [details] [diff] [review]
Part 7: Use software backed NV12 images on 10.6

Patch has been in m-c for a few days. Also this fixes a crash so it definitely meet the bar. After stabilizing on Aurora for 2-3 days I plan to approve uplift to m-b.
Attachment #8644182 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 48

4 years ago
Are we uplifting the whole stack?

Personally I think it would be worthwhile.
(In reply to Jean-Yves Avenard [:jya] from comment #48)
> Are we uplifting the whole stack?
> 
> Personally I think it would be worthwhile.

You recommended just the one patch!

I would be ok with uplifting the full queue (especially to aurora), but there has been two regressions that would need to be uplifted too.
(In reply to Ritu Kothari (:ritu) from comment #45)
> Thanks Chris for making the uplift request. Can you also request uplift to
> Aurora? I would like to uplift to Aurora first and let it stabilize for 2-3
> days and then uplift to Beta to ensure there is no unexpected fallout.

I hadn't request uplift to Aurora because these patches landed on merge day (2015-08-11) and I mistakenly thought they had made the 42 merge to Aurora. :)


(In reply to Matt Woodrow (:mattwoodrow) from comment #49)
> (In reply to Jean-Yves Avenard [:jya] from comment #48)
> > Are we uplifting the whole stack?
> > 
> > Personally I think it would be worthwhile.
> 
> You recommended just the one patch!
> 
> I would be ok with uplifting the full queue (especially to aurora), but
> there has been two regressions that would need to be uplifted too.

Since we've already had two regressions in Nightly 43, I would be nervous about uplifting the full queue to Beta 41. Jean-Yves said patch #7 will re-enable software compositor code that we had already shipped in Firefox 36 and 37. I have no opinion about uplifting the full queue to Aurora 42.
(Reporter)

Comment 51

4 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #49)
> (In reply to Jean-Yves Avenard [:jya] from comment #48)
> > Are we uplifting the whole stack?
> > 
> > Personally I think it would be worthwhile.
> 
> You recommended just the one patch!

Because it's the safest route.

Not that I feel the other is too great of a risk

> 
> I would be ok with uplifting the full queue (especially to aurora), but
> there has been two regressions that would need to be uplifted too.

I only know of one.. The drawing in the menu bar.
Needs rebasing for Aurora uplift.
Flags: needinfo?(matt.woodrow)
(Reporter)

Comment 53

4 years ago
Enabled by default.

This is a patch for aurora and beta.
Marking 42 as fixed per comment 54.
(In reply to Jean-Yves Avenard [:jya] from comment #53)
> Created attachment 8649702 [details] [diff] [review]
> Add software backed NV12 images support.
> 
> Enabled by default.
> 
> This is a patch for aurora and beta.

In that case, can you please request an aurora and beta uplift on this one?
Flags: needinfo?(jyavenard)
(Reporter)

Comment 57

4 years ago
Comment on attachment 8649702 [details] [diff] [review]
Add software backed NV12 images support.

Part 7: Use software backed NV12 images on 10.6

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Firefox 41 will continue to have reproducible OS X video crashes. This crash is a long-standing Apple bug, not a Firefox regression.

[Describe test coverage new/current, TreeHerder]: Patch #7 will switch to a software video compositor that we used in Firefox 36 and 37.

[Risks and why]: We don't need to uplift all seven patches on this bug. We can uplift just patch #7 to switch from the crashing hardware video compositor to a software video compositor that we used in Firefox 36 and 37. Patch #7 can't be applied as-is on Beta 41.

Patch was modified for aurora and beta so it would always use software NV12 images.

[String/UUID change made/needed]: None.
Flags: needinfo?(jyavenard)
Attachment #8649702 - Flags: approval-mozilla-beta?
(Reporter)

Updated

4 years ago
Flags: needinfo?(matt.woodrow)
Since this patch landed on Aurora42 2 days ago, I'd like this to stabilize some more (maybe until mid next week) before approving uplift to Beta.
Comment on attachment 8644182 [details] [diff] [review]
Part 7: Use software backed NV12 images on 10.6

Removing a? for beta because Jean-Yves requested a? for a rebased version of the patch in comment 57.
Attachment #8644182 - Flags: approval-mozilla-beta?
Comment on attachment 8649702 [details] [diff] [review]
Add software backed NV12 images support.

This patch stabilized on Aurora for a few days and seems safe for uplift to Beta41.
Attachment #8649702 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for beta.
Flags: needinfo?(jyavenard)
(Reporter)

Comment 62

4 years ago
rebased and pushed
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.