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

fix: improve accuracy of package framerate detection #653

Conversation

from-the-river-to-the-sea
Copy link
Contributor

@from-the-river-to-the-sea from-the-river-to-the-sea commented Jan 7, 2022

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

bug fix

  • What is the current behavior? (You can also link to an open issue here)

Sofie sometimes detects the framerate of video packages incorrectly, particularly in the case of progressive scan files. This is because buildPackageFormatString looks at codec_time_base to determine a video's framerate. In certain video formats (such as XAVC MXF and H.264 MP4), codec_time_base is often double that of the video's actual framerate. Both ffmpeg and DaVinci Resolve export files which have this same characteristic. At first glance, it might seem that codec_time_base is counting fields instead of frames, but this also happens with progressive scan files. Beyond that, there are some progressive scan files I've encountered which have codec_time_base values like 499/50000, despite having an r_frame_rate of 50/1.

This discrepancy can result in false warnings in the rundown view where Sofie says that a package has the wrong media format, even though the package is actually of the correct format (or vice versa).

  • What is the new behavior (if this is a feature change)?

In this PR, buildPackageFormatString has been changed to look at r_frame_rate instead of codec_time_base when determining a package's framerate.

  • Other information:

buildFormatString has not been changed, as it doesn't seem to have access to r_frame_rate data. I'm not sure how much we care about this, seeing as buildFormatString seems to be for the older Media Manager and doesn't seem to be part of the Package Manager data flow.

Also of note is that this will cause interlaced packages to report their frames per second instead of their fields per second. If this is not desired, a conditional statement that checks deepScan.field_order could be added which retains the existing behavior for interlaced content. Such a change might look like this instead:

if (stream.r_frame_rate) {
	const formattedFramerate = /(\d+)\/(\d+)/.exec(stream.r_frame_rate) as RegExpExecArray
	let fps = Number(formattedFramerate[1]) / Number(formattedFramerate[2])
	if (
		deepScan.field_order === PackageInfo.FieldOrder.TFF ||
		deepScan.field_order === PackageInfo.FieldOrder.BFF
	) {
		fps *= 2
	}
	fps = Math.floor(fps * 100 * 100) / 100
	format += fps
}

@nytamin nytamin added Contribution from SuperFly.tv Contributions sponsored by SuperFly.tv 🐛🔨 bugfix This is a fix for an issue labels Jan 7, 2022
@nytamin nytamin requested a review from a team January 7, 2022 14:07
Copy link
Member

@jstarpl jstarpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It definitely seems like r_frame_rate is the "frames per second" (frames, not fields!) value, while what codec_time_base means can vary. I think the use of codec_time_base may have been used to facilitate the non-standard use of format monikers like "1080i5000", where the 5000 refers to the sampling rate rather than frame rate.

@Julusian Julusian changed the base branch from release39 to release40 February 3, 2022 16:01
@Julusian Julusian merged commit ad7926c into nrkno:release40 Feb 3, 2022
@from-the-river-to-the-sea from-the-river-to-the-sea deleted the fix/more-accurate-package-framerate-detection branch February 3, 2022 17:19
@Julusian Julusian mentioned this pull request Feb 9, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛🔨 bugfix This is a fix for an issue Contribution from SuperFly.tv Contributions sponsored by SuperFly.tv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants