-
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?
Changes from 4 commits
de2e7ed
a73ca31
267bb9c
dda0ac9
6ef41b2
e2a89a8
812b1ce
dd06e30
60fd073
6de8c3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Copyright 2024 The Cobalt Authors. All Rights Reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
#include <string> | ||
#include <vector> | ||
|
||
#include "starboard/common/string.h" | ||
|
||
#include "testing/gtest/include/gtest/gtest.h" | ||
|
||
namespace starboard { | ||
namespace { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider to add more test cases, such as As we will use this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
TEST(StringTest, SplitString) { | ||
std::string str = "The quick brown fox jumps over the lazy dog"; | ||
|
||
std::vector<std::string> output = SplitString(str, '.'); | ||
EXPECT_TRUE(output.empty()); | ||
|
||
std::vector<std::string> vec = {"The", "quick", "brown", "fox", "jumps", | ||
"over", "the", "lazy", "dog"}; | ||
output = SplitString(str, ' '); | ||
EXPECT_EQ(output.size(), vec.size()); | ||
for (int i = 0; i < vec.size(); ++i) { | ||
EXPECT_EQ(output[i], vec[i]); | ||
} | ||
} | ||
|
||
} // namespace | ||
} // namespace starboard |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
|
||
#include <algorithm> | ||
#include <cctype> | ||
#include <sstream> | ||
#include <string> | ||
|
||
#include "starboard/common/log.h" | ||
|
@@ -107,13 +108,102 @@ SbMediaAudioCodec GetAudioCodecFromString(const char* codec, | |
return kSbMediaAudioCodecPcm; | ||
} | ||
#if SB_API_VERSION >= 15 | ||
if (strcmp(codec, "iamf") == 0 || strncmp(codec, "iamf.", 5) == 0) { | ||
if (strcmp(codec, "iamf") == 0 || IsIamfMimeType(codec)) { | ||
return kSbMediaAudioCodecIamf; | ||
} | ||
#endif // SB_API_VERSION >= 15 | ||
return kSbMediaAudioCodecNone; | ||
} | ||
|
||
#if SB_API_VERSION >= 15 | ||
bool IsIamfMimeType(std::string mime_type) { | ||
// Reference: Immersive Audio Model and Formats; | ||
// v1.0.0 | ||
// 6.3. Codecs Parameter String | ||
// (https://aomediacodec.github.io/iamf/v1.0.0-errata.html#codecsparameter) | ||
if (mime_type.find("iamf") == std::string::npos) { | ||
return false; | ||
} | ||
|
||
constexpr int kMaxIamfCodecIdLength = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
4 // FOURCC string "iamf". | ||
+ 1 // delimiting period. | ||
+ 3 // primary_profile as 3 digit string. | ||
+ 1 // delimiting period. | ||
+ 3 // additional_profile as 3 digit string. | ||
+ 1 // delimiting period. | ||
+ 9; // The remaining string is one of | ||
// "Opus", "mp4a.40.2", "fLaC", or "ipcm". | ||
|
||
if (mime_type.size() > kMaxIamfCodecIdLength) { | ||
return false; | ||
} | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should specify why If the format of IAMF is always So we can get rid of checking There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
return false; | ||
} | ||
|
||
if (vec[0] != "iamf") { | ||
return false; | ||
} | ||
|
||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. vec[1] is std::string, should we consider to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue with |
||
stream >> primary_profile; | ||
if (stream.fail() || vec[1].size() != 3 || primary_profile > 255) { | ||
return false; | ||
} | ||
|
||
// The additional profile string should be three digits, and should be between | ||
// 0 and 255 inclusive. | ||
stream = std::stringstream(vec[2]); | ||
int additional_profile; | ||
stream >> additional_profile; | ||
if (stream.fail() || vec[2].size() != 3 || additional_profile > 255) { | ||
return false; | ||
} | ||
|
||
// 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 commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
if (codec.size() != 4 || ((codec != "Opus") && (codec != "mp4a") && | ||
(codec != "fLaC") && (codec != "ipcm"))) { | ||
return false; | ||
} | ||
|
||
// Only IAMF codec parameter strings with "mp4a" should be greater than 4 | ||
// elements. | ||
if (codec == "mp4a") { | ||
if (vec.size() != 6) { | ||
return false; | ||
} | ||
|
||
// The fields following "mp4a" should be "40" and "2" to signal AAC-LC. | ||
stream = std::stringstream(vec[4]); | ||
int object_type_indication; | ||
stream >> object_type_indication; | ||
if (stream.fail() || vec[4].size() != 2 || object_type_indication != 40) { | ||
return false; | ||
} | ||
|
||
stream = std::stringstream(vec[5]); | ||
int audio_object_type; | ||
stream >> audio_object_type; | ||
if (stream.fail() || vec[5].size() != 1 || audio_object_type != 2) { | ||
return false; | ||
} | ||
} else { | ||
if (vec.size() > 4) { | ||
return false; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
#endif // SB_API_VERSION >= 15 | ||
|
||
} // namespace media | ||
} // namespace starboard | ||
} // namespace shared | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -263,6 +263,48 @@ TEST(CodecUtilTest, DoesNotParse1AsPcmForNonWavSubtypes) { | |
EXPECT_EQ(GetAudioCodecFromString("1", "webm"), kSbMediaAudioCodecNone); | ||
} | ||
|
||
TEST(CodecUtilTest, ParsesIamfCodec) { | ||
#if SB_API_VERSION < 15 | ||
GTEST_SKIP() << "IAMF is unsupported on Starboard " << SB_API_VERSION; | ||
#endif // SB_API_VERSION < 15 | ||
EXPECT_EQ(GetAudioCodecFromString("iamf", ""), kSbMediaAudioCodecIamf); | ||
EXPECT_EQ(GetAudioCodecFromString("iamf.000.000.Opus", ""), | ||
kSbMediaAudioCodecIamf); | ||
EXPECT_EQ(GetAudioCodecFromString("iamf.000.000.mp4a.40.2", ""), | ||
kSbMediaAudioCodecIamf); | ||
EXPECT_EQ(GetAudioCodecFromString("iamf.000.000.fLaC", ""), | ||
kSbMediaAudioCodecIamf); | ||
EXPECT_EQ(GetAudioCodecFromString("iamf.000.000.ipcm", ""), | ||
kSbMediaAudioCodecIamf); | ||
} | ||
|
||
TEST(CodecUtilTest, IsIamfMimeType) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
#if SB_API_VERSION < 15 | ||
GTEST_SKIP() << "IAMF is unsupported on Starboard " << SB_API_VERSION; | ||
#endif // SB_API_VERSION < 15 | ||
EXPECT_TRUE(IsIamfMimeType("iamf.000.000.Opus")); | ||
EXPECT_TRUE(IsIamfMimeType("iamf.255.255.Opus")); | ||
EXPECT_TRUE(IsIamfMimeType("iamf.000.000.mp4a.40.2")); | ||
EXPECT_TRUE(IsIamfMimeType("iamf.000.000.fLaC")); | ||
EXPECT_TRUE(IsIamfMimeType("iamf.000.000.ipcm")); | ||
|
||
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.Opu")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf,000.000.Opus")); | ||
EXPECT_FALSE(IsIamfMimeType("Iamf.000.000.Opus")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.0pus")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf.xxx.yyy.Opus")); | ||
|
||
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.mp4a.40.3")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.flac")); | ||
|
||
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.fLaC.40.2")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.mp4a.40.30")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.mp4a.40.20")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf.000.00.Opus")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf.999.999.Opus")); | ||
EXPECT_FALSE(IsIamfMimeType("iamf.256.256.Opus")); | ||
} | ||
|
||
} // namespace | ||
} // namespace media | ||
} // namespace starboard | ||
|
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.