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: fix relative seeks #10674

Merged
merged 2 commits into from Oct 12, 2016

Conversation

@FernetMenta
Copy link
Member

commented Oct 10, 2016

Holding down the key generates many relative seek commands. If the calling thread is blocked, there is not enough time for rendering.
Now VideoPlayer processes those messages asynchronously.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2016

jenkins build this please

@popcornmix

This comment has been minimized.

Copy link
Member

commented Oct 10, 2016

You'll need a fix like: popcornmix@459aebf
Should I see any change with this PR? Behaviour when holding down right hasn't changed (video plays at normal speed until released).

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2016

I will pick the omx fix, thanks

video plays at normal speed until released

if this is the case, something is wrong with mmal or input. a seek flushes demux queue and decoder. does player get seek commands?

@FernetMenta FernetMenta force-pushed the FernetMenta:sync branch from 3ae4dbc to dc15a29 Oct 10, 2016

@popcornmix

This comment has been minimized.

Copy link
Member

commented Oct 10, 2016

I've added a log message to SeekTimeRelative, but I only see that when I release the key.
The seek bar does move while I'm holding the key down, and it does seek to the expected place when I release.
http://paste.ubuntu.com/23305122/

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2016

Where did you add the log? Maybe in issue in SeekHandler. Could you add some logging to this chain of functions please. Something must go wrong before it comes to VideoPlayer.

@da-anda

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

might you guys be talking about different things? Multiple skips steps vs only one with instant seek (aka old behavior before skip steps)?

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2016

@da-anda use case here is having additive seek disabled by setting delay to zero

@popcornmix

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

Log was in SeekTimeRelative.
I've found the issue. I had set a single seek step forwards and backwards but not zeroed the skip delay. I had imagined that with only a single seek step then the delay would be ignored (what does it mean in this case?)
I now get ~10 log message per second from SeekTimeRelative. Player shows 0% buffering constantly while holding the seek key down, but you do get one or two video frames displayed about once per second.

Seeking is much slower when delay is set to 0.
With delay=750ms and step=30s I can seek from start to end of 42 minute video in 7 seconds.
With delay=0ms and step=30s I can seek from start to end of 42 minute video in 114 seconds.

I assume that in the first case we get ~10 seeks of 30s per second.
In the second I assume we only update the current seek position when video frame emerge, so ~1 seek of 30s per second.

Log file with delay set to zero.
http://paste.ubuntu.com/23307782/

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2016

Seeking is much slower when delay is set to 0.
With delay=750ms and step=30s I can seek from start to end of 42 minute video in 7 seconds.
With delay=0ms and step=30s I can seek from start to end of 42 minute video in 114 seconds.

That's expected behaviour, isn't it? Before being able to do the next jump you have to land first.

but you do get one or two video frames displayed about once per second.

That's a remaining issue. Every relative seek should result in a displayed video frame.

@popcornmix

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

That's expected behaviour, isn't it? Before being able to do the next jump you have to land first.

It's one possible expected behaviour...
Compare with 8x fast forward. The clock moves at 8x real time regardless of how long it is taking the seek/demux/decode/render paths. On a slower platform/network/disk the clock moves at a fixed rate but you get fewer decoder video frames during the process.
The repeated skip could behave the same way. The number of 30s seeks per second depends on the key repeat rate. Skipping from one end of a one hour video could always take the same time, with speed of platform determining the time between video frames actually appearing.

But that is probably a harder system to implement, and isn't a blocker for this PR.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2016

if all you want to achieve is getting fast from one end to the other, you should use additive seek or big step forward. not producing a picture after a relative seek makes no sense bacause you don't know where you are.

@popcornmix

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

No problem - as long as the behaviour is expected I'm happy.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2016

jenkins build this please

@FernetMenta FernetMenta merged commit 706f246 into xbmc:master Oct 12, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins4kodi You did a great job. Have a cookie.
Details

@FernetMenta FernetMenta deleted the FernetMenta:sync branch Oct 12, 2016

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