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

[media] Create IamfMimeUtil #4061

Closed
wants to merge 20 commits into from
+420 −0
Closed

[media] Create IamfMimeUtil #4061

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 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
6c157b3
Merge branch 'main' into narrow-iamf-mime-check
osagie98 Sep 25, 2024
2d7d9a6
Create IamfMimeUtil
osagie98 Oct 1, 2024
0a6671d
Remove unused includes
osagie98 Oct 1, 2024
8d2173d
Remove most test asserts
osagie98 Oct 1, 2024
7c3dcf2
Consolidate string tests
osagie98 Oct 1, 2024
fa49cad
Const vector
osagie98 Oct 1, 2024
46d8d39
Remove IamfMimeUtil redundancies
osagie98 Oct 1, 2024
905668a
Re add StringSplit tests
osagie98 Oct 1, 2024
6eeccba
Remove extraneous parentheses
osagie98 Oct 1, 2024
e67695d
Remove IAMF CodecUtilTest
osagie98 Oct 2, 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
31 changes: 31 additions & 0 deletions starboard/common/string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,37 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider double checking if returning an empty vector when the delimiter is not found is a common practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It now returns the full string if non empty, like the Chromium method does.

inline std::vector<std::string> SplitString(std::string& input,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const std::string&

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

char delimiter) {
std::vector<std::string> output;
if (input.empty()) {
return output;
}

size_t start = 0;
while (start != std::string::npos) {
size_t end = input.find_first_of(delimiter, start);
std::string piece;

if (end == std::string::npos) {
if (output.empty()) {
break;
}
piece = input.substr(start);
start = std::string::npos;
} else {
piece = input.substr(start, end - start);
start = end + 1;
}

output.emplace_back(piece);
}

return output;
}

} // namespace starboard

#endif // STARBOARD_COMMON_STRING_H_
54 changes: 54 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,54 @@
// 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 {

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, ' ');
ASSERT_EQ(output.size(), vec.size());
for (int i = 0; i < vec.size(); ++i) {
ASSERT_EQ(output[i], vec[i]);
}

str = "iamf.001.001.Opus";
output = SplitString(str, '.');
vec = {"iamf", "001", "001", "Opus"};
ASSERT_EQ(output.size(), vec.size());
for (int i = 0; i < vec.size(); ++i) {
ASSERT_EQ(output[i], vec[i]);
}
}

TEST(StringTest, SplitStringEmpty) {
std::string str;
std::vector<std::string> output = SplitString(str, '.');
ASSERT_TRUE(output.empty());
}

} // namespace
} // namespace starboard
91 changes: 90 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 @@ -106,12 +107,100 @@ SbMediaAudioCodec GetAudioCodecFromString(const char* codec,
if (is_wav && strcmp(codec, "1") == 0) {
return kSbMediaAudioCodecPcm;
}
if (strcmp(codec, "iamf") == 0 || strncmp(codec, "iamf.", 5) == 0) {
if (strcmp(codec, "iamf") == 0 || IsIamfMimeType(codec)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't check the validity of the codec string in this function.

The best way to check the validity of the iamf codec string I can think of is to check it in SbMediaIsAudioSupported(). We can add an iamf_util, and check the validity of codec string while deciding if the sub codec can be supported.

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

return kSbMediaAudioCodecIamf;
}
return kSbMediaAudioCodecNone;
}

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;
}

// 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".
constexpr int kMaxIamfCodecIdLength = 22;

if (mime_type.size() > kMaxIamfCodecIdLength) {
return false;
}

std::vector<std::string> vec = SplitString(mime_type, '.');
// The mime type must be in 4 parts for all substreams other than AAC, which
// is 6 parts.
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;
}

} // namespace media
} // namespace starboard
} // namespace shared
Expand Down
3 changes: 3 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,8 @@ class VideoConfig {
SbMediaAudioCodec GetAudioCodecFromString(const char* codec,
const char* subtype);

bool IsIamfMimeType(std::string mime_type);

} // namespace media
} // namespace starboard
} // namespace shared
Expand Down
43 changes: 43 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,49 @@ TEST(CodecUtilTest, DoesNotParse1AsPcmForNonWavSubtypes) {
EXPECT_EQ(GetAudioCodecFromString("1", "webm"), kSbMediaAudioCodecNone);
}

TEST(CodecUtilTest, ParsesIamfCodec) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check if this makes use of IamfMimeUtil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, IamfMimeUtil is intended to be used in SbMediaIsAudioSupported() implementations.

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

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"));

EXPECT_FALSE(IsIamfMimeType("iamf.000.Opus"));
EXPECT_FALSE(IsIamfMimeType("iacb.000.000.Opus"));
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.mp4a.40.2.0"));
EXPECT_FALSE(IsIamfMimeType("iamf.256.000.Opus"));
EXPECT_FALSE(IsIamfMimeType("iamf.000.256.Opus"));
EXPECT_FALSE(IsIamfMimeType("iamf.000.000.mp4a.4.2."));
}

} // namespace
} // namespace media
} // namespace starboard
Expand Down
Loading