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.

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

Fix #819, Base-Enhanced Profile #828

Merged
merged 9 commits into from
Jun 17, 2024
Merged

Fix #819, Base-Enhanced Profile #828

merged 9 commits into from
Jun 17, 2024

Conversation

sunghee-hwang
Copy link
Collaborator

@sunghee-hwang sunghee-hwang commented May 28, 2024

Copy link
Collaborator

@felicialim felicialim left a comment

Choose a reason for hiding this comment

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

lgtm % editorial suggestions

index.bs Show resolved Hide resolved
index.bs Outdated
- They SHALL be able to reconstruct one [=Audio Element=].
- They MAY use [=demixing_info_parameter_data=] or [=default_demixing_info_parameter_data=] to do down-mixing.
When the [=primary_profile=] field is set to 0, the following constraints apply to the [=IA Sequence=]:
- There SHALL be at least one [=Mix Presentation OBU=] that complies with the simple profile, in other words, a [=Mix Presentation OBU=] for only one [=Audio Element=] that parsers, which comply with the simple profile, recognize.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this Mix Presentation constraint applies to all Profiles, what do you think about moving it to the common restrictions above? As an example:

There SHALL be at least one Mix Presentation OBU that complies with the conformance points of the primary profile set in the IA Sequence.

index.bs Outdated
- They SHALL be able to parse an [=IA Sequence=] with [=primary_profile=] = 2.
- They SHALL be able to handle up to 28 channels.
- One example is a mix with 3rd-order Ambisonics (16 channels) + 7.1.4ch (12 channels).
- They SHALL be able to reconstruct twenty eight [=Audio Element=]s.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "28" might be easier to read than "twenty eight"

Copy link
Collaborator Author

@sunghee-hwang sunghee-hwang left a comment

Choose a reason for hiding this comment

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

Implement reviewer's suggesetion

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
This section specifies the conformance points of the base-enhanced profile.

When the [=primary_profile=] field is set to 2, the following constraints apply to the [=IA Sequence=]:
- There SHALL be at least one [=Mix Presentation OBU=] for at most 28 [=Audio Element=]s that parsers, which comply with the base-enhanced profile, recognize.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To clarify the meaning of 28 channels or [=Audio Element=]s.

Suggested change
- There SHALL be at least one [=Mix Presentation OBU=] for at most 28 [=Audio Element=]s that parsers, which comply with the base-enhanced profile, recognize.
- There SHALL be at least one [=Mix Presentation OBU=] for at most 28 [=Audio Element=]s that parsers, which comply with the base-enhanced profile, recognize.
- If [=additional_profile=] is set less than or equal to 2, there SHALL be at most 28 channels, the sum of channels across all [=Audio Element=]s in the [=IA Sequence=] that parsers, which comply with the base-enhanced profile, recognize. Otherwise, there MAY be more 28 channels or 28 [=Audio Element=]s in the [=IA Sequence=].

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wanted to check my understanding.

Is the intention that if primary_profile = additional_profile = base-enhanced, then there is a max of 28 channels in the [=IA Sequence=]?

And if primary_profile = simple/base and additional_profile = base-enhanced, then there can be more than 28 channels in the [=IA Sequence=]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to check my understanding.

Is the intention that if primary_profile = additional_profile = base-enhanced, then there is a max of 28 channels in the [=IA Sequence=]?
Yes, it is

And if primary_profile = simple/base and additional_profile = base-enhanced, then there can be more than 28 channels in the [=IA Sequence=]?
Yes, it can

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. Just some minor editorial tweaks suggested below.

Suggested change
- There SHALL be at least one [=Mix Presentation OBU=] for at most 28 [=Audio Element=]s that parsers, which comply with the base-enhanced profile, recognize.
- There SHALL be at least one [=Mix Presentation OBU=] with at most 28 [=Audio Element=]s that parsers complying with the base-enhanced profile can recognize.
- If [=additional_profile=] is set to less than or equal to 2, there SHALL be at most 28 channels in total across all [=Audio Element=]s in the [=IA Sequence=] that parsers complying with the base-enhanced profile can recognize. Otherwise, there MAY be more than 28 channels or 28 [=Audio Element=]s in the [=IA Sequence=].```

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might be better to remove "can".

@sunghee-hwang sunghee-hwang requested a review from felicialim June 14, 2024 01:27
@tdaede tdaede merged commit 331a1cc into main Jun 17, 2024
@sunghee-hwang sunghee-hwang deleted the base-enhanced-profile branch July 2, 2024 02:30
@sunghee-hwang sunghee-hwang restored the base-enhanced-profile branch July 8, 2024 01:20
sunghee-hwang added a commit that referenced this pull request Jul 8, 2024
sunghee-hwang added a commit that referenced this pull request Jul 10, 2024
The Missed text for Base-Enhanced profile during merging PR #828
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants