Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VideoPlayer: consider offset to vsync #9629

Merged
merged 4 commits into from Apr 16, 2016

Conversation

@FernetMenta
Copy link
Member

commented Apr 15, 2016

PTS of a video frame refers to the time the frames comes to display. But this time depends on vsync. This change determines the offset from video player's clock to vsync and adjusts audio sync.

Render thread looks at the middle of the the timespan of a frame. This reduces the chance that it doesn't pick a frame because it is not due by i.e. a 1ms. This resulted in rendering a frames twice and having to drop /skip next frames.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

@MilhouseVH could you test if this fixes the issue on your ION system?

@@ -39,6 +39,8 @@ CDVDClock::CDVDClock()
m_systemAdjust = 0;
m_speedAdjust = 0;
m_startClock = 0;
m_vSyncAdjust = 0;
m_frameTime = 1/60 * DVD_TIME_BASE;

This comment has been minimized.

Copy link
@popcornmix

popcornmix Apr 15, 2016

Member

Integer division will always set m_frameTime to zero

This comment has been minimized.

Copy link
@popcornmix

popcornmix Apr 15, 2016

Member

Should it know about the actual framerate?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 15, 2016

Author Member

oh, thanks. yes it should know about frametime because DISCONT corrections by audio are only allowed in intervals of frametime

@popcornmix

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

Just an observation.

We will never present a frame early, but may present a frame late (due to cpu being busy). The greater the ime we can tolerate being late the better (e.g. when rendering the first subtitle of a new font or bringing up OSD we'd prefer not to skip if possible).
Scheduling frames at mid-point of vsyncs allows 8ms of lateness (at 60Hz).
Scheduling frames just after vsync allows 16ms of lateness (at 60Hz).

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

We will never present a frame early,

this is not correct on all platforms. if drivers buffer internally, you have to take case that frames are not presented early.

Scheduling frames at mid-point of vsyncs allows 8ms of lateness (at 60Hz).

Frames are not scheduled at mid-point. It does not matter if lateness is 8ms or 16ms. Either we are late by an entire frame or not. If we are late by a frame the system allows 10 frames to recover until player will request a drop.

@FernetMenta FernetMenta force-pushed the FernetMenta:vplayer branch from bc683dc to 27ba0b3 Apr 15, 2016

@FernetMenta FernetMenta force-pushed the FernetMenta:vplayer branch from 27ba0b3 to 26796e9 Apr 15, 2016

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

I added another commit that simplifies lateness detection. Lateness is calculated in frames now.

@FernetMenta FernetMenta added this to the Krypton 17.0-alpha1 milestone Apr 15, 2016

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2016

Thanks @FernetMenta, it does fix the issue, mostly. Playback is now looking normal, although I am seeing occasional frame skips and/or drops - perhaps 1 skip or drop every 3-4 minutes (23.976 h264 with "Adjust refresh rate" enabled). The VsyncOff value is increasing slowly (0.2-0.3-ish every 1-2 seconds) until it seems to reach some sort of limit and a skip or drop occurs, then it resets (or even starts to count down).

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

@MilhouseVH increasing VSyncOff means that video fps does not exactly match refresh rate. You know that with a custom modeline NVidia systems switch to 23.971 instead of 23.976, don't you? Did you try with "sync playback to display"?

@fritsch

This comment has been minimized.

Copy link
Member

commented Apr 16, 2016

Please post /usr/lib/kodi/kodi-xrandr | pastebinit please - here we will
see if you have the correct nvidia Modelines, which are still after 5+
years to be set manually :-(

2016-04-16 8:51 GMT+02:00 Rainer Hochecker notifications@github.com:

@MilhouseVH https://github.com/MilhouseVH increasing VSyncOff means
that video fps does not exactly match refresh rate. You know that with a
custom modeline NVidia systems switch to 23.971 instead of 23.976, don't
you? Did you try with "sync playback to display"?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#9629 (comment)

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2016

@MilhouseVH increasing VSyncOff means that video fps does not exactly match refresh rate. You know that with a custom modeline NVidia systems switch to 23.971 instead of 23.976, don't you?

Ummm... no I didn't actually! I've never knowingly needed it until the changes in these recent builds after #409.

Did you try with "sync playback to display"?

This is with "Sync playback to display" and "Adjust display refresh rate" both enabled:

s1

then a few minutes later:

s2

Please post /usr/lib/kodi/kodi-xrandr | pastebinit please - here we will see if you have the correct nvidia Modelines, which are still after 5+ years to be set manually :-(

/usr/lib/kodi/kodi-xrandr: http://sprunge.us/OGNK

I'm using a custom xorg.conf, mainly to allow the Revo3700 to find the display when it powers on before the Amp (Onkyo TX-NR828):

/storage/.config/xorg.conf: http://sprunge.us/WRHb
/storage/.config/edid.bin: http://sprunge.us/giYN (dumped using RPi edidparser)

Thanks both.

@fritsch

This comment has been minimized.

Copy link
Member

commented Apr 16, 2016

See:

<mode id="0x1c9" name="1920x1080" w="1920" h="1080" hz="24.00000" current="false" preferred="false"/>
<mode id="0x1ca" name="1920x1080" w="1920" h="1080" hz="23.97091" current="true" preferred="false"/>

You miss the proper 23.9768 mode, e.g.:

ModeLine       "1920x1080_24" 74.250 1920 2558 2602 2750 1080 1084 1089 1125 +hsync +vsync
ModeLine       "1920x1080_23.976" 74.175 1920 2558 2602 2750 1080 1084 1089 1125 +hsync +vsync

See the OpenELEC wiki for more information.

@koenkooi

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2016

As a data point: disabling vsync on Jarvis fixed the staggering amount of framedrops when using matching refreshrates and fps.

See http://forum.kodi.tv/showthread.php?tid=227199&pid=2312398#pid2312398 for more info.

@fritsch

This comment has been minimized.

Copy link
Member

commented Apr 16, 2016

That's not a data point, that's just insane forcing TearFree ... and Jarvis has absolutely nothing (zero) to do with that part of the code, which is completely rewritten.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2016

Thanks @fritsch.

After adding the missing modelines, it still wasn't working - turns out the edid.bin from my Onkyo TX-NR828 amp is faulty:

[518502.451] (WW) NVIDIA(0): No valid modes for "DFP-0:1920x1080_23.976"; removing.

So I'm now using an edid.bin from a Marantz amp which seems to be working better now.

OE Wiki page for reference: http://wiki.openelec.tv/index.php?title=Configuring_a_Custom_xorg.conf

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

jenkins build this please

@FernetMenta FernetMenta merged commit d403095 into xbmc:master Apr 16, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Merged build finished.
Details

@FernetMenta FernetMenta deleted the FernetMenta:vplayer branch Apr 16, 2016

}
else
{
m_clockSync.m_enabled = true;

This comment has been minimized.

Copy link
@popcornmix

popcornmix May 3, 2016

Member

Should this be false?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 4, 2016

Author Member

oops. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.