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

Implement checking Dolby Atmos #1275

Merged
merged 5 commits into from
Sep 22, 2023

Conversation

klatoszewski-oke
Copy link
Contributor

I have implemented checking Dolby Atmos in currect representation. #1266

@peaBerberian
Copy link
Collaborator

Nice thank you!

Just some minor nitpicks:

  • we only update files in the dist directories when doing releases. Can you unstage it here?

  • isSpatialAudio doesn't seem to be added on the API-side to the audio track objects returned to the various audio track APIs, so I guess player.getAvailableAudioTracks() wouldn't say which audio track have spatial audio Representations.

    Here we just have to update the toAudioRepresentation method of a Representation (a getAvailableAudioTracks API call goes through the TrackChoiceManager's method of the same name internally, which mainly calls Adaptation.toAudioTrack to all Audio Adaptation of the current Period itself calling Representation.toAudioRepresentation on its inner Representations, so just changing toAudioRepresentation repercutes on audio-track-related API).

  • we may want to also add it to our WebAssembly parser (equivalent of the node_parsers part of the default JS MPD parser but when the WebAssembly DASH parser is used) . Though this is very straightforward when you already know the code so I can do it.

  • if we add it to the getAvailableAudioTracks and getAudioTrack method (both by updating toAudioRepresentation like written above) as well than to the audioTrackChange event (it is added automagically to that event if added to getAudioTrack as the RxPlayer just calls that method behind the hood when triggering that event), we want to update the api documentation (in doc/api) of those. I can update it myself also here if you want

  • isSpatialAudio isn't added here to the audio preferences API - so it's not possible for an application to tell the RxPlayer in advance that it should first choose spatial audio. Though this may be seen as a different feature and thus done in another PR, so no need in that PR.

@klatoszewski-oke
Copy link
Contributor Author

Hello,

Thanks for review.

  • we may want to also add it to our WebAssembly parser (equivalent of the node_parsers part of the default JS MPD parser but when the WebAssembly DASH parser is used) . Though this is very straightforward when you already know the code so I can do it.

If it is not problem for you, you could add this code.

@peaBerberian
Copy link
Collaborator

If it is not problem for you, you could add this code.

Done!

While checking deeper into this, I also made some small adjustments:

  • isSpatialAudio was always either true or false.

    Why not technically, but the logic of the RxPlayer for now for such metadata (e.g. isAudioDescription, isDub for dubbing tracks, width, height, codecs etc.) is to only set it at parser-level when its status is explicitely stated in the Manifest. So false would be for cases where we explicitely know that the Representation does not contain spatial audio.

    For example, we could have spatial audio other than Dolby Atmos with a different signalization, in which case we would mark as isSpatialAudio: false by mistake.

    So for now it is set either to true for JOC, or undefined in other cases.

  • I also only specified the property on Representation objects (in src/manifest/representation) if it was set, to follow the he previous style of that class.

  • looking at specs, it seems that Dolby Atmos JOC is always linked to the schemeIdUri "tag:dolby.com,2018:dash:EC3_ExtensionType:2018". I added this other check to be sure we don't make false assumption if another "JOC" comes out in the future for an unrelated property.

  • There was no need to define it in src/parsers/manifest/dash/node_parser_types.ts. Those types are for what we call our MPD intermediate representation which is in fact just the original MPD re-formatted as a JS Object so we can work on it more easily.
    Though I understand that the Manifest parser code can be complex to get into with lot of repetitions (even the fact that the property has to be repeated in src/manifest/* may seems weird). There may be "simplifications" (between quotes because that repetition's purpose was to make each step simpler) in the future to make it clearer.

  • I updated the documentation for "local" manifest (offline playback of downloaded Manifests - we use this at Canal+ on some devices and applications). I also made it optional there.

@peaBerberian peaBerberian modified the milestones: 3.31.1, 3.32.0 Sep 12, 2023
@peaBerberian peaBerberian changed the base branch from master to next September 12, 2023 17:17
@klatoszewski-oke
Copy link
Contributor Author

Thanks, looks good.

@klatoszewski-oke
Copy link
Contributor Author

I can see SonarCloud analysis fails. How can we fix this?

@peaBerberian
Copy link
Collaborator

peaBerberian commented Sep 18, 2023

I can see SonarCloud analysis fails. How can we fix this?

Yes, that's not much of an issue, just that the sonarcloud key is only linked to this project, not forks, or something like that.

It's not blocking its merge as there's another sonarcloud check before an official release anyway. I just didn't merge it yet because I did not take the time yet to properly check that everything is right here, but I think I'll be able to do so this week.


As for the corresponding release, it may be done in next weeks.
For the whole story, I've been invested lately on a more R&D-oriented subject on whether running in WebWorker could attenuate some of the issues we're encountering with performance at Canal+ ( #1272).
That work won't be merged in next releases as it is still unstable and unproven, but the large amount of work spent on it uncovered multiple potential improvements to the current code that I would like to backport to both the future v3 and v4 releases.

So I guess I will take time to backport those this week, and try making the release in the weeks coming after.

In any case, we also now have sort of release previews where we regularly publish on npm pre-releases under the dev tag (they are releases without having to perform the whole release process, including lot of manual testing and documentation, which takes a lot of time). So if you need this fast, we could add it to a dev release as soon as it is merged.

@peaBerberian peaBerberian merged commit 2938062 into canalplus:next Sep 22, 2023
2 of 3 checks passed
peaBerberian added a commit that referenced this pull request Sep 22, 2023
peaBerberian added a commit that referenced this pull request Sep 22, 2023
peaBerberian added a commit that referenced this pull request Sep 25, 2023
peaBerberian added a commit that referenced this pull request Sep 26, 2023
peaBerberian added a commit that referenced this pull request Sep 27, 2023
@peaBerberian peaBerberian mentioned this pull request Oct 2, 2023
peaBerberian added a commit that referenced this pull request Oct 13, 2023
peaBerberian added a commit that referenced this pull request Oct 13, 2023
peaBerberian added a commit that referenced this pull request Oct 13, 2023
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.

2 participants