Explicitly check for supported IAMF codecs parameter strings #4061
Conversation
| return false; | ||
| } | ||
|
|
||
| constexpr int kMaxIamfCodecIdLength = |
There was a problem hiding this comment.
Although addition is a cheap operation, should we consider directly put the total number here, and then comment how it is calculated?
starboard/common/string.h
Outdated
|
|
||
| std::stringstream stream(input); | ||
| std::string token; | ||
| while (std::getline(stream, token, delimiter)) { |
There was a problem hiding this comment.
Just curious if this is the most efficient way to split strings.
There was a problem hiding this comment.
Not sure if it's the most efficient, but I found it most readable. I replaced it with an implementation similar to Chromium's SplitString(), already in use in media code, so I think we can be confident it's efficient enough.
osagie98
changed the title
|
|
||
| namespace starboard { | ||
| namespace { | ||
|
|
There was a problem hiding this comment.
Consider to add more test cases, such as EmptyString. Refer to https://source.chromium.org/chromium/chromium/src/+/main:base/strings/string_split_unittest.cc.
As we will use this SplitString() to parse our MIME string, we should consider to include example MIME strings (especially IAMF), e.g., iamf.000.000.mp4a.40.2, in our test cases.
| } | ||
|
|
||
| std::vector<std::string> vec = SplitString(mime_type, '.'); | ||
| if (vec.size() != 4 && vec.size() != 6) { |
There was a problem hiding this comment.
Maybe we should specify why 4 and 6 here as a comment. Same for other places.
If the format of IAMF is always iamf.<primary_profile>.<additional_profile>.<audio_format>, then we can consider make SplitString() as a special case, such as std::vector<std::string> SplitString(std::string& input, char delimiter, int num_fields=-1), so if num_fields=4, this is used to parse iamf MIME strings.
So we can get rid of checking vec.size() != 6 here, and check vec.size() != 4 || vec[0] != "iamf".
There was a problem hiding this comment.
Added the comment. Seems that adding a special case is pretty specific to this use case, I'm not sure if it'd see any use outside of this function to justify adding it in. Happy to discuss it.
| // The primary profile string should be three digits, and should be between 0 | ||
| // and 255 inclusive. | ||
| int primary_profile; | ||
| std::stringstream stream(vec[1]); |
There was a problem hiding this comment.
vec[1] is std::string, should we consider to use std::stoi() to convert string to integer here?
There was a problem hiding this comment.
Also, std::isdigit() can check if each character is a digit or not. Then, make this a helper function, so we don't need the similar codes in all other places.
There was a problem hiding this comment.
The issue with std::stoi() is that it throws an exception if it can't convert the field to an int. std::isdigit() will help with that but I wonder if it's much more efficient than using stream.
| } | ||
|
|
||
| // The codec string should be one of "Opus", "mp4a", "fLaC", or "ipcm". | ||
| std::string codec = vec[3]; |
There was a problem hiding this comment.
Maybe consider to refactor the following code to switch-case, so it is more readable.
For example:
switch(codec) {
case "Opus":
case "fLaC":
case "ipcm":
return true;
default:
// handle "mp4a.40.2" and check its format here
// return true if it meets
}
return false;
There was a problem hiding this comment.
Unfortunately switch statements can't work with strings, only values resolving to int.
| kSbMediaAudioCodecIamf); | ||
| } | ||
|
|
||
| TEST(CodecUtilTest, IsIamfMimeType) { |
There was a problem hiding this comment.
Consider to add cases where the length of resulting string vector is not 4, like iamf.xxx.Opus, iamf.1xx.2yy.
b/356704307