wjt commented 9 years ago

I was just skim-reading this code, and it looks like various mpris properties are not implemented correctly. Here's some code from OMXControl.cpp:

  else if (dbus_message_is_method_call(m, DBUS_INTERFACE_PROPERTIES, "CanQuit")

This is supposed to implement the CanQuit property, but it doesn't: it implements a method called CanQuit on org.freedesktop.DBus.Properties. You request a D-Bus property by calling the method org.freedesktop.DBus.Properties.Get, passing the interface name (in this case org.mpris.MediaPlayer2) and the property name (in this case CanQuit) as arguments; or by calling GetAll on the same interface, passing only the interface, which gives you back a dict of properties.

So, instead of all the many

dbus_message_is_method_call(m, DBUS_INTERFACE_PROPERTIES, "Foo")

checks, I would expect to see these two cases handled:

dbus_message_is_method_call(m, DBUS_INTERFACE_PROPERTIES, "GetAll")
dbus_message_is_method_call(m, DBUS_INTERFACE_PROPERTIES, "Get")

Later on there's stuff like this:

  else if (dbus_message_is_method_call(m, DBUS_INTERFACE_PROPERTIES, "Volume"))

which should instead be something based on

if (dbus_message_is_method_call(m, DBUS_INTERFACE_PROPERTIES, "Set")) {
    dbus_message_get_args(m, &error, DBUS_TYPE_STRING, &interface, DBUS_TYPE_STRING, &property, DBUS_TYPE_VARIANT, &new_value, DBUS_TYPE_INVALID);
   // ... check interface and property, change internal state to new_value accordingly

popcornmix commented 9 years ago

Afraid I didn't write the dbus code and I am not an expert on it. I'd be happy to consider a pull request if you can produce a more correct implementation.

traumedy commented 9 years ago

i find the open source community more entertaining than cartoons.

I want to believe it ends in the inverse of the lowest common denominator, but I'm not so sure that the math works out..

Thank you for heading this project, I have followed the development for a long time considering it for use in a low cost video array solution. You are solid.

On Tuesday, November 25, 2014, popcornmix notifications@github.com wrote:

Afraid I didn't write the dbus code and I am not an expert on it. I'd be happy to consider a pull request if you can produce a more correct implementation.

— Reply to this email directly or view it on GitHub https://github.com/popcornmix/omxplayer/issues/276#issuecomment-64398540 .

vcrocher commented 8 years ago

While implementing a DBus controller for omxplayer I came across this same "problem" (mis-implementation of MPRIS2) so I started to work on re-writting OMXControl.cpp (here) . The changes I'm working on:

Turn some Methods to Properties according to MPRIS2 specs
Add distinct Play, Pause and PlayPause controls according to MPRIS2 specs as well (already done)
Add Rate control support (done but possible bugs)

I would be happy to have people testing it when a bit more advanced.

In case you would consider a pull request on this one: what about compatibility ? Would you like to keep both the current (mis-implemented) methods together with the new Properties or only the new ones ? If only the new one, it's easy to update dbuscontrol.sh to use the new implementation but will break compatibility for other dbus controllers. If keeping both, code might be quite messy and more difficult to maintain.

jehutting commented 8 years ago

Personally, I think you shouldn't break the current implementation; although it is mis-implemented you simply don't know how it is being used. People have built all kind of programs/wrappers/etc. around omxplayer dbus interface.

I also think that there is no need to break it; have something like an --mpris2 option which "switch"es on run-time to the correct mpris2 implementation. When you "detect" a given dbus command is not an mpris2 command (as there are e.g. Hide/Unhide Video, SetAlpha), just past it to the current dbus implementaton. These non-mpris2 commands are the omxplayer mpris2 extensions.

Create a mpris2control.sh which is to be used in stead of dbuscontrol.sh.

Wrong or good, let people decide by themselves which dbus interface to use.

popcornmix commented 8 years ago

Can any author's of applications that control omxplayer comment here? If we assume omxplayer is updated to use correct MPRIS2 specs, and there is a way of querying if omxplayer is the new or old version, then omxplayer control apps could be updated to handle either both schemes or just the new scheme (with a message suggesting updating omxplayer if it is too old).

Does that seem acceptable to authors?

vcrocher commented 8 years ago

Can't comment for them. But it is perfectly possible to keep both implementations even without the use of a switch (suggested by jehutting): the two implementations are not in conflict but simply redundant.

In order to keep both and still have a maintainable code I can introduce a few methods in the OMXControl class (to ensure that concurrent methods/properties will have the same effect). Maybe we can add a "deprecated" warning or log when old methods are used and plan to remove them in 10 years...

The only true conflict I see in my new implementation is the change of Pause method: it's now only pausing the player and will have not effect if the player was already paused whereas it previously had the same effect as the PlayPause control: Pausing when Playing and Playing when Paused. PlayPause method remains untouched.

popcornmix commented 8 years ago

@kennyyy24 Feel free to open a PR with your improvements and describe what is fixed and what may potentially break. We can let people comment on the PR if they see issues. But in general if we are doing something wrong then we should fix it. I imagine there are only a handful of apps that control omxplayer through dbus and if the breakage is minor, and can be fixed up (in the controlling apps) simply then I think that may be okay.

vcrocher commented 8 years ago

I will.

But since the idea is to improve DBUS support, two questiona to improve the SupportedMimeTypes and SupportedURISchemes properties:

SupportedMimeTypes is empty at the moment. Do we have a list of minimal support (I imagine that depends on ffmpeg) ?
SupportedURISchemes lists only 'file' and 'http' at the moment whereas I think they are other supported. Same question: do we have a list I can use?

vcrocher commented 8 years ago

I've just opened a pull request with these modifications:

419

blakee commented 8 years ago

This PR is going to break a lot of existing implementations for reasons other than D-BUS: Things like this which seem innocuous are going to break scripts like this or this.

Personally I would like to see the existing D-BUS behaviour continue to work (possibly with deprecation warnings), I agree with jehutting that a command-line switch would be a good way of implementing this. I don't see why this needs to be a breaking change for all the players out there already.

vcrocher commented 8 years ago

@blakee You are right, my bad: the change of the closing message was here for testing purpose only and I forgot to revert it to the original before submitting the pull request. You can consider this change is not here any more.

Regarding keeping both original and new implementation, as said it's possible even without a switch (I worry that the switch would be more confusing than helpful) for everything but the new Pause behaviour. The only drawback will be to have redundant code which might end-up with different behaviours for the two implementation if next updates don't pay much attention.

I can propose something like this: https://gist.github.com/kennyyy24/c4b0e721e82cb74c6bde#file-omxcontrolboth-cpp .

vcrocher commented 8 years ago

Here it is: #419 : nothing change but the behavior of the Pause command if you don't want to. Previous implementation of properties as methods will generate a log but still working as previously. New implementation available in parallel.

popcornmix / omxplayer

Properties on mpris MediaPlayer2 interface mis-implemented #276

419