-
Notifications
You must be signed in to change notification settings - Fork 111
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
RSDK-1729 - change resolution constraints to be of type IntExact #1781
Conversation
|
if attrs.Width > 0 { | ||
constraint.Width = prop.IntExact(attrs.Width) | ||
} else { | ||
constraint.Width = prop.IntRanged{Min: 0, Ideal: 640, Max: 4096} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did the ideal width change from 800 to 640? Same question for height changing from 600 to 480.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more aligned with the "default driver" (first) proposed by v4l for most webcams. Also, in the fitnessDistance algorithm of mediadevices, only min and max will matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mediadevices uses Ideal
too in its fitnessDistance algorithm. It uses it when comparing one IntRanged
to another (see IntRanged.Compare). The fitnessDistance
method will compare IntRanged
s (a type of IntConstraint
) here.
All that being said I don't think it matters too much. This looks LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, making so it matches exactly to the specified resolution will finally fix the matching bug we've seen
change resolutions constraints to be IntExact instead of IntRanged.