Closed Bug 1153469 Opened 5 years ago Closed 5 years ago
stuttering/corruption with 4k youtube and external monitor
Trying Youtube with a uhd monitor I ran into serious playback issues. Dell P2715Q was set the multistream secondary mode, so laptop could only drive it at 2160p30. (This is because of a hardware limitation with my older laptop; newer macs can drive it at 2160p60 is in single stream displayport mode.) Visiting https://www.youtube.com/watch?v=iNJdPyoqt8U and selecting "2160p4k" from the quality popup under the gear menu in the youtube player, I see a number of issues: Playback stutters like frame ordering is incorrect. This is especially obvious near scene changes where it will flip between frames from either side of the cut. It also sometimes plays backwards for a few frames. There are also occasional corrupt frames with glitches like areas of white blocks, or the left and right halves of the frame swapped (like the base pointer was incorrect) or tearing between different frames. I see the issues even when not playing back at full screen, but I don't see them when no external monitor is connected. Reproduced with an early 2013 macbook pro running 10.8.5 in Aurora 39 and Nightly 40, and in Nightly 40 on a late 2013 macbook pro running 10.10.3. Former has nVidia GeForce GT 650M and 1 GB of video ram. Later has GT 750M with 2 GB of video ram (and also integrated intel GPUs, but I don't think those were being used at the time). Note that the video is very large. "Stats for Nerds" claimed between 10 and 70 Mbps for traffic, so without a fast connection it will just buffer all the time.
How does it play with safari?
In http://uri-labs.com/macosx_headers/CVPixelBufferIOSurface_h/index.html: can read: "The CVPixelBuffer will retain the IOSurface. IMPORTANT NOTE: If you are using IOSurface to share CVPixelBuffers between processes and those CVPixelBuffers are allocated via a CVPixelBufferPool, it is important that the CVPixelBufferPool does not reuse CVPixelBuffers whose IOSurfaces are still in use in other processes. CoreVideo and IOSurface will take care of this for if you use IOSurfaceCreateMachPort and IOSurfaceLookupFromMachPort, but NOT if you pass IOSurfaceIDs. " Matt, we are increasing the usage count of the IOSurface, but we aren't using mach port anywhere in the MacIOSurface. Sounds like this is what is happening here. The CVPixelBuffer is re-use before the IOSurface has the chance to be rendered. So what is displayed appears out of order as a frame could be displayed more than once.
We're seeing this in the non-e10s case, so I don't think this is specific to cross process sharing. Maybe we need to do something else to stop the IOSurface being re-used though? We do share IOSurfaces to the compositor thread using the IOSurfaceID, but we should be ensuring that at least one MacIOSurface is alive at all times (which is what is holding the use-count reference), so that should be fine. It's possible that last assertion isn't true, and we let all MacIOSurfaces die while still holding an IOSurfaceID?
You should try making the MacIOSurfaceTextureClientOGL destructor  hold a ref to the MacIOSurface until the compositor side has been torn down too, that would make it impossible for this to happen. We have a 'KeepUntilFullDeallocation' method for this, see what TextureClientD3D11  does.  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/MacIOSurfaceTextureClientOGL.cpp#18  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.cpp#187
The MacIOSurface does hold a reference to the IOSurface. Is the MacIOSurface released before the compositor finished with the IOSurface itself?
MacIOSurfaceTextureClientOGL holds a MacIOSurface, as does MacIOSurfaceTextureHostOGL. Our layers infrastructure asynchronously constructs the Host after the Client is created, and does so by serializing the IOSurfaceID. It is however possible that the Client is destroyed before the Host is created, so we'd have a brief period where no MacIOSurfaces exist for the IOSurfaceID that has been serialized.
Matt, you champion.. That did fix the issue I was seeing, especially that one https://roystanross.wordpress.com/super-mario-64-hd/
Attachment #8591314 - Flags: review?(matt.woodrow)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Comment on attachment 8591314 [details] [diff] [review] Ensure IOSurface isn't released before being composited Review of attachment 8591314 [details] [diff] [review]: ----------------------------------------------------------------- Awesome!
Attachment #8591314 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8591314 [details] [diff] [review] Ensure IOSurface isn't released before being composited Approval Request Comment [Feature/regressing bug #]:1153469 [User impact if declined]: Invalid/Corrupted frame being displayed. [Describe test coverage new/current, TreeHerder]: Locally tested on videos known to have problems. [Risks and why]: It's a wonder it ever worked without it [String/UUID change made/needed]: None
5 years ago
Duplicate of this bug: 1142901
Tracking this for 38+.
Comment on attachment 8591314 [details] [diff] [review] Ensure IOSurface isn't released before being composited Should be in 38 beta 4.
5 years ago
Duplicate of this bug: 1148171
You need to log in before you can comment on or make changes to this bug.