Skip to content

Moved check_conf function from xeve_app implementation to xeve API#61

Merged
mpeg5 merged 3 commits intomasterfrom
dev-CheckCfgFunction2XeveAPI
Jun 21, 2022
Merged

Moved check_conf function from xeve_app implementation to xeve API#61
mpeg5 merged 3 commits intomasterfrom
dev-CheckCfgFunction2XeveAPI

Conversation

@dkozinski
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Collaborator

@dariusz-f dariusz-f left a comment

Choose a reason for hiding this comment

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

Looks good.
This change is suggested by FFmpeg community.

@dkozinski
Copy link
Copy Markdown
Collaborator Author

The idea behind moving implementation of check_conf function from the xeve library client to the xeve library implementation comes from ffmpeg communinty suggestions following code review of our libxeve wrapper implementation for ffmpeg project.
Our implementation uses function analogous to the check_conf used by the xeve_app reference implementation and it's practicaly exact copy of the check_conf function.
FFmpeg community suggested us moving this function to xeve library.
If the sequece of the checks and tests (from check_cfg function) must be done just right before using codec and it is essential for proper codec behaviour it will be good idea to provide the client such a function that will check whether the codec configuration is OK.
It will make the user free up from responsibility of providing its own implementation of check_conf function and what is more important release the user from the need of making it up to date evertytime when it is needed.

Copy link
Copy Markdown
Collaborator

@kpchoi kpchoi left a comment

Choose a reason for hiding this comment

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

Please find my comments.

inc/xeve.h Outdated
int XEVE_EXPORT xeve_config(XEVE id, int cfg, void * buf, int * size);
int XEVE_EXPORT xeve_param_default(XEVE_PARAM* param);
int XEVE_EXPORT xeve_param_ppt(XEVE_PARAM* param, int profile, int preset, int tune);
int XEVE_EXPORT xeve_check_conf(XEVE_CDSC* cdsc);
Copy link
Copy Markdown
Collaborator

@kpchoi kpchoi Jun 19, 2022

Choose a reason for hiding this comment

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

I'd like to suggest the followings;

  • change the "xeve_check_conf()" API name to "xeve_param_check()"
  • use "XEVE_PARAM* param" instead of "XEVE_CDSC* cdsc" for input argument, because it would be kinds of utility function for parameter setting.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Owner

@mpeg5 mpeg5 left a comment

Choose a reason for hiding this comment

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

LGTM

@mpeg5 mpeg5 merged commit 9efa6d6 into master Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants