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

Search backwards for keyframe when resuming playback #11713

Merged

Conversation

@dgam1
Copy link
Contributor

commented Feb 21, 2017

Currently, when resuming playback of a video, a human can easily notice how the player will jump up to several seconds ahead of the resume point. The reason being that the player will seek to the closest keyframe after the position one wants to jump to.
This patch sets the backward flag so that the player jumps to the closest keyframe before the resume point. (See CDVDDemuxFFmpeg::SeekTime, FFmpeg flag AVSEEK_FLAG_BACKWARD)

Instead of skipping a few seconds after resuming, the last couple of seconds will be repeated and the user will not miss content.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
Copy link
Member

left a comment

looks good

@Rechi

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

jenkins build this please

@ksooo

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

@FernetMenta should we backport this to Krypton? Seems low risk, but nice improvement.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

Looks like a trivial change on first glance but we rely heavily on ffmpeg here. Setting the backward flag on first seek is probably not the normal case. There are many different containers with their own seek functions. To be save all those needs to be tested with this change.

EDIT: I was referring to backport.

@dgam1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2017

@FernetMenta From what I can tell, every implementation of CDVDDemux's SeekTime() in xbmc/cores/VideoPlayer/DVDDemuxers/ ignores the backwards parameter, except the one in CDVDDemuxFFmpeg.cpp. Both in the master and the Krypton branch.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

@dgam1 I was not referring to implementations of CDVDDemux but to containers like mkv, avi, mpegts, etc. Every container (ffmpeg calls it a format) has its own way of seeking. In particular formats without an index like mpegts may do harder in finding a key frame before the seek pos.

Not long ago I fixed an issue in ffmpeg that made seeking in mkv containers stall up to 10 seconds. This just should underpin that seeking in general is not a trivial process and even a tiny change should not be underestimated.

@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Feb 21, 2017
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

let's first test this properly on master

@MartijnKaijser MartijnKaijser merged commit 755d7db into xbmc:master Feb 21, 2017
2 of 3 checks passed
2 of 3 checks passed
jenkins4kodi You are a failure. Fix the code and try again......
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ukmark62 ukmark62 referenced this pull request Jul 21, 2019
1 of 7 tasks complete
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.