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

SL-19042: Replace FMOD with VLC for parcel audio #149

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

LLGuru
Copy link
Contributor

@LLGuru LLGuru commented Apr 4, 2023

No description provided.

@LLGuru LLGuru requested a review from cosmic-linden April 4, 2023 01:22
@LLGuru LLGuru requested a review from callumlinden April 4, 2023 01:23
@LLGuru LLGuru force-pushed the SL-19042 branch 4 times, most recently from 0ce07c1 to a643cbb Compare April 4, 2023 09:48
Copy link
Contributor

@cosmic-linden cosmic-linden left a comment

Choose a reason for hiding this comment

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

Code changes look good, but I'm not too familiar with VLC, so I don't know the implications of this.

Copy link
Contributor

@callumlinden callumlinden left a comment

Choose a reason for hiding this comment

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

Looks good. I think VLC supports many more audio formats than FMOD - we should test the more common ones and make sure it works then add the list to the Wiki so other people know what works.

@callumlinden callumlinden self-requested a review April 4, 2023 16:44
<key>Type</key>
<string>Boolean</string>
<key>Value</key>
<integer>0</integer>
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should make VLC the dafault one when it's present, otherwise what's the point? But ability to toggle back to fmod is still usefull in case we run into issues or for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or alternative option - give it proper testing first and enable it by default afterward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or alternative option - give it proper testing first and enable it by default afterward.

That was the plan

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that by the time D582 reaches testing we will forget this change exists, and it is likely to get regression tested with the option still off and passed as is. I suggest requesting a pre-merge testing next time.

P.S. Updated test plan to include UseMediaPluginsForStreamingAudio since otherwise QA wouldn't even know it's there.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can still request testing for this specific feature now, and once that's done, enable it by default in 582.

@LLGuru LLGuru merged commit ba8bcf6 into DRTVWR-582-maint-U Apr 4, 2023
@LLGuru LLGuru deleted the SL-19042 branch April 4, 2023 17:44
@github-actions github-actions bot locked and limited conversation to collaborators Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants