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

propagate Properties common to all Representations to MediaInfo #4682

Conversation

stschr
Copy link
Contributor

@stschr stschr commented Jan 30, 2025

This PR re-establishes the use case that Supplemental- and EssentialProperties can be assigned to either AdaptationSets or Representations.

@stschr
Copy link
Contributor Author

stschr commented Jan 30, 2025

A few open questions:

  • Does the use of propertyType make sense? We could implement this also with getEssentialPropertiesForRepresentation()/ getSupplementalPropertiesForRepresentation() and more.
  • Up to now, I've implemented the new function provided by getCommonEssentialPropertiesForAdaptationSet() in parallel to the older getEssentialPropertiesForAdaptationSet(). Keep the old functions? Implement the new feature under the old name?
    EDIT: Hmm, we need to keep in mind that CapabilitiesFilter() also accesses this function and I see pros and cons here
    EDIT2: Thinking further, EssentialProperties in context of capability-filtering have another (not yet implemented) feature: the @id attribute. Without going into details here, I think it's cleaner if we stick with the old functions in the Capabilites module.
    This leases us keeping both functions in DashManifestModule - I'll clean my code in that sense.

@dsilhavy : Let me know what you think... (question 1 only)

@stschr stschr marked this pull request as draft January 30, 2025 15:46
@stschr stschr requested a review from dsilhavy January 30, 2025 15:46
@stschr
Copy link
Contributor Author

stschr commented Jan 31, 2025

Just figured out, we have duplicated code that could be avoided:
Functions getEssentialPropertiesForAdaptationSet() and getEssentialPropertiesForRepresentation() in DashManifestModel.js are the same.
Should I combine them into one getEssentialPropertiesForAdaptationSetOrRepresentation()?

Likewise in DashAdapter.js with getEssentialPropertiesForAdaptationSet() and getEssentialPropertiesForRepresentation.

@stschr stschr marked this pull request as ready for review January 31, 2025 15:40
@dsilhavy dsilhavy added this to the 5.0.0 milestone Feb 3, 2025
src/dash/models/DashManifestModel.js Outdated Show resolved Hide resolved
src/dash/models/DashManifestModel.js Show resolved Hide resolved
src/dash/models/DashManifestModel.js Outdated Show resolved Hide resolved
src/dash/models/DashManifestModel.js Show resolved Hide resolved
@dsilhavy
Copy link
Collaborator

dsilhavy commented Feb 3, 2025

A few open questions:

  • Does the use of propertyType make sense? We could implement this also with getEssentialPropertiesForRepresentation()/ getSupplementalPropertiesForRepresentation() and more.
  • Up to now, I've implemented the new function provided by getCommonEssentialPropertiesForAdaptationSet() in parallel to the older getEssentialPropertiesForAdaptationSet(). Keep the old functions? Implement the new feature under the old name?
    EDIT: Hmm, we need to keep in mind that CapabilitiesFilter() also accesses this function and I see pros and cons here
    EDIT2: Thinking further, EssentialProperties in context of capability-filtering have another (not yet implemented) feature: the @id attribute. Without going into details here, I think it's cleaner if we stick with the old functions in the Capabilites module.
    This leases us keeping both functions in DashManifestModule - I'll clean my code in that sense.

@dsilhavy : Let me know what you think... (question 1 only)

I suggest we keep both getEssentialPropertiesForAdaptationSet and getCommonEssentialPropertiesForAdaptationSet as we need getEssentialPropertiesForAdaptationSet in CapabilitiesFilter. The name getCommonEssentialPropertiesForAdaptationSet is a bit confusing, though. This function returns all EssentialProperties that are on AdaptationSet level and the common ones from Representation. Maybe it should be something like getCombinedEssentialPropertiesForAdaptationSet. To indicate that this is not only the common data.

Just figured out, we have duplicated code that could be avoided:
Functions getEssentialPropertiesForAdaptationSet() and getEssentialPropertiesForRepresentation() in DashManifestModel.js are the same.
Should I combine them into one getEssentialPropertiesForAdaptationSetOrRepresentation()?
Likewise in DashAdapter.js with getEssentialPropertiesForAdaptationSet() and getEssentialPropertiesForRepresentation.

I would like to keep dedicated functions exposed to the outside. So getEssentialPropertiesForAdaptationSet and getEssentialPropertiesForRepresentation should both stay. However, if they share code I think it would be good to use an internal/private function that combines the common code. The private function is then shared by the public functions.

@stschr
Copy link
Contributor Author

stschr commented Feb 3, 2025

Thanks for the review!
I think, I've all covered now.

"Common" was indeed misleading, I like "combined" ;-)
I also implemented this private function for common code. As final change, we may want to move the public functions (getSupplementalPropertiesForAdaptationSet(), getCombinedSupplementalPropertiesForAdaptationSet() and getSupplementalPropertiesForRepresentation()) for SupplementalProperties upwards next to those for EssentialProperties...

@stschr stschr marked this pull request as draft February 3, 2025 17:49
@stschr stschr marked this pull request as ready for review February 3, 2025 18:35
src/dash/models/DashManifestModel.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
@dsilhavy
Copy link
Collaborator

dsilhavy commented Feb 4, 2025

Thanks for the review! I think, I've all covered now.

"Common" was indeed misleading, I like "combined" ;-) I also implemented this private function for common code. As final change, we may want to move the public functions (getSupplementalPropertiesForAdaptationSet(), getCombinedSupplementalPropertiesForAdaptationSet() and getSupplementalPropertiesForRepresentation()) for SupplementalProperties upwards next to those for EssentialProperties...

Thanks, Stephan, that looks good. One minor variable naming change would be nice (see my latest review comments). Please also move the functions upwards as suggested by you.

@dsilhavy dsilhavy merged commit 7d0d1d0 into Dash-Industry-Forum:development Feb 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants