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

[bug fix] odd dimensions cause encoder issues #95

Merged
merged 4 commits into from
Jun 8, 2021

Conversation

chrisgervang
Copy link
Collaborator

@chrisgervang chrisgervang commented Jun 7, 2021

Video dimensions cannot be odd in many video formats. Solution: Round scaling factors to the closest even factor to always get an even resolution in the final video. Any number multiplied by 2 is even.

The ratio is a scaling factor and uses float (e.g. 1.345), whereas the dimensions represent pixels and those should be int (e.g. 1080 or 720).

- video dimensions cannot be odd in MP4 and many other formats. Force an even scaling factor and dimension, which will always result in an even resolution in the final video.
@@ -54,7 +55,7 @@ export class StageMap extends Component {

_resizeVideo() {
const {width, dimension} = this.props;
this._setDevicePixelRatio(dimension.width / width);
this._setDevicePixelRatio(nearestEvenFloat(dimension.width / width));
Copy link

Choose a reason for hiding this comment

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

what's the reason behind using Float here and Int above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ratio is a scaling factor and uses float (e.g. 1.345), whereas the dimensions represent pixels and those should be int (e.g. 1080 or 720).

@@ -0,0 +1,7 @@
export const nearestEvenInt = num => 2 * Math.round(num / 2);
Copy link

Choose a reason for hiding this comment

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

what's the reason for diving num / 2 and then round and multiplying 2?

Copy link
Collaborator Author

@chrisgervang chrisgervang Jun 8, 2021

Choose a reason for hiding this comment

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

The function needs to return an even number, in this example we can see how this works:

Math.round(13) = 13
2 * Math.round(13 / 2) = 14

Any number multiplied by 2 is even.

@coveralls
Copy link

coveralls commented Jun 7, 2021

Pull Request Test Coverage Report for Build 919445446

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 919391618: 0.0%
Covered Lines: 2
Relevant Lines: 2

💛 - Coveralls

Comment on lines 3 to 7
export const nearestEvenFloat = num => {
num = num * 1000;
num = 2 * Math.round(num / 2);
return num / 1000;
};
Copy link

Choose a reason for hiding this comment

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

What's your thought on making this function takes in decimal place rather than making it fixed at 1000?

ex)
export const nearestEvenFloat = (num, decimalPlaces)

Copy link

Choose a reason for hiding this comment

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

we can also do something like
factorTen = Math.pow(10, decimalPlaces)

Copy link
Collaborator Author

@chrisgervang chrisgervang Jun 8, 2021

Choose a reason for hiding this comment

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

Oh I like that. That makes me think we can just make a nearestEven function and use 0 for the int case.

@chrisgervang chrisgervang merged commit 4b8c889 into visgl:master Jun 8, 2021
@chrisgervang chrisgervang deleted the chr/nearest-even branch June 8, 2021 23:04
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.

3 participants