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

MediaInfo: introduce DescriptorType #4158

Conversation

stschr
Copy link
Contributor

@stschr stschr commented Mar 28, 2023

This PR addresses #4123 and #4124 to the extend that MediaInfo is populated with new elements containing an array of new DescriptorType for all manifest elements of this type (viewpoint, role, accessibility, audioChannelConfiguration).

Old elements are still available to maintain backward compatibility, but could be deleted in future (when migrating to v5.0?).

settings related API (setInitialMediaSettingsForType , setTrack, extractSettings, matchSettings,.. ) are not yet updated. as I'm hesitant to expose two redundant elements to customers.

@dsilhavy : May I ask you to review and provide your feedback? Thank you!

@dsilhavy dsilhavy added this to the 4.7.0 milestone Mar 28, 2023
@dsilhavy dsilhavy self-requested a review March 28, 2023 13:19
@dsilhavy
Copy link
Collaborator

Thanks @stschr I will check it

src/dash/vo/DescriptorType.js Show resolved Hide resolved
src/dash/vo/MediaInfo.js Outdated Show resolved Hide resolved
src/dash/models/DashManifestModel.js Outdated Show resolved Hide resolved
src/dash/models/DashManifestModel.js Outdated Show resolved Hide resolved
src/dash/DashAdapter.js Outdated Show resolved Hide resolved
src/dash/DashAdapter.js Outdated Show resolved Hide resolved
src/dash/DashAdapter.js Outdated Show resolved Hide resolved
src/dash/DashAdapter.js Outdated Show resolved Hide resolved
src/dash/DashAdapter.js Show resolved Hide resolved
@dsilhavy
Copy link
Collaborator

@stschr Thank you for the PR I left some comments. If you read this in time: Can you joint our dash.js call tomorrow. I would like to discuss this question with you again:

My suggestion would be to save the data as it is done below for supplementalPropertiesAsArray but in supplementalProperties and not use supplementalPropertiesAsArray

We can highlight this change in the release notes in case applications use this attribute in some way. However, the question arises if then do it all at once and remove the old format for viewpoint, role, accessibility, audioChannelConfiguration already here as well.

@stschr
Copy link
Contributor Author

stschr commented Apr 12, 2023

Thanks @dsilhavy !
Indeed I plan to join the call tomorrow.

Agree - in principle - with all your suggestions.
Changes are WIP, with the low hanging fruits already committed. Hope to get everything 'editorial' done prior to our call tomorrow where we can discuss the non-backward-compatible changes.

karma.unit.conf.js Outdated Show resolved Hide resolved
@stschr
Copy link
Contributor Author

stschr commented Apr 14, 2023

Just to get a common understanding:
With the duality of "legacy" vs "new" elements in a 2-phased release, how do we want to handle these names? (e.g. roles and rolesWithSchemeIdUri)

  • As discussed yesterday, V4.7.0 will have both names.
  • But in V5.0, do we just deprecate=remove the legacy name?
    Or do we want to rename the *WithSchemeIdUri to the 'legacy' name (not nice)? Is such decision still important if we rename the parent object (MediaInfo --> e.g. TracKInfo) ?

Otherwise:
I just rebased my branch with development, thus this PR is ready to be merged.

@dsilhavy
Copy link
Collaborator

Thanks @stschr I will check asap.

Regarding your question: In my opinion, we should use the legacy name in version 5. As a user, I would expect to get schemeIdUri and value for each object (role, viewpointetc.). The extension withSchemeIdUri seems to be redundant as I would expect to find this information anyways.

@bbert
Copy link
Contributor

bbert commented May 4, 2023

@dsilhavy @stschr
I am rebasing XML parser pull request and face an issue with what has been done in this PR.
As a remind, in XML parser pull request, we take the opportunity for simplification to remove all references to "asArray", while assuming which nodes shall be represented as arrays or not according to the DASH spec.

Regarding SupplementalProperty, these are stored in 2 ways in MediaInfo: supplementalProperties and supplementalPropertiesAsArray, with supplementalProperties as an object with schemeIdUri property as a key.

Is there any valid reason to keep the supplementalProperties as an object, except for the PlaybackController which looks for SupplementalProperty with schemeIdUri equal to DVB LL scheme? In that case we can simply filter the array.

@dsilhavy
Copy link
Collaborator

dsilhavy commented May 4, 2023

@bbert Thanks we kept this for backwards compatiblity. I agree with you, please see my comment here: #4158 (comment)

@bbert
Copy link
Contributor

bbert commented May 5, 2023

@dsilhavy OK fine, so I suggest in XML parser PR to store in supplementalProperties as an array, as I done for all other elements that are parsed as arrays (0..N or 1..N in the spec)

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.

None yet

3 participants