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

Replaced ATMOS with IAB terminology #30

Closed
wants to merge 1 commit into from

Conversation

palemieux
Copy link
Contributor

Closes #15

only tested on cmake builds

@palemieux palemieux requested a review from jhursty March 11, 2020 03:48
@jhursty
Copy link
Contributor

jhursty commented Mar 11, 2020

I think the patch goes beyond addressing the labeling issue by also changing the API. These changes will break compilation on all projects that rely on the library for IAB (nee Atmos) file accessors.

Speaking for my own work, this will create an epoch in source control beyond which future work must rely on the new API. sed is my friend though, and the disruption would not be too painful if there is general agreement to make the changes. (@palemieux, do you have a sed script you want to share?)

OTOH, changing just the symbol and label strings will address #15 as well. Issues stemming from the presence of antiquated symbols in the API can be addressed by documentation.

I'd like more input from the community before taking such a big step. I'm going to hold this request for two weeks to allow comment. Comments must be submitted to this thread on or before 25 March 2020.

There are only 17 people receiving alerts from this project, so if you know someone affected by this request who might not be listening please pass this message along.

@palemieux
Copy link
Contributor Author

palemieux commented Mar 11, 2020

More background below.

I think the patch goes beyond addressing the labeling issue by also changing the API.

I started by only changing the labels mentioned in the issue. It unfortunately felt like the cure was worse than the ill: it led to inconsistencies in naming in the code base and would still require changes in client code.

Issues stemming from the presence of antiquated symbols in the API can be addressed by documentation.

In the context of D-Cinema, merely changing the external observables, e.g. replacing "Atmos" with "IAB" in command line options and dump output, is probably sufficient -- and maybe even overkill.

In the context of IMF, it is critical to use IAB consistently since ATMOS was never used there.

These changes will break compilation on all projects that rely on the library for IAB (nee Atmos) file accessors.

Yes.

do you have a sed script you want to share?

I do not have one now, but could work on one. Note that some manual work might nevertheless be needed depending on the coding style/conventions on the client side.

so if you know someone affected by this request who might not be listening please pass this message along.

Looking forward to feedback/thoughts.

@jpviollet
Copy link

We can’t break existing API. Adding the new IAB symbols without removing existing ATMOS symbols is ok. The current proposal is too risky. We’d like any proposed alternative change to be posted here for review prior to implementation. Thank you in advance.

@palemieux
Copy link
Contributor Author

Adding the new IAB symbols without removing existing ATMOS symbols is ok.

This would require duplicating a large number of function calls and structures, in addition to merely duplicating UL definitions. Is that what you are proposing?

We can’t break existing API.

Also, are you concerned with D-Cinema and/or IMF?

@jpviollet
Copy link

We discussed various options allowing to have information about both symbol families with John per phone prior to posting this comment. I'll let him decide to disclose them here. The concern is for both IMF and D-Cinema. Thank you.

@palemieux
Copy link
Contributor Author

The concern is for both IMF and D-Cinema. Thank you.

I do not see how IMF is germane here since ASDCPLib does not support IAB (or ATMOS) for IMF.

Can you clarify what your concerns are with IMF?

@jpviollet
Copy link

My apologizes, it was a misunderstanding on my side. There is indeed no impact on IMF.

@palemieux
Copy link
Contributor Author

My apologizes, it was a misunderstanding on my side. There is indeed no impact on IMF.

Understood. Thanks.

@palemieux
Copy link
Contributor Author

It looks like support for IAB Track Files can be added without any reference to ATMOS symbols. See:

#33

@jhursty jhursty closed this Jul 6, 2020
@jhursty
Copy link
Contributor

jhursty commented Jul 6, 2020

superseded by IAB track file #45

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.

Incorrect symbols used for Immersive Audio Data Essence Descriptor and SubDescriptor
3 participants