-
Notifications
You must be signed in to change notification settings - Fork 116
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
Explicitly check for supported IAMF codecs parameter strings #4061
base: main
Are you sure you want to change the base?
Conversation
return false; | ||
} | ||
|
||
constexpr int kMaxIamfCodecIdLength = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although addition is a cheap operation, should we consider directly put the total number here, and then comment how it is calculated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious if this is the most efficient way to split strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
namespace starboard { | ||
namespace { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
std::vector<std::string> vec = SplitString(mime_type, '.'); | ||
if (vec.size() != 4 && vec.size() != 6) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately switch statements can't work with strings, only values resolving to int.
kSbMediaAudioCodecIamf); | ||
} | ||
|
||
TEST(CodecUtilTest, IsIamfMimeType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to add cases where the length of resulting string vector is not 4, like iamf.xxx.Opus
, iamf.1xx.2yy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
b/356704307