[osx] Force reset in VideoSyncCocoa when refresh rate changes #6359

jingai · 2015-02-08T01:01:15Z

The existing implementation just updates m_fps, but nothing is fed back to the reference clock. Instead of calling UpdateFPS(), just force a reset, as it does in VideoSyncD3D.

This PR makes refresh rate adjustment work on OSX, which was the intended fix in PR #6146 / #6147.

However, it's a little suboptimal because the first time it goes to set things up, the call to CVDisplayLinkGetActualOutputVideoRefreshPeriod() in Cocoa_GetCVDisplayLinkRefreshPeriod() returns 0.0, which makes Cocoa_GetCVDisplayLinkRefreshPeriod() default to a return value of 60.0. So the video reference clock is set to 60.0 right off the bat.

In reality, the display is just in the process of switching over (which can take seconds on some TVs), so this means for every video played where refresh rate adjustment is required, it has to create the device, destroy it, and create it again.

Additionally, the audio is out of sync even after it does figure things out. Pressing 'm' to bring up VideoOSD forces it to sync up, but it's a little annoying to have to do that every time. [side note: what function is called to force-sync things when VideoOSD is brought up?]

I'm soliciting some discussion about how this should really be handled properly. Ideally, I think it should only set things up after the resolution change has completed (that is, CVDisplayLinkGetActualOutputVideoRefreshPeriod() returns > 0.0) to avoid creating and destroying things more than necessary (to speed up the start of playback). But it also needs to handle refresh rate/resolution changes on-the-fly as well (i.e., getting audio and video back in sync if the user just changes the refresh rate outside of Kodi, without requiring the user to bring up VideoOSD or some other UI element that forces a resync).

FernetMenta · 2015-02-08T07:17:58Z

is this really needed after #6356 ?

xbmc/video/videosync/VideoSyncCocoa.cpp

+  if (!m_displayLost)
+  {
+    m_displayLost = true;
+    m_lostEvent.Wait();


jingai · 2015-02-08T09:10:32Z

I'll check with #6356 tomorrow and let you know, but I suspect they both are needed. As noted, the reference clock will not be updated if the refresh rate changes after it was initialized.

jingai · 2015-02-08T10:23:19Z

Updated to ignore OnLostDevice.

Checking in combination with #6356 now.

jingai · 2015-02-08T11:15:27Z

With this PR plus #6356 I still have to bring up the VideoOSD (press 'm') to get it to sync up.

At initial setup it says:

05:53:06 T:4726853632   DEBUG: CVideoSyncCocoa: cleaning up Cocoa
05:53:06 T:4726853632   DEBUG: CVideoSyncCocoa: setting up Cocoa
05:53:06 T:4726853632   DEBUG: CVideoSyncCocoa: Detected refreshrate: 60 hertz

^^^^^ note this is the first detection, and it's wrong

05:53:06 T:4726853632   DEBUG: CVideoSyncCocoa: Detected refreshrate: 24 hertz
05:53:06 T:4726853632   DEBUG: CVideoReferenceClock: Detected refreshrate: 24.000 hertz
05:53:06 T:4734943232   DEBUG: CVideoReferenceClock: Clock speed 100.100000%

^^^^^ detected the correct refresh rate here, and set the reference clock

05:53:10 T:4734943232   DEBUG: CPullupCorrection: detected pattern of length 1: 41708.33, frameduration: 41708.333333
05:53:30 T:4595998720   DEBUG: Thread JobWorker 4595998720 terminating (autodelete)
05:53:30 T:4594388992   DEBUG: Thread JobWorker 4594388992 terminating (autodelete)
05:53:30 T:4620288000   DEBUG: Thread JobWorker 4620288000 terminating (autodelete)
05:54:04 T:140735221764864   DEBUG: ------ Window Init (VideoOSD.xml) ------
05:54:04 T:140735221764864    INFO: Loading skin file: VideoOSD.xml, load type: KEEP_IN_MEMORY

^^^^^ I hit 'm' here to bring up the VideoOSD, and it sync'ed everything up. Prior to that, the audio was not in sync with the video.

FernetMenta · 2015-02-08T11:21:19Z

I don't think that the remaining issue is related to the clock. you were testing with vda, right? what if you switch off vda?

jingai · 2015-02-08T11:29:52Z

Same thing. With or without vda, the audio is out of sync until i press 'm' to bring up VideoOSD.

FernetMenta · 2015-02-08T11:32:13Z

can you post a debug log which includes pressing m

Memphiz · 2015-02-08T12:04:19Z

i don't get it - this was implemented like done in DRM

https://github.com/xbmc/xbmc/blob/master/xbmc/video/videosync/VideoSyncCocoa.cpp#L140

this aborts the thread on resetdisplay - so why does that not work?

Memphiz · 2015-02-08T12:11:20Z

refreshchanged method seems to be unused in this pr now

jingai · 2015-02-09T00:30:37Z

@FernetMenta, sorry for the delay, had to go to bed.. here you go http://pastebin.com/vfngg8vY

That log shows a few things:

It incorrectly identifies the refresh rate several times as 60Hz (display not yet actually switched),
It eventually picks up the correct rate (24Hz) because of the patch here, where it sets m_resetDisplay true in the vblank handler when it sees a new refresh rate,
Audio and Video are out of sync until I pressed 'm' (400%, but it's not always that).

jingai · 2015-02-09T00:33:17Z

@Memphiz, the whole problem is that it doesn't correctly identify the refresh rate when setting things up, because it's racing against the display change (and wins), I had to make the vblank handler set m_resetDisplay to true when the resolution change actually finishes. Without this patch, it just sets m_fps in VideoSyncCocoa but it is never fed back to the reference clock, so the codec info overlay reports that the display is at 60Hz.

jingai · 2015-02-11T19:45:17Z

I went through looking for commits that were seemingly/potentially related to A/V sync, which amounted to:

de25db1 dvdplayer: discard render buffers if gui is not active
0aab42e dvdplayer: only consider standard framerates for vfr
fd3f91a dvdplayer: fix clock speed
f014a75 dvdplayer: reset speed of video reference clock after refresh rate changed
274a6cc [pvr] - set framerate if not set by demuxer
f1937ef dvdplayer: fix changing refresh rate after invalid state
213857b dvdplayer: consider audio stream stalled if queue ran dry
5cacbc6 dvdplayer: avoid unwanted dropping in output stage of video
06d4816 VAAPI: avoid reset when running out of surfaces
27f434b dvdplayer: do not reset hw decoder if just ffmpeg get_buffer failed
66513a0 dvdplayer: drop dead code - IHardwareDecoder::Section
55ce02e dvdplayer: fix calculation of level when data based

I branched from the Helix branch and applied all of the above. Without my patch, it never gets the refresh rate right (stays at 60).

With my patch (plus the above commits), it's better, but if I don't set the "Pause playback during refresh rate change" option to at least 2 seconds, it's wildly out of sync and won't dial back in (pressing 'm' does sync it up though). Even with the pause it's a small bit out of sync, but it corrects for it within a few seconds or so.

FernetMenta · 2015-02-12T07:00:09Z

when it's out of sync, is audio early or late? we need to find out why pressing m syncs up. what if you disable "sync playback to display"?

jingai · 2015-02-12T13:52:28Z

It's early in both cases (with and without "sync playback to display"). And in both cases, pressing 'm' causes it to sync up.

FernetMenta · 2015-02-13T06:59:07Z

could you please disable "sync playback", enable verbose logging for video and post debug log which includes the issue

EDIT: without vda, full debug log please

jingai · 2015-02-13T12:03:08Z

Will do in just a minute. Wanted to show this one to you too first.

Last night we watched a show and I noticed it took a full minute to figure out what the display's refresh rate was.. maybe something is wrong with the way Cocoa_GetCVDisplayLinkRefreshPeriod() detects the refresh rate, or a bug in OSX, I don't know.

Also, for some reason the audio cut out twice (log says device list changed, so it closed the stream and reopened). This also caused the audio and video to go out of sync (again, pressing 'm' sync'ed it back up).

http://pastebin.com/PPk4gwgK

FernetMenta · 2015-02-13T12:08:42Z

please don't cut the logs

jingai · 2015-02-13T12:33:08Z

I just tried without VDA and 'sync playback' (but with the above patches) and could not reproduce the A/V sync issue. So I re-enabled 'sync playback' and it still works.

I then re-enabled VDA and the problem is back immediately. Disabled it again, and it's gone. So it's definitely something to do with VDA.

I checked again with the official 14.1 release and it reports 60Hz again in the codec overlay (the issue my patch addresses). Different issue, but wanted to verify that my patch here was in fact still necessary without VDA.

This is with my patch and the aforementioned commits. Without my patch at least, I can't tell if disabling VDA solves the A/V sync issue because it's syncing to the wrong rate (60Hz instead of 24Hz).

I'll get full logs next time. Was trying to give you just the portion that showed the issue (we watched several shows last night), but I understand.

FernetMenta · 2015-02-13T12:43:00Z

this is an old vda issue. have you tried switching to ffmpeg vda. current vda will die soon after I have got my Mac.

jingai · 2015-02-13T12:44:41Z

I haven't tried that. How would I go about switching to ffmpeg vda?

edit: Nevermind, I found it in the sources. Trying now..

jingai · 2015-02-13T13:15:46Z

It works with "useffmpegvda" enabled.

The only remaining issue seems to be that it can take a long time to detect the refresh rate change. The log below shows it taking 20 seconds to figure it out, but I've seen it take upwards of a minute as well. This could be an OSX thing though, I don't know.

Besides that though, with my patch (and your patches noted above) and "useffmpegvda" enabled, it seems to be working normally.

http://pastebin.com/d01dy60T

jingai · 2015-02-13T13:44:12Z

Hmm, I think I know how I can fix the remaining issue.

Right now, Cocoa_GetCVDisplayLinkRefreshPeriod() returns a hard-coded value of 60.0 when CVDisplayLinkGetActualOutputVideoRefreshPeriod() returns <= 0.0. Instead, it should probably return the anticipated refresh rate; that is, the refresh rate associated with the resolution found in ChooseBestResolution(). This way, even if the OS doesn't quite know what the refresh rate currently is, at least the video won't appear 'jerky' to the user.

If I'm off-base, let me know; otherwise, I can look into implementing it this way.

FernetMenta · 2015-02-13T13:59:31Z

video ref clock should not call Cocoa_GetCVDisplayLinkRefreshPeriod at all. GraphicsContext should be single source of truth (see DRM) Windowing for Darwin should have a mechanism to listen to internal and external triggered changes of refresh rate. see Linux X11. While playing a video you can change refresh rate externally with i.e. xrandr.

jingai · 2015-02-13T14:12:34Z

It currently does, though. When setting things up, it calls CVideoReferenceClock::UpdateRefreshrate() which in turn calls m_pVideoSync->GetFps(), which on Darwin calls Cocoa_GetCVDisplayLinkRefreshPeriod() for its own return value.

If I'm understanding all of this right, you're suggesting that CVideoSyncCocoa::GetFps() should just return g_graphicsContext.GetFPS()? Assuming g_graphicsContext.GetFPS() returns the correct value anyway. I can try this when I get home if I have this right.

FernetMenta · 2015-02-13T14:19:29Z

yes. and in case g_graphicsContext.GetFPS returns not the correct value, it needs to be fixed.

jingai · 2015-02-13T14:20:25Z

Ok, I'll test this either tonight or tomorrow morning then. Valentine's Day dinner tonight with the wife so she may not let me spend time on it until tomorrow :)

Memphiz · 2015-02-13T14:27:28Z

Mhhh well Cocoa_GetCVDisplayLinkRefreshPeriod calculates the refreshrate based on the displaylink vsync callback of the os. If this is not what it should do then we could get rid of alot of code indeed. And of course is osx windowing knowing when the video mode changes - thats why it implements IDispResource. I think i am missing a big point in this whole idea. To me it always seamed that everything we need for osx is available already - its just that components don't play together as they should it seems. Most likely because something is implemented not like its ment to be.

FernetMenta · 2015-02-14T17:22:29Z

thanks. this looks ok to me

jingai · 2015-02-14T17:34:27Z

Awesome. Want me to rebase and squash then?

FernetMenta · 2015-02-14T17:44:08Z

lets wait for @Memphiz to comment. he can decide whether we can drop rounding for ios too

xbmc/video/videosync/VideoSyncCocoa.cpp


 bool CVideoSyncCocoa::Setup(PUPDATECLOCK func)
 {
-  CLog::Log(LOGDEBUG, "CVideoSyncCocoa: setting up Cocoa");
+  CLog::Log(LOGDEBUG, "CVideoSyncCocoa::%s - setting up Cocoa", __FUNCTION__);
  bool setupOk = false;


Consider graphicsContext as single-source-of-truth for the refresh rate instead of returning Cocoa_GetCVDisplayLinkRefreshPeriod() in CVideoSyncCocoa::GetFps().

Memphiz · 2015-02-14T18:00:01Z

i have no clue - @davilla wrote that code - maybe he can ask mr. goldfish what was the reason for the rounding

FernetMenta · 2015-02-14T18:04:49Z

we had this for all platforms, ancient xrandr and Windows only returned int. I have changed the video ref clock to operate with float. All platforms capable of delivering float should do so.

davilla · 2015-02-14T18:07:16Z

too long ago, I'm sure there was a reason as I tend to stay away from MathUtils::round_int unless I absolutely need it to make something work correctly.

FernetMenta · 2015-02-14T18:10:14Z

https://github.com/xbmc/xbmc/blob/Gotham/xbmc/video/VideoReferenceClock.h#L96
that was the reason

Memphiz · 2015-02-14T18:16:32Z

fine by me then

Memphiz · 2015-02-14T18:16:48Z

for ios too of course

jingai · 2015-02-14T18:20:25Z

Done.

FernetMenta · 2015-02-14T18:45:53Z

jenkins build this please

FernetMenta · 2015-02-15T07:38:13Z

jenkins build this please

Memphiz · 2015-02-15T11:10:03Z

@jingai - you made the vblankhandler method osx only by moving the #endif - but its called from ios - thats why ios failed to link

Memphiz · 2015-02-15T11:18:38Z

@jingai - nvm its a leftover - you need to remove the m_videosync member from ios windowing and also the call to vblankhandler in ioseaglview - i can do it later today

jingai · 2015-02-15T14:15:26Z

Yeah, it didn't appear to be applicable to iOS so it seemed reasonable. I haven't built for iOS so I didn't notice that, sorry. I can add those changes to this PR but I wouldn't be able to do it until later today either.

jingai · 2015-02-15T14:33:38Z

There, I just reverted that. It belongs in the cleanup commit you're going to make anyway.

Memphiz · 2015-02-15T14:36:23Z

@jingai i had a talk with fernetmenta on irc and i have the bigger picture now. I hope you are not mad at me but i will refactor the implementations of videosync for cocoa and ios into 2 seperate implementations and clean it up. Thx a lot for pointing out the fix for osx (relying on fps from graphicscontext that is) - without your afford i wouldn't have looked into it. While its a bit demotivating for sure that your work seems unused now - believe me it was more then welcome and helpfull.

jingai · 2015-02-15T14:39:08Z

Oh it doesn't bother me at all so long as it gets fixed :) It's been in a semi-non-functional state for a while now and it just bothered me enough to look into it.

Let me know when you've got something to test and I'll check it on my end too.

Memphiz · 2015-02-15T17:11:37Z

@jingai you could already give this a shot if you want (osx only for now - will do ios now...)

https://github.com/Memphiz/xbmc/tree/darwinvideosync

jingai · 2015-02-15T21:10:04Z

@Memphiz, so far so good. Working exactly as my patches did. I'll keep an eye on your branch and test as it comes along.

Memphiz · 2015-02-15T21:17:51Z

superseeded by #6456 for helix and #6455 for master

FernetMenta reviewed Feb 8, 2015
View reviewed changes

jingai force-pushed the osxfixrefreshrateadjust branch from f2c7505 to ff39dc9 Compare February 8, 2015 10:19

jingai force-pushed the osxfixrefreshrateadjust branch from ff39dc9 to 2423e8e Compare February 9, 2015 19:45

afedchin reviewed Feb 14, 2015
View reviewed changes

jingai added 2 commits February 14, 2015 12:50

[osx] VideoSyncCocoa: Return FPS from graphicsContext in GetFps().

a7e3e9e

Consider graphicsContext as single-source-of-truth for the refresh rate instead of returning Cocoa_GetCVDisplayLinkRefreshPeriod() in CVideoSyncCocoa::GetFps().

[osx/ios] VideoSyncCocoa log refresh rate as floating point value.

e24b883

[ios] CVideoSyncCocoa::GetFps() return float value.

2e7ed31

jingai force-pushed the osxfixrefreshrateadjust branch from 19ff8f8 to 2e7ed31 Compare February 14, 2015 18:19

[ios] Add CVideoSyncCocoa::VblankHandler back in.

431422b

Memphiz closed this Feb 15, 2015

MartijnKaijser modified the milestone: Abandoned, obsolete or superseeded Feb 17, 2015

[osx] Force reset in VideoSyncCocoa when refresh rate changes #6359

[osx] Force reset in VideoSyncCocoa when refresh rate changes #6359

Conversation

jingai commented Feb 8, 2015

FernetMenta commented Feb 8, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jingai commented Feb 8, 2015

jingai commented Feb 8, 2015

jingai commented Feb 8, 2015

FernetMenta commented Feb 8, 2015

jingai commented Feb 8, 2015

FernetMenta commented Feb 8, 2015

Memphiz commented Feb 8, 2015

Memphiz commented Feb 8, 2015

jingai commented Feb 9, 2015

jingai commented Feb 9, 2015

jingai commented Feb 11, 2015

FernetMenta commented Feb 12, 2015

jingai commented Feb 12, 2015

FernetMenta commented Feb 13, 2015

jingai commented Feb 13, 2015

FernetMenta commented Feb 13, 2015

jingai commented Feb 13, 2015

FernetMenta commented Feb 13, 2015

jingai commented Feb 13, 2015

jingai commented Feb 13, 2015

jingai commented Feb 13, 2015

FernetMenta commented Feb 13, 2015

jingai commented Feb 13, 2015

FernetMenta commented Feb 13, 2015

jingai commented Feb 13, 2015

Memphiz commented Feb 13, 2015

FernetMenta commented Feb 14, 2015

jingai commented Feb 14, 2015

FernetMenta commented Feb 14, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Memphiz commented Feb 14, 2015

FernetMenta commented Feb 14, 2015

davilla commented Feb 14, 2015

FernetMenta commented Feb 14, 2015

Memphiz commented Feb 14, 2015

Memphiz commented Feb 14, 2015

jingai commented Feb 14, 2015

FernetMenta commented Feb 14, 2015

FernetMenta commented Feb 15, 2015

Memphiz commented Feb 15, 2015

Memphiz commented Feb 15, 2015

jingai commented Feb 15, 2015

jingai commented Feb 15, 2015

Memphiz commented Feb 15, 2015

jingai commented Feb 15, 2015

Memphiz commented Feb 15, 2015

jingai commented Feb 15, 2015

Memphiz commented Feb 15, 2015