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)
Tracking
()
VERIFIED
FIXED
mozilla43
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+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
12.46 KB,
patch
|
ritu
:
approval-mozilla-beta+
|
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.
Reporter | ||
Comment 1•10 years ago
|
||
This patch make the VideoToolbox decoder output a YUV frame, and will display it as-is, (giving a nice negative film effect)
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Reporter | ||
Updated•10 years ago
|
Assignee: jyavenard → nobody
Reporter | ||
Updated•10 years ago
|
Status: ASSIGNED → NEW
Assignee | ||
Comment 2•10 years ago
|
||
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).
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8482661 -
Attachment is obsolete: true
Updated•10 years ago
|
Priority: -- → P2
Assignee | ||
Updated•10 years ago
|
Attachment #8565260 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
Assignee: nobody → matt.woodrow
Attachment #8569419 -
Flags: review?(bgirard)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8569421 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•10 years ago
|
Attachment #8569419 -
Attachment description: Bug 1061525 - Part 1: Add support for planar MacIOSurfaces → Part 1: Add support for planar MacIOSurfaces
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8569422 -
Flags: review?(bgirard)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8569423 -
Flags: review?(bgirard)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8569424 -
Flags: review?(jyavenard)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8569427 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8569430 -
Flags: review?(nical.bugzilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8569423 -
Flags: review?(bgirard) → review?(nical.bugzilla)
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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•10 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 14•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8569423 -
Flags: review?(nical.bugzilla) → review+
Updated•10 years ago
|
Attachment #8569427 -
Flags: review?(nical.bugzilla) → review+
Updated•10 years ago
|
Attachment #8569430 -
Flags: review?(nical.bugzilla) → review+
Reporter | ||
Comment 16•9 years ago
|
||
What's the status with this ?
Any progress? Would be great to have...
Reporter | ||
Comment 17•9 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
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dfbd6e73c7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae6602192a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b48d13edb48d
https://hg.mozilla.org/integration/mozilla-inbound/rev/19c8682665a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5ce3d6412a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e90b9dab7fa
Backed out for mochitest-2 failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=12450634&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddf7484b5ea
Assignee | ||
Comment 20•9 years ago
|
||
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•9 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)
Assignee | ||
Comment 22•9 years ago
|
||
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•9 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
Assignee | ||
Comment 24•9 years ago
|
||
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•9 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•9 years ago
|
||
Using NV12 IOSurface cause some hangs.
Attachment #8644182 -
Flags: review?(matt.woodrow)
Reporter | ||
Comment 27•9 years ago
|
||
Assignee | ||
Comment 28•9 years ago
|
||
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+
Assignee | ||
Comment 29•9 years ago
|
||
Looks like that didn't fix the problem :(
Reporter | ||
Comment 30•9 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•9 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•9 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•9 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•9 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•9 years ago
|
Flags: needinfo?(matt.woodrow)
Reporter | ||
Comment 35•9 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•9 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 | ||
Comment 37•9 years ago
|
||
Doh !
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac1cc96964b0
Issue was in patch #5.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(matt.woodrow)
Comment 38•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/97535b0b245d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c929f2deaad
https://hg.mozilla.org/integration/mozilla-inbound/rev/d86e1a40899c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3b4dd184e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/cab8bed7404e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb3603f5c9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f57e11edbde2
https://hg.mozilla.org/mozilla-central/rev/97535b0b245d
https://hg.mozilla.org/mozilla-central/rev/9c929f2deaad
https://hg.mozilla.org/mozilla-central/rev/d86e1a40899c
https://hg.mozilla.org/mozilla-central/rev/3f3b4dd184e2
https://hg.mozilla.org/mozilla-central/rev/cab8bed7404e
https://hg.mozilla.org/mozilla-central/rev/2eb3603f5c9f
https://hg.mozilla.org/mozilla-central/rev/f57e11edbde2
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment 40•9 years ago
|
||
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
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → verified
Flags: needinfo?(jyavenard)
Reporter | ||
Comment 41•9 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)
Comment 42•9 years ago
|
||
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•9 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)
Comment 44•9 years ago
|
||
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?
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)
Assignee | ||
Comment 46•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
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•9 years ago
|
||
Are we uplifting the whole stack?
Personally I think it would be worthwhile.
Assignee | ||
Comment 49•9 years ago
|
||
(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.
Comment 50•9 years ago
|
||
(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•9 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.
Reporter | ||
Comment 53•9 years ago
|
||
Enabled by default.
This is a patch for aurora and beta.
Reporter | ||
Comment 54•9 years ago
|
||
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•9 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•9 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 59•9 years ago
|
||
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+
Reporter | ||
Comment 63•9 years ago
|
||
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•