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] Double the initial refresh rate for interlaced streams #24121

Merged
merged 1 commit into from Dec 9, 2023

Conversation

CrystalP
Copy link
Contributor

@CrystalP CrystalP commented Nov 20, 2023

Description

Switch resolution/refresh rate only once when starting the playback of interlaced video streams, instead of twice with the resulting black frames and audio drop out / pitch change.

For example, this stream has the top field first flag and is detected as interlaced by ffmpeg analysis:

Stream #0:0(eng): Video: mpeg2video (Main), yuv420p(tv, top first), 720x576 [SAR 16:15 DAR 4:3], 25 fps, 25 tbr, 1k tbn (default) (forced)

Current behavior:
Initial switch to a resolution with 25Hz refresh rate, followed a few seconds later by a switch to 50Hz.
Desired behavior:
Initial switch to 50Hz. No additional resolution changes.

To accomplish that, propagate the field order information from the ffmpeg analysis results to the hints used when a stream is opened, so that VideoPlayer can double the initial refresh rate for interlaced streams.

.ts stream handling in demuxer is different to enable fast switching for PVR addons and has a complicated history (see #5487 and #3590). The consequence is that the ffmpeg analysis results are lost and this PR is unable to propagate the interlaced flags and to remove the additional resolution change.
#19983 added an exception for hevc which doesn't seem to have disturbed PVR (likely because they only stream H264?)

Addressing that / revisiting .ts stream opening is beyond the scope of this PR and close to a release is not a good time for that kind of risky changes anyway.

Motivation and context

Unnecessary resolution switch for streams detected as interlaced.
Noticed personally and was brought up in forum thread https://forum.kodi.tv/showthread.php?tid=375049

How has this been tested?

Used the sample provided by user in the forum thread.
Regression testing with other samples that are detected as unknown or progressive field order.

What is the effect on users?

Remove extra resolution change when starting playback of interlaced streams.
Except .ts

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)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • 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

@CrystalP CrystalP added Type: Improvement non-breaking change which improves existing functionality Component: Players For changed player parts within "./xbmc/cores" v21 Omega labels Nov 20, 2023
@CrystalP CrystalP added this to the Omega 21.0 Beta 2 milestone Nov 20, 2023
@CrystalP CrystalP force-pushed the double-interlaced-refresh-rate branch from 4589cfd to 92372b6 Compare November 20, 2023 16:12
@CrystalP CrystalP changed the title [VideoPlayer] Use demux interlaced flags to double initial refresh rate [VideoPlayer] Double the initial refresh rate for interlaced streams Nov 20, 2023
@smp79
Copy link
Contributor

smp79 commented Nov 21, 2023

It didn't fix the double resolution change for me.

debug log

@CrystalP
Copy link
Contributor Author

It didn't fix the double resolution change for me.

Thanks for the feedback and the log, I also noticed an issue on a .ts file and there is special handling of that type by Kodi's demuxer which seems to lose the field order information. I'll take a closer look, but it's likely outside the scope of this change and there likely were reasons it was done that way.

When starting the playback of an interlaced stream, instead of switching to the probed refresh rate,
go directly to 2x the probed refresh rate as videoplayer would otherwise switch to 2x refresh rate
a few seconds later anyway, resulting in some black frames while the screen adjusts and audio drop out or a pitch change.

Does not work for ts streams because of special opening by demuxer, which loses most of the probed data.
Addressing that is beyond the scope of this change.
@CrystalP CrystalP force-pushed the double-interlaced-refresh-rate branch from 92372b6 to d3903fb Compare December 9, 2023 03:49
@CrystalP
Copy link
Contributor Author

CrystalP commented Dec 9, 2023

I looked closer at the .ts behavior and it's the consequence of a change for fast channel switching of PVR addons. There is some complicated history available in #5487 and #3590 and I'm not touching that with a 10 foot pole close to release.

Squashed the commits, modified the commit messages a bit, I think it's ready for merge.

@CrystalP
Copy link
Contributor Author

CrystalP commented Dec 9, 2023

Jenkins build this please

@fuzzard fuzzard merged commit 7b3a9fb into xbmc:master Dec 9, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Players For changed player parts within "./xbmc/cores" Type: Improvement non-breaking change which improves existing functionality v21 Omega
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants