awslabs / amazon-kinesis-video-streams-webrtc-sdk-c

Amazon Kinesis Video Streams Webrtc SDK is for developers to install and customize realtime communication between devices and enable secure streaming of video, audio to Kinesis Video Streams.
https://awslabs.github.io/amazon-kinesis-video-streams-webrtc-sdk-c/group__PublicMemberFunctions.html
Apache License 2.0
1.02k stars 307 forks source link

Add status->string function #1127

Closed kev-the-dev closed 3 years ago

kev-the-dev commented 3 years ago

I have found it is difficult to make sense of the status codes returned by kinesis. AFAIK there is no provided function to convert these codes to a human-readable string. Even looking it up in the header file is difficult, as you have to perform the math backwards to determine what series of integer additions equals the hex code you got.

My proposal (which I'd like to work on if the maintainers approve):

zhuker commented 3 years ago

This has been discussed previously While i would personally support it, the library is mostly used in embedded cameras where binary size and memory footprint is of high value Therefore your PR is likely to be rejected :shrug:

zhuker commented 3 years ago

as a workaround when i need to quickly lookup an error code eg 0x5a000005 i open full text search in my ide and search for STATUS.*0005 you'd see a bunch of defines with a header like so:

 * WEBRTC ICE related codes. Values are derived from STATUS_ICE_BASE (0x5a000000)
 *  @ */
...
#define STATUS_ICE_CANDIDATE_STRING_IS_TCP                                 STATUS_ICE_BASE + 0x00000005
...

note STATUS_ICE_BASE 0x5a000000

disa6302 commented 3 years ago

@kev-the-dev ,

As @zhuker rightly pointed out, we have considered this in the past, but, we will not be looking into adding the conversion since it would increase the memory footprint. We have quite a lot of status codes that would bloat the binary size.

We do have a documentation work item to explain error codes (https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/issues/737), but there is not an ETA yet.

I am going to close this ticket out since we already have a documentation work item in our bucket.

kev-the-dev commented 3 years ago

Thanks for the additional context, the concern over memory footprint makes sense. Would you be open to storing this as an inline function in a separate, optionally included header file, so as to only impact memory for users who chose to use this function?