Skip to content

C API 2.0: External source info #5872

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

Merged
merged 3 commits into from
Apr 10, 2025
Merged

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Apr 8, 2025

Category:

New feature (non-breaking change which adds functionality)

Description:

This PR adds the ability to query for external source descriptors that contain:

  • device
  • (optional) data type
  • (optional) ndim
  • (optional) layout

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: DALI-4240

mzient added 3 commits April 8, 2025 18:49
Signed-off-by: Michal Zientkiewicz <[email protected]>
Signed-off-by: Michal Zientkiewicz <[email protected]>
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient mzient force-pushed the c_api2_external_source_info branch from b594d9b to 8d11321 Compare April 8, 2025 16:49
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [26640097]: BUILD STARTED

@banasraf banasraf self-assigned this Apr 8, 2025
@NVIDIA NVIDIA deleted a comment from dali-automaton Apr 9, 2025
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [26640097]: BUILD PASSED

*
* @param pipeline [in] The pipeline
* @param out_input_desc [out] A pointer to the location where the descriptor is written.
* @param name [in] The name of the input whose descriptor to obtain.]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param name [in] The name of the input whose descriptor to obtain.]
* @param name [in] The name of the input whose descriptor to obtain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix in upcoming PR.

* @retval DALI_SUCCESS
* @retval DALI_ERROR_INVALID_OPERATION the pipeline wasn't built before the call
*/
DALI_API daliResult_t daliPipelineGetInputCount(daliPipeline_h pipeline, int *out_input_count);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DALI_API daliResult_t daliPipelineGetInputCount(daliPipeline_h pipeline, int *out_input_count);
DALI_API daliResult_t daliPipelineGetInputCount(daliPipeline_h pipeline, int *out_input_count);

The out_input seems a bit confusing name at first. Why not just input_count? Does the out_ prefix relate to the fact this is the output of this function?

Copy link
Member

@stiepan stiepan Apr 10, 2025

Choose a reason for hiding this comment

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

Or it rephrases the type of the argument? If that's not a convention of the API to mark output arguments (I don't recall we have such convention), I'd avoid alluding to the type of the desc class in the name and go with simpler input_count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I noticed this, too. It's used throughout the header this way and yes, all output parameters are prefixed with out_.

@mzient mzient merged commit 67bd7ba into NVIDIA:main Apr 10, 2025
6 checks passed
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.

4 participants