Closed Bug 1061525 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect, P2)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- verified

People

(Reporter: jya, Assigned: mattwoodrow)

References

Details

Attachments

(9 files, 2 obsolete files)

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
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.
Blocks: 1059066
Attached 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)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Assignee: jyavenard → nobody
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).
Blocks: 1077277
Blocks: 1133633
Attached patch mac-io-yuv (obsolete) — Splinter Review
Attachment #8482661 - Attachment is obsolete: true
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 #8569430 - Flags: review?(nical.bugzilla)
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+
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)
What's the status with this ? Any progress? Would be great to have...
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)
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?
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).
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)
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 :(
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
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
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
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
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.
Flags: needinfo?(matt.woodrow)
this is with part5 (that makes use of NV12 IOSurface) removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40308a41df73
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
Depends on: 1192675
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)
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)
(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+
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.
Depends on: 1192571
(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.
(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)
Enabled by default. This is a patch for aurora and beta.
(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?
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?
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)
rebased and pushed
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: