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

Remove dependency on gi.GstAudio #31

Closed
robcee opened this issue Mar 25, 2021 · 5 comments
Closed

Remove dependency on gi.GstAudio #31

robcee opened this issue Mar 25, 2021 · 5 comments

Comments

@robcee
Copy link

robcee commented Mar 25, 2021

Doing some exploratory work on issue #6, I noticed we're importing gi.repository.GstAudio. This requires installation and import of Gtk, Cairo and a bunch of other Gnome-related libs that may not be applicable on a headless device.

Looking deeper, it appears we're only using GstAudio.StreamVolume.convert_volume() method to convert between linear and cubic volumes. The actual implementation in GstAudio is pretty straightforward in this case:

case GST_STREAM_VOLUME_FORMAT_CUBIC:
      g_return_val_if_fail (val >= 0.0, 0.0);
      switch (to) {
        case GST_STREAM_VOLUME_FORMAT_LINEAR:
          return val * val * val;
        case GST_STREAM_VOLUME_FORMAT_CUBIC:
          return val;
        case GST_STREAM_VOLUME_FORMAT_DB:
          return 3.0 * 20.0 * log10 (val);
      }

and the reverse (Linear to Cubic):

case GST_STREAM_VOLUME_FORMAT_LINEAR:
      g_return_val_if_fail (val >= 0.0, 0.0);
      switch (to) {
        case GST_STREAM_VOLUME_FORMAT_LINEAR:
          return val;
        case GST_STREAM_VOLUME_FORMAT_CUBIC:
          return pow (val, 1 / 3.0);
        case GST_STREAM_VOLUME_FORMAT_DB:
          return 20.0 * log10 (val);
      }

I propose removing the dependency and imports, and implementing this directly.

source: https://github.com/jojva/gst-plugins-base/blob/master/gst-libs/gst/audio/streamvolume.c

edit: updated to include the reverse case.

@kingosticks
Copy link
Member

Mopidy requires GStreamer which requires GObject Introspection for use in Python. Since gi.repository.GstAudio is always available to us, why re-implement this code?

@kingosticks
Copy link
Member

I might remember this wrong but isn't the Cairo stuff a suggested package rather than an actual dependency?

@robcee
Copy link
Author

robcee commented Mar 25, 2021

you might be right, @kingosticks. I believe it was suggested only.

Still feels heavy to me to bring in the gi stuff into an extension that's only using it for some trivial math. It would make development easier on this extension. Nevertheless, it's the call of the maintainers and I defer to your expertise.

Feel free to close this.

@djmattyg007
Copy link

If it's a dependency of core Mopidy code, changing its status as a direct dependency in this codebase won't make a lot of difference.

You're definitely correct to be wary of unnecessary dependencies. Just this week there's been another example of where long dependency chains can create hidden risks:

https://www.theregister.com/2021/03/25/ruby_rails_code/

However, in this case it's likely fine.

If you've brought in suggested dependencies by accident, and want to prevent these kinds of accidents from occurring again in future, I recommend following this SuperUser answer:

https://superuser.com/a/615583

This assumes you're using a Debian-based system of course.

@robcee
Copy link
Author

robcee commented Apr 5, 2021

based on comments by @kingosticks and @djmattyg007 above, I'm closing this. WONTFIX.

@robcee robcee closed this as completed Apr 5, 2021
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

No branches or pull requests

3 participants