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

Improve video constraints (width|height|aspectRatio|frameRate).(ideal|exact) and add (aspectRatio|frameRate).(ideal|exact) support #419

Closed
wants to merge 7 commits into from

Conversation

hthetiot
Copy link
Contributor

@hthetiot hthetiot commented Oct 26, 2019

  • implement getUserMedia constraints video.(width|height|aspectRatio|frameRate).(ideal|exact) and add video.(aspectRatio|frameRate).(ideal|exact) support
  • update PluginRTCVideoCaptureController aspectRatio and improve default constraints handling
  • Handle aspectRatio: 16/9, that was failing before due NSNumber to Float32 issue (e.g 1.7777777777777777 to 1.777778)
  • Add more debug for fail exact requirement

Testing

To test task/fix-video-constraints branch

cordova plugin remove cordova-plugin-iosrtc --verbose
cordova plugin add https://github.com/cordova-rtc/cordova-plugin-iosrtc#task/fix-video-constraints --verbose
cordova platform remove ios --no-save
cordova platform add ios --no-save

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 26, 2019

@menelike this should do it.
You cannot set default aspect ratio only like you did, otherwise this would fail.

width: {
    max: 1280,
    min: 640
},
height: {
    max: 720,
    min: 720
}

Feel free to fork this branch and tests.

@hthetiot
Copy link
Contributor Author

This will fail:

constraints: {
        video: {
          height: {
            ideal: 240,
            min: 180,
            max: 480
          }
        }
      }

…nstrainRangeValues to support height without with and handle partially ideal+min+max
@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 28, 2019

constraints: {
        video: {
          height: {
            ideal: 240,
            min: 180,
            max: 480
          }
        }
      }

This will works now, but ideal is not really implemented.

@hthetiot
Copy link
Contributor Author

@menelike Can you try this last changes, this would help a lot.

@menelike

This comment was marked as off-topic.

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 29, 2019

behavior of aspectRatio: 16/9 vs aspectRatio: {exact: 16/9} should be the same.
See Spec.

@menelike the spec say it, read it again.
I never said ideal I said exact

If 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 Author

We dont support resizeMode out of topic.

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 29, 2019

@menelike Please try the basic. (height|width|framerate|aspectRatio).(min|max|exact|ideal).
You keep changing the scope every issues, now you talking about resizeMode that not what we want do achive here, we want simple support for (height|width|framerate|aspectRatio).(min|max|exact|ideal) nothing else.

I'm starting to losing patience.

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 29, 2019

Every time you are in a issue you add stuff that complexify the feature request, add to the request.
Please test the basic so we can iterate, nothing else than the basics.

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 29, 2019

don't know why it switches between 715 and 716, to my understanding >= 640 && <= 720 should result in 640 and > 720 && <= 800 should result in 800. Though the spec is not clear if 720 should result in 640 or 800 - or I have missed that part.

Chrome do not follow spec, they still have 2014 spec for getUserMedia DO NOT compare with chrome. We not going to implement based on Chrome specially since it's not even clear why they do what they do.

@hthetiot
Copy link
Contributor Author

Ideal is simple, if you have a min/max and it match multiple sizes, it should pick the one that is ideal if available, that this PR will not support yet. This PR will only support min/max or exact and if ideal will be equivalent to min=max=ideal for now.

@hthetiot
Copy link
Contributor Author

I pushing this to 6.0.1 if it was tested properly could have been released on 6.0.0 but like usual it get out of topics....

@hthetiot hthetiot modified the milestones: 6.0.0, 6.0.1 Oct 29, 2019
@hthetiot hthetiot changed the title Task/fix video constraints Improve video constraints deo.(width|height|aspectRatio|frameRate).(ideal|exact) and add video.(aspectRatio|frameRate).(ideal|exact) support Oct 29, 2019
@hthetiot hthetiot changed the title Improve video constraints deo.(width|height|aspectRatio|frameRate).(ideal|exact) and add video.(aspectRatio|frameRate).(ideal|exact) support Improve video constraints (width|height|aspectRatio|frameRate).(ideal|exact) and add (aspectRatio|frameRate).(ideal|exact) support Oct 29, 2019
@menelike

This comment was marked as off-topic.

@menelike

This comment was marked as off-topic.

@menelike

This comment was marked as off-topic.

@hthetiot hthetiot modified the milestones: 6.0.1, 6.0.2, 6.0.x Oct 30, 2019
@menelike

This comment was marked as off-topic.

@hthetiot
Copy link
Contributor Author

Closed in favor of #451 451

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

Successfully merging this pull request may close these issues.

2 participants