bdrazhzhov / audio-service-mpris

Flutter audio_service plugin implementing Media Player Remote Interfacing Specification
MIT License
5 stars 4 forks source link

Requested bus name "org.mpris.MediaPlayer2.Audio playback.instance1107757" is not valid #1

Closed ryanheise closed 10 months ago

ryanheise commented 10 months ago

Hi @bdrazhzhov and congratulations on your plugin. I would like to link to it from the audio_service README, although before I do I am just doing through the motions of getting it working.

One issue I noticed is that I got this error on my first run:

flutter: Configure AudioServiceLinux.
[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: org.freedesktop.DBus.Error.InvalidArgs: Requested bus name "org.mpris.MediaPlayer2.Audio playback.instance1107757" is not valid
#0      DBusClient._callMethod (package:dbus/src/dbus_client.dart:1114:13)
<asynchronous suspension>
#1      DBusClient.callMethod (package:dbus/src/dbus_client.dart:608:12)
<asynchronous suspension>
#2      DBusClient.requestName (package:dbus/src/dbus_client.dart:332:18)
<asynchronous suspension>
#3      AudioServiceMpris.configure (package:audio_service_mpris/audio_service_mpris.dart:82:5)
<asynchronous suspension>
#4      AudioService.init (package:audio_service/audio_service.dart:1001:5)
<asynchronous suspension>
#5      main (package:audio_service_example/main.dart:37:19)
<asynchronous suspension>

It seems it is using the android channel name as the dbus name, but the dbus name doesn't allow spaces, hence the error. Would it make more sense to reuse the android channel id if anything? The channel name is meant to be a user facing string so it will have spaces, while the channel id is the internal identifier that looks more like a package name.

The other way to go is to have some entirely different key for the Linux config values, although that may require more thought if it is to be done in a way that allows the audio_service_mpris config parameter keys to evolve independently of audio_service.

bdrazhzhov commented 10 months ago

@ryanheise, Thanks for the issue you found. I think I can fix it in the near future.

bdrazhzhov commented 10 months ago

@ryanheise, I've changed the plugin behavior. Now it uses the android channel id (as you proposed) for dbus interface name string.

Released version 0.1.1 with the changes.

ryanheise commented 10 months ago

Doing more testing, I found a similar issue with an invalid name used for the dbus object path:

    _mpris.metadata = Metadata(
        trackId: '/trackId/${request.mediaItem.id}',
        title: request.mediaItem.title,
        length: request.mediaItem.duration,
        artist: artist,
        artUrl: request.mediaItem.artUri.toString(),
        album: request.mediaItem.album,
        genre: genre);

If the app developer uses the media URL as the MediaItem.id (which is relatively common practice), this results in the above trackId being set to something containing https:// where the colon and double slash are not allowed in the dbus object path which validates like this:

  /// Creates a new D-Bus object path with the given [value].
  ///
  /// An exception is shown if [value] is not a valid object path.
  DBusObjectPath(String value) : super(value) {
    if (value != '/') {
      if (value.contains(RegExp('[^a-zA-Z0-9_/]')) ||
          value.contains('//') ||
          !value.startsWith('/') ||
          value.endsWith('/')) {
        throw ArgumentError.value(value, 'value', 'Invalid object path');
      }
    }
  }

Maybe a simple workaround is that I guess you could just generate a uuid instead? As there doesn't seem to be a need for it to be the same as the MediaItem.id (although if you need some mapping, you could keep track of the mapping between the two IDs in a Map...)

bdrazhzhov commented 10 months ago

@ryanheise, you're right.

Need to look into it. I think I'll fix it soon.

bdrazhzhov commented 10 months ago

@ryanheise,

I released version 0.1.2 with the dbus path object creation fix for trackId field of Metadata.

ryanheise commented 10 months ago

Sorry to trouble you again! Would you be able to relax the constraint to this? (edit: or skip to the last paragraph for a lower maintenance alternative):

  uuid: ">=3.0.5 <5.0.0"

This should be possible since your use of uuid is compatible between both version 3 and version 4.

The reason for this is to support the widest possible range of dependencies to avoid any constraint solving issues during pub get. In particular, uuid 4 is relatively new, so some plugins haven't upgraded to it yet. So if an app depends on different plugins that use different versions of uuid, those plugins can't be used together in the same app. If a plugin actually can work with different versions of uuid, then it can be helpful to reflect that in the constraint spec, and this will allow constraint solving to find a solution to use both plugins at the same time.

Having said that, I'm thinking it might be easier from a maintenance point of view if you don't depend on uuid, and instead come up with some kind of internal solution. For example, would it work to simply keep an auto-incrementing sequence number in memory, and just assign each new Metadata the next sequence number? Then you don't need to worry about always keeping your uuid dependency at the widest range whenever a new major version of uuid comes out.

bdrazhzhov commented 10 months ago

@ryanheise,

I think I can remove uuid from the plugin. There is no point in the mpris:trackid property (for which I used uuid), it can be temporarily removed because it is required for working with tracklist (currently I don't plan to support it).

I'll keep your caveats in mind when I decide to add tracklist support to the plugin.

I released version 0.1.3 with the changes.

ryanheise commented 10 months ago

Thanks @bdrazhzhov , everything seems to be working perfectly! I have now added it to the audio_service example in the latest commit.