-
Notifications
You must be signed in to change notification settings - Fork 59
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
Conversation
0ce07c1
to
a643cbb
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
<key>Type</key> | ||
<string>Boolean</string> | ||
<key>Value</key> | ||
<integer>0</integer> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
No description provided.