Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Explicitly check for supported IAMF codecs parameter strings #4061

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Explicitly check for supported IAMF codecs parameter strings #4061

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
de2e7ed
Explicity check for supported IAMF codecs parameter strings
osagie98 Aug 28, 2024
a73ca31
Merge branch 'main' into narrow-iamf-mime-check
osagie98 Sep 9, 2024
267bb9c
Use stringstream
osagie98 Sep 9, 2024
dda0ac9
cleanup
osagie98 Sep 9, 2024
6ef41b2
Update SplitString implementation
osagie98 Sep 10, 2024
e2a89a8
Add extra test case in StringTest
osagie98 Sep 10, 2024
812b1ce
Merge branch 'main' into narrow-iamf-mime-check
osagie98 Sep 12, 2024
dd06e30
Add test cases
osagie98 Sep 16, 2024
60fd073
Remove sb version guards
osagie98 Sep 16, 2024
6de8c3c
Merge branch 'main' into narrow-iamf-mime-check
osagie98 Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions starboard/common/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ target(gtest_target_type, "common_test") {
"memory_test.cc",
"player_test.cc",
"socket_test.cc",
"string_test.cc",
"test_main.cc",
]
deps = [
Expand Down
18 changes: 18 additions & 0 deletions starboard/common/string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <stdio.h>
#endif
#include <cstring>
#include <sstream>
#include <string>
#include <vector>

Expand Down Expand Up @@ -103,6 +104,23 @@ static SB_C_FORCE_INLINE int strlcat(CHAR* dst, const CHAR* src, int dst_size) {
dst_length;
}

// Splits a string on a char delimiter. Returns an empty vector if the delimiter
// is not found
inline std::vector<std::string> SplitString(std::string& input,
char delimiter) {
std::vector<std::string> output;
if (input.find(delimiter) == std::string::npos) {
return output;
}

std::stringstream stream(input);
std::string token;
while (std::getline(stream, token, delimiter)) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

output.push_back(token);
}
return output;
}

} // namespace starboard

#endif // STARBOARD_COMMON_STRING_H_
41 changes: 41 additions & 0 deletions starboard/common/string_test.cc
View file
Open in desktop
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 {

Copy link
Contributor

@borongc borongc Sep 12, 2024 •
edited
Loading

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
92 changes: 91 additions & 1 deletion starboard/shared/starboard/media/codec_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <algorithm>
#include <cctype>
#include <sstream>
#include <string>

#include "starboard/common/log.h"
Expand Down Expand Up @@ -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 =
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Copy link
Contributor

@borongc borongc Sep 12, 2024 •
edited
Loading

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".

Copy link
Contributor Author

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.

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]);
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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];
Copy link
Contributor

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;

Copy link
Contributor Author

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.

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
Expand Down
5 changes: 5 additions & 0 deletions starboard/shared/starboard/media/codec_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#ifndef STARBOARD_SHARED_STARBOARD_MEDIA_CODEC_UTIL_H_
#define STARBOARD_SHARED_STARBOARD_MEDIA_CODEC_UTIL_H_

#include <string>
#include <vector>

#include "starboard/common/optional.h"
Expand Down Expand Up @@ -70,6 +71,10 @@ class VideoConfig {
SbMediaAudioCodec GetAudioCodecFromString(const char* codec,
const char* subtype);

#if SB_API_VERSION >= 15
bool IsIamfMimeType(std::string mime_type);
#endif // SB_API_VERSION >= 15

} // namespace media
} // namespace starboard
} // namespace shared
Expand Down
42 changes: 42 additions & 0 deletions starboard/shared/starboard/media/codec_util_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
Loading