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 aspect ratio #418

Conversation

menelike
Copy link
Contributor

@menelike menelike commented Oct 26, 2019

This PR fixes:

  • aspectRatio (height is not set based on width and aspect ratio where 4/3 is the default aspect ratio)
  • determination of format (height, width and framerates)

Todo

  • getConstrainRangeValues needs to work well with both Int64 and Float32 (since aspectRatio is passed as NSNumber => Float32)

This has been tested in conjunction with #416 and #407 with janus-gateway as the backend, an iPhone XR with iOS 13.1.2 and ios-cordova 4.5.5 (downgraded manually) and H264 🚀

@menelike

This comment was marked as off-topic.

@hthetiot
Copy link
Contributor

@menelike Can you for once in a issue or PR do not mix many topics in one.
Here you do half a thing and talk about 4 others, that really annoying and more noisy than anything else. Finish what you start before talking about somehting else.

You have many thing right in your change here but the problem is because you talk about many topic instead of focusing your PR goal, you become more noisy than usefull here. Focus and do one thing at the time and create dedicated issues instead of talking about many things for nothing. I would really not like to work with you directly, you need to finish something before talking about another one.

I will reply to your mixed topics but be aware next time, I will close and ignore your comment if you continue to mix topics and dont get more clean, you wasted a lot of my time with this kind of comments so fare. create dedicated issue for go fork else where

  1. Out of topic Forget TypeScript that for people that dont understand Javascript prototype, iosRTC will never be TypeScript as long as I maintain it. Dont bring that topic again , I'm tired of people that use TypeScript where it's not needed. If you dont understand Javascript that you problem not mine, also TypeScript is fake it get transpiled to JavaScript and they dont even have real Class it's fake and use prototype and they dont even have super beside constructor cause they too stupid to make it.

  2. Out of topic jshint is ok, I dont care about been fasionable and use new lint shit just because it's cool. Jshint does the job, if you want ESLint do a PR. For Swift, Xcode does the lint already.

  3. "The next step would be to support ideal and exact values on all possible constraints"
    ideal and exact is implemented you need to look at isConstrainRangeRequired you clearly did not understood the way it works currently. You are right that it can be improved.

  4. Please Use Ident tab for swift, read you PR diff and you will see you change is unreadable simply because you did not respect indentation style.

  5. "this part cries for unit tests", "talk, talk talk", do dont talk.

  6. I dont review half baked PR, make a PR once your are done, and do atomic changes not many at the same time. I will do seprate PR with some of you changes because making many things at one half lead no were.

The hardest part in our industry is to finish what we start, You are not going to succeed if you dont focus.

@hthetiot hthetiot closed this Oct 26, 2019
@menelike

This comment was marked as off-topic.

@hthetiot
Copy link
Contributor

hthetiot commented Oct 26, 2019

behavior of aspectRatio: 16/9 vs aspectRatio: {exact: 16/9} should be the same.
See Spec. Before implementing the Javascript the swift needed to handle more anyway.

iIf the constraint is required (constraintValue either contains one or more members named 'min', 'max', or 'exact', or is itself a bare value and bare values are to be treated as 'exact'), and the settings dictionary's value for the constraint does not satisfy the constraint, the fitness distance is positive infinity.

https://www.w3.org/TR/mediacapture-streams/#media-track-constraints

@hthetiot
Copy link
Contributor

hthetiot commented Oct 26, 2019

The max|min video height/width fix you made has been backported here: 26450c9

While an implementation for aspect ratio with more cases handling and real typing is available here #419

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.

2 participants