Skip to content

Fix frame_rate on Windows#149

Draft
Giotino wants to merge 1 commit intol1npengtul:senpaifrom
Giotino:senpai
Draft

Fix frame_rate on Windows#149
Giotino wants to merge 1 commit intol1npengtul:senpaifrom
Giotino:senpai

Conversation

@Giotino
Copy link
Copy Markdown

@Giotino Giotino commented Oct 14, 2023

Comment on lines +972 to +975
if denominator != 1 {
numerator = 0;
}
numerator
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As fare as I understand the API https://learn.microsoft.com/en-us/windows/win32/medfound/mf-mt-frame-rate-attribute the numerator should be divided by the denominator to get the frame rate.

Suggested change
if denominator != 1 {
numerator = 0;
}
numerator
if denominator != 0 {
numerator / denominator
} else {
0
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, but as you can see here https://github.com/Giotino/nokhwa/blob/senpai/nokhwa-bindings-windows/src/lib.rs#L599-L636 Nokhwa doesn't support non integer framerates and simply reports 0 when there is one.
And, since the framerate is an integer, it's gonna report a wrong number if we use your approach, which is not to be ignored, but we should agree that framerate is not really supported by the device.

This was also written in my original issue (#144 (comment)):

Also nokhwa should keep in mind that there might be some framerates that are not X/1 (like 2997/1000), I think here we have 2 solutions that doesn't require to rewrite the frame_rate handling:

  1. Return an error when the framerate is not X/1
  2. "Transform" A/B -> X/1 rounding X (es. 2997/1000 -> 30/1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think a rounded framerate is better than no frame rate

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Another argument in favor of rounding them are common rates such as NTSC's 24000/1001(=23.976...), for ease of use often referred to as 24fps. The same goes for e.g. 59.94, the aforementioned 29.97, and any VFR source reporting the average over the last N seconds

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

0.11 supports non-integer frame rates as the new FrameRate struct is a rational fraction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants