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

getVideoEncoderConfigurationOptions does not follow the specs #113

Closed
bl0ggy opened this issue Jul 18, 2019 · 4 comments
Closed

getVideoEncoderConfigurationOptions does not follow the specs #113

bl0ggy opened this issue Jul 18, 2019 · 4 comments

Comments

@bl0ggy
Copy link
Contributor

bl0ggy commented Jul 18, 2019

As stated in ONVIF-Media-Service-Spec.pdf chapter 5.5.4 :

If a configuration token is provided, the device shall return the options compatible with that
configuration. If a media profile token is specified, the device shall return the options
compatible with that media profile. If both a media profile token and a configuration token are
specified, the device shall return the options compatible with both that media profile and that
configuration. If no tokens are specified, the options shall be considered generic for the
device.

the token should be optional, not forced like in Cam.prototype.getVideoEncoderConfigurationOptions in media.js (line 287 at the moment I write this issue) :

if (this.videoEncoderConfigurations && this.videoEncoderConfigurations[0]) {
    token = this.videoEncoderConfigurations[0].$.token;
} else {
    return callback(new Error('No video encoder configuration token is present!'));
}

So there are currently 3 issues here :

  1. We cannot get generic video encoder configuration options (which is a list of all supported options)
  2. We cannot get video encoder configurations options with a media profile token
  3. People expecting to receive generic video encore configuration options by omitting the token right after a connection will get back an error as this does not have videoEncoderConfigurations before we call getVideoEncoderConfigurations function

EDIT: the specifications link was for v1706 (Jun 2017), I updated it to the latest one, which has the same specifications for this issue.
EDIT2: the specifications link was for Media2, which is different from Media, but the specifications are still the same for this issue.

bl0ggy added a commit to bl0ggy/onvif that referenced this issue Jul 18, 2019
…onOptions

Takes either a video encoder configuration token or a media profile token or both or none.
Do not force using token.
@bl0ggy
Copy link
Contributor Author

bl0ggy commented Jul 18, 2019

I just forked your repo and committed a fix.
Would you mind checking if that's correct for you ?
I have not created/updated any test as I do not know how coffee works.

@agsh
Copy link
Owner

agsh commented Jul 19, 2019

Hi, I've looked at your commit, it seems fine. You can make a pull request to see linter errors.
Sorry, I don't have much time to write the tests. Also I haven't got any device to test. So I think, it is normal not to write the tests for now, or to rewrite it in js sometime. Nobody loves coffeescript now)

@bl0ggy
Copy link
Contributor Author

bl0ggy commented Jul 23, 2019

May I know, are you alerted when I add a new commit or post a comment on the pull request ?
In case you are not, I pushed 2 more commits about tests and posted one comment because I don't know what causes the fail on travis and how to debug it :/

@chriswiggins
Copy link
Collaborator

Closing this in favour of the pull request

agsh added a commit that referenced this issue Feb 18, 2020
Issue #113 : follow up ONVIF specs for getVideoEncoderConfigurationOp…
This issue was closed.
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

No branches or pull requests

3 participants