[media] Create IamfMimeUtil #4061

osagie98 · 2024-08-28T21:20:51Z

b/356704307

borongc · 2024-09-10T15:33:15Z

starboard/shared/starboard/media/codec_util.cc

+    return false;
+  }
+
+  constexpr int kMaxIamfCodecIdLength =


Although addition is a cheap operation, should we consider directly put the total number here, and then comment how it is calculated?

borongc · 2024-09-10T15:56:47Z

starboard/common/string.h

+
+  std::stringstream stream(input);
+  std::string token;
+  while (std::getline(stream, token, delimiter)) {


Just curious if this is the most efficient way to split strings.

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.

starboard/common/string_test.cc

borongc · 2024-09-12T15:57:06Z

starboard/shared/starboard/media/codec_util.cc

+  }
+
+  std::vector<std::string> vec = SplitString(mime_type, '.');
+  if (vec.size() != 4 && vec.size() != 6) {


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

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.

borongc · 2024-09-12T16:08:25Z

starboard/shared/starboard/media/codec_util.cc

+  // The primary profile string should be three digits, and should be between 0
+  // and 255 inclusive.
+  int primary_profile;
+  std::stringstream stream(vec[1]);


vec[1] is std::string, should we consider to use std::stoi() to convert string to integer here?

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.

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.

borongc · 2024-09-12T16:14:19Z

starboard/shared/starboard/media/codec_util.cc

+  }
+
+  // The codec string should be one of "Opus", "mp4a", "fLaC", or "ipcm".
+  std::string codec = vec[3];


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;

Unfortunately switch statements can't work with strings, only values resolving to int.

borongc · 2024-09-12T16:22:38Z

starboard/shared/starboard/media/codec_util_test.cc

+            kSbMediaAudioCodecIamf);
+}
+
+TEST(CodecUtilTest, IsIamfMimeType) {


Consider to add cases where the length of resulting string vector is not 4, like iamf.xxx.Opus, iamf.1xx.2yy.

jasonzhangxx · 2024-09-25T20:27:22Z

starboard/shared/starboard/media/codec_util.cc

@@ -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)) {


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.

xiaomings · 2024-09-25T22:26:32Z

starboard/common/string.h

@@ -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


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

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

xiaomings · 2024-09-25T22:26:43Z

starboard/common/string.h

@@ -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
+inline std::vector<std::string> SplitString(std::string& input,


const std::string&

xiaomings · 2024-10-01T17:45:54Z

starboard/shared/starboard/media/codec_util_test.cc

@@ -263,6 +263,18 @@ TEST(CodecUtilTest, DoesNotParse1AsPcmForNonWavSubtypes) {
  EXPECT_EQ(GetAudioCodecFromString("1", "webm"), kSbMediaAudioCodecNone);
 }

+TEST(CodecUtilTest, ParsesIamfCodec) {


Just to double check if this makes use of IamfMimeUtil?

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

xiaomings · 2024-10-01T17:46:48Z

starboard/shared/starboard/media/iamf_util.h

+  kIamfSubstreamCodecMp4a,
+  kIamfSubstreamCodecFlac,
+  kIamfSubstreamCodecIpcm,
+  kIamfSubstreamCodecUnknown


Unless this is a map of spec defined somewhere, consider moving kIamfSubstreamCodecUnknown to the first, so when the value is zero initialized, it's initialized to kIamfSubstreamCodecUnknown.

xiaomings · 2024-10-01T17:47:13Z

starboard/shared/starboard/media/iamf_util.h

+    return profile_ <= kIamfProfileMax &&
+           substream_codec_ != kIamfSubstreamCodecUnknown;
+  }
+  int profile() const { return profile_; }


Add DCHECK(is_valid()), and the same to below.

xiaomings · 2024-10-01T17:49:14Z

starboard/shared/starboard/media/iamf_util.cc

+  if (mime_type.find("iamf") == std::string::npos) {
+    return;
+  }


This is probably redundant to the check in line 57? And if we do want to check, we should check that the return value is 0.

xiaomings · 2024-10-01T17:54:09Z

starboard/shared/starboard/media/iamf_util.cc

+
+  // The codec string should be one of "Opus", "mp4a", "fLaC", or "ipcm".
+  std::string codec = vec[3];
+  if (codec.size() != 4 || ((codec != "Opus") && (codec != "mp4a") &&


The check codec.size() != 4 is redundant.

xiaomings · 2024-10-01T17:55:04Z

starboard/shared/starboard/media/iamf_util.cc

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


Shall we just check vec[4] == '40' && vec[5] == '2'?

The test will be implemented in a separate PR youtube#4188

Adds a test for GetAudioCodecFromString() to parse the codecs parameter of an IAMF MIME. `GetAudioCodecFromString()` will more strictly parse for valid values of the IAMF codec param in a follow up #4061. b/356704307

osagie98 · 2024-10-09T19:22:36Z

This PR will be close. The 25.lts PR is here: #4244

osagie98 added 4 commits August 28, 2024 14:19

Explicity check for supported IAMF codecs parameter strings

de2e7ed

b/356704307

Merge branch 'main' into narrow-iamf-mime-check

a73ca31

Use stringstream

267bb9c

cleanup

dda0ac9

osagie98 marked this pull request as ready for review September 9, 2024 22:22

osagie98 requested review from jasonzhangxx, borongc and zhongqiliang September 9, 2024 22:23

osagie98 added the cp-25.lts.1+ Cherry Pick to the 25.lts.1+ branch label Sep 9, 2024

borongc reviewed Sep 10, 2024

View reviewed changes

osagie98 added 2 commits September 10, 2024 14:55

Update SplitString implementation

6ef41b2

Add extra test case in StringTest

e2a89a8

osagie98 requested a review from borongc September 10, 2024 22:00

osagie98 changed the title ~~Explicity check for supported IAMF codecs parameter strings~~ Explicitly check for supported IAMF codecs parameter strings Sep 10, 2024

borongc reviewed Sep 12, 2024

View reviewed changes

starboard/common/string_test.cc Show resolved Hide resolved

borongc reviewed Sep 12, 2024

View reviewed changes

osagie98 added 4 commits September 12, 2024 10:07

Merge branch 'main' into narrow-iamf-mime-check

812b1ce

Add test cases

dd06e30

Remove sb version guards

60fd073

Merge branch 'main' into narrow-iamf-mime-check

6de8c3c

osagie98 requested review from borongc and xiaomings September 16, 2024 21:28

Merge branch 'main' into narrow-iamf-mime-check

6c157b3

jasonzhangxx reviewed Sep 25, 2024

View reviewed changes

xiaomings reviewed Sep 25, 2024

View reviewed changes

Create IamfMimeUtil

2d7d9a6

osagie98 added 4 commits October 1, 2024 09:11

Remove unused includes

0a6671d

Remove most test asserts

8d2173d

Consolidate string tests

7c3dcf2

Const vector

fa49cad

osagie98 requested review from xiaomings and jasonzhangxx October 1, 2024 17:08

osagie98 changed the title ~~Explicitly check for supported IAMF codecs parameter strings~~ [media] Create IamfMimeUtil Oct 1, 2024

osagie98 changed the base branch from main to 25.lts.1+ October 1, 2024 17:09

osagie98 changed the base branch from 25.lts.1+ to main October 1, 2024 17:09

xiaomings reviewed Oct 1, 2024

View reviewed changes

osagie98 added 3 commits October 1, 2024 11:19

Remove IamfMimeUtil redundancies

46d8d39

Re add StringSplit tests

905668a

Remove extraneous parentheses

6eeccba

osagie98 requested a review from xiaomings October 1, 2024 18:32

Remove IAMF CodecUtilTest

e67695d

The test will be implemented in a separate PR youtube#4188

osagie98 changed the base branch from main to 25.lts.1+ October 3, 2024 18:44

osagie98 mentioned this pull request Oct 3, 2024

[media] Add codec util test for IAMF #4188

Merged

osagie98 changed the base branch from 25.lts.1+ to main October 9, 2024 18:34

osagie98 marked this pull request as draft October 9, 2024 19:22

osagie98 closed this Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[media] Create IamfMimeUtil #4061

[media] Create IamfMimeUtil #4061

osagie98 commented Aug 28, 2024

borongc Sep 10, 2024

osagie98 Sep 10, 2024

borongc Sep 10, 2024

osagie98 Sep 10, 2024

borongc Sep 12, 2024 •

edited

Loading

osagie98 Sep 16, 2024

borongc Sep 12, 2024

borongc Sep 12, 2024

osagie98 Sep 12, 2024

borongc Sep 12, 2024

osagie98 Sep 16, 2024

borongc Sep 12, 2024

osagie98 Sep 16, 2024

jasonzhangxx Sep 25, 2024

osagie98 Oct 1, 2024

xiaomings Sep 25, 2024

osagie98 Oct 1, 2024

xiaomings Sep 25, 2024

osagie98 Oct 1, 2024

xiaomings Oct 1, 2024

osagie98 Oct 1, 2024

xiaomings Oct 1, 2024

osagie98 Oct 1, 2024

xiaomings Oct 1, 2024

osagie98 Oct 1, 2024

xiaomings Oct 1, 2024

osagie98 Oct 1, 2024

xiaomings Oct 1, 2024

osagie98 Oct 1, 2024

xiaomings Oct 1, 2024

osagie98 Oct 1, 2024

osagie98 commented Oct 9, 2024

[media] Create IamfMimeUtil #4061

[media] Create IamfMimeUtil #4061

Conversation

osagie98 commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borongc Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osagie98 commented Oct 9, 2024

borongc Sep 12, 2024 •

edited

Loading