Unexpected behavior (and some issues) in the baseline profile APIs

[f2bo]

I've recently been exploring extensibility of the CDI baseline profile and observe behavior that I find unexpected, at least, given my interpretation of the information found here and that I partially reproduce below.

Extensibility is addressed through a generic configuration mechanism which is used even for the CDI baseline profile. It is based on a structure containing a URI and optional parameter data. The URI is defined such that it ensures uniqueness and optionally points to documentation on how to interpret the parameter data. The format of the payload data is also dependent on the URI. Helper functions ease the process of creating and parsing the generic configuration structure for the CDI baseline profile.

The URIs used for the CDI baseline profile are:

https://cdi.elemental.com/specs/baseline-video
https://cdi.elemental.com/specs/baseline-audio
https://cdi.elemental.com/specs/baseline-ancillary-data

The documents at those URIs fully specify the various aspects of each media type including the parameter data and the in memory representation of payload data. These files also reside in CDI_SDK/doc/specs.

Since the media format details are specificated outside of the SDK, new formats (beyond the CDI baseline profile) can be added without changing the SDK. They can be publicly documented or they can remain private for situations where interoperability is not required.

My initial understanding is that a transmitter can, for example, define a custom video format by calling CdiAvmRegisterBaselineProfile with a media type of kCdiAvmVideo and at some point, use CdiAvmMakeBaselineConfiguration2 with a custom video configuration derived from CdiAvmBaselineConfigCommon to initialize an AVM generic configuration structure. This will result in the make_config_ptr function specified by the custom profile being called where it presumably needs to set the uri member of the configuration to something other than "https://cdi.elemental.com/specs/baseline-video".

On the receiver side, CdiAvmRegisterBaselineProfile must also be called in order to handle this custom video format so that it can convert the generic configuration structure back to the custom video configuration by calling CdiAvmParseBaselineConfiguration2. However, this currently fails with a status of kCdiStatusNonFatal as the function validates the uri member and only allows URIs for the built-in payload types.

aws-cdi-sdk/src/cdi/baseline_profile.c

Lines 285 to 287 in 89d21b0

	int key = CdiUtilityStringToEnumValue(avm_uri_strings, config_ptr->uri);
	CdiAvmBaselineConfigCommon common_config = { 0 };
	if (CDI_INVALID_ENUM_VALUE != key) {

This is unexpected. Setting the URI as https://cdi.elemental.com/specs/baseline-video instead does work, but this seems to go against what seems to be implied by the extensibility description above.

There's also a second aspect that isn't entirely clear. The uri member of the CdiAvmConfig structure is documented as follows. Note in particular the reference to "or other" when describing the payload types, implying that it should be possible to register a media type other than video, audio, or ancillary data.

The URI unambiguously specifies the type (audio, video, ancillary data, or other) of data comprising an AVM stream within an AVM connection. Typically, it will be a URL to a document that describes how to interpret the bytes of the enclosing structure's data member as well as how payload data is to be formatted.

On the other hand, the CdiAvmRegisterBaselineProfile must be called with a media_type parameter with one of the values in the CdiBaselineAvmPayloadType enumeration and it's unclear how to register a custom media type. For example, calling this function with a kCdiAvmNotBaseline media type isn't allowed and will return a result of kCdiStatusInvalidParameter whereas using a value not defined by CdiBaselineAvmPayloadType is "allowed" but, currently, as the parameter isn't validated, it will result in data corruption when the profile data is written beyond the boundaries of the baseline_profile_array.

From the above, it seems that the implementation still relies on many aspects of the non-extensible API and that it would have been more appropriate for the media_type parameter to have been a URI instead of a CdiBaselineAvmPayloadType.

Finally, there's a discrepancy between the behavior of CdiAvmKeyEnumToString (and probably other similar functions) and what is currently documented, where it states that it will use profile version 01.00 if the version_ptr parameter is NULL. The actual behavior is to return the first registered profile which will not necessarily be v1.00 if CdiAvmRegisterBaselineProfile has been called previously.

aws-cdi-sdk/include/cdi_baseline_profile_api.h

Lines 225 to 235 in 57ce52b

	/**
	* Function used to convert an enum value to a string.
	*
	* @param key_type Enum which indicates which key-value array to search for enum_value.
	* @param enum_value Value to convert to a string.
	* @param version_ptr Pointer to profile version to use. If NULL, profile version 01.00 is used.
	*
	* @return Pointer to returned string. If no match was found, NULL is returned.
	*/
	CDI_INTERFACE const char* CdiAvmKeyEnumToString(CdiAvmBaselineEnumStringKeyTypes key_type, int enum_value,
	const CdiAvmBaselineProfileVersion* version_ptr);

In fact, there seems to be a deeper issue in the manner in which the baseline profiles are set up since this occurs in the FindProfileVersion function where once InitializeBaselineProfiles is called, it sets the initialized flag.

aws-cdi-sdk/src/cdi/baseline_profile.c

Lines 154 to 162 in 57ce52b

	// No need to use lock if we have already completed initialization.
	if (!CdiOsAtomicLoad32(&initialized)) {
	CdiOsStaticMutexLock(mutex_lock);
	if (!CdiOsAtomicLoad32(&initialized)) {
	InitializeBaselineProfiles();
	CdiOsAtomicStore32(&initialized, true);
	}
	CdiOsStaticMutexUnlock(mutex_lock);
	}

Because CdiAvmRegisterBaselineProfile does NOT call FindProfileVersion unless the initialized flag has been set, it might register the specified profile as the first one resulting in behavior that can change depending on whether any other function that also depends on FindProfileVersion has been called previously. Moreover, the check for duplicate registrations and the call to FindProfileVersion itself is skipped when the baseline profiles have not been initialized allowing, for example, to register the same profile multiple times or even registering alternate v1.0 or v2.0 profiles that will override those provided by the SDK.

aws-cdi-sdk/src/cdi/baseline_profile.c

Lines 214 to 216 in 57ce52b

	if (initialized && NULL != FindProfileVersion(media_type, &version, false)) {
	ret = kCdiStatusDuplicateBaselineVersion;
	}