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][pvr] Use audio stream to determine start time of programs with no video #15596

Merged
merged 1 commit into from Mar 7, 2019

Conversation

Projects
None yet
4 participants
@djp952
Copy link
Contributor

commented Feb 23, 2019

Description

This is an attempt to provide a minimally invasive solution to #15563. When an MPEG-TS program with no video stream(s) is played live via a PVR client, the seek functionality fails to work properly. The player UI also shows crazy invalid seek values (like -9 hours instead of +10s seconds when seeking forward). I tracked this down to the DVDDemuxFFmpeg class never setting the start time member variable since there are no video streams.

I didn't want to muck around with the demuxer at all (I'd screw it up), so I'm attempting more of a workaround here as opposed to a true fix. My opinion is that determination of the start time may be better served at a different point in the demuxer logic, the IsVideoReady() function doesn't seem like the ideal place to do this.

That said, what I did here to solve my immediate problem was to add code to IsVideoReady() that triggers if no video streams were found and the start time is still unknown. In that case, check for an audio stream and use it to initialize the start time instead. I didn't want to change the result of the function, so it will still return true/false based on examination of the video stream(s) only.

One immediate concern I have with my proposed change is if there should be a secondary check of the audio stream properties other than just it being an audio stream. For example, when setting start time from a video stream, that stream must have a non-NULL codecpar->extradata.

Please let me know if a less hackish/workaroundish type of change will be proposed to better solve the issue, I can cancel this PR. Thanks!

Motivation and Context

This change, or something more comprehensive than this change, is required to allow proper seek functionality on live PVR MPEG-TS programs that do not have any associated video elementary streams. Without setting the start time indicator properly, the seek calculations are incorrect and the PVR will be asked to seek to invalid locations. Additionally this is causing a Player UI issue during seek, for example a +10 second seek attempt will show as a very large negative seek attempt, like -9 hours.

Related issue: #15563

How Has This Been Tested?

I tested via the PVR functions to ensure that there was no impact to normal audio/video programs, both Live and Recorded. Under all circumstances except audio-only MPEG-TS the start time was set at the existing point. For audio-only MPEG-TS the start time was set at the added fallback point.

I also ran through as many different types of MPEG-based media I have to try and ensure there were no side effects. MPEG, DVD, video-only MPEG, Blu-Ray, etc. While I found no problems I do not believe I have an exhaustive set of media available to test everything.

The target system was Windows 10, x64, using Kodi 18.1 built from the master branch.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

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
@ksooo

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@FernetMenta do you have an opinion on this proposed fix?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

I agree with @djp952. This is probably the best fix for the time being. ffmpeg's mpegts demuxer was not designed for live streams. That's the reason why all those patches are required. At some later time the file/class can be cleaned up.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Should any comments be added to the code indicating as such?

@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

I'll add a comment (and try to be terse). Would you guys prefer using words like "temporary" or just briefly describe the block of code for what it does? i.e. - "// If no video stream(s) have been detected attempt to set the start time from an available audio stream"

@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Comments added, happy to change them if they are not acceptable. Thank you!

@ksooo

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

I agree with @djp952. This is probably the best fix for the time being.

@FernetMenta I take this as "go ahead and merge"?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

@ksooo yes

@ksooo

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

@djp952 thank you for your contribution.

@ksooo ksooo merged commit 089d357 into xbmc:master Mar 7, 2019

1 check passed

default You're awesome. Have a cookie
Details

@ksooo ksooo added this to the Leia 18.2-rc1 milestone Mar 7, 2019

@djp952 djp952 deleted the djp952:audioonlympegts-starttime branch Apr 8, 2019

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