OpenHome services #45

ademenev · 2013-10-29T14:55:46Z

I consider OpenHome services working now

There are few things I think could be done:

cleanup shared_metadata module
moderate writes to playlist file
most important - sometimes, when changing tracks, there are huge lags before subscribed devices receive notifications. I have tracked this down to UpnpNotify() taking too long too execute, but have not found the reason and solution yet. Possibly this problem has something to do with the fact that control point periodically sends requests to Time service, and on track change many notifications are sent to different services, and action can somehow interfere with notifications

this does not create an OpenHome renderer yet, but can be controlled with an UPnP device and service analyzer, e.g. UPnP Inspector

… output module

OpenHome services do not use LastChange for eventing

…e (same for previous) This has to be revisited when properly implementing shuffle and repeat

Manually merged upstream commit b59259c, I am at a point where automatic merge would not work

Manually merged upstream commit fd8f9c7

Manually merged upstream commit 238a3c6

Manually merged upstream commit 90f9b9b

When using LastChange, not all modified variables must be sent in event. We use sendevents flag to filter out unwanted variables. See: hzeller@47da829 hzeller@ed0f527

See: hzeller@47da829 hzeller@ed0f527

hzeller · 2013-11-11T05:38:22Z

src/upnp_transport.c

@@ -191,7 +194,7 @@ enum UPNPTransportError {
 	[TRANSPORT_VAR_ABS_TIME_POS] = "NOT_IMPLEMENTED",
 	[TRANSPORT_VAR_REL_CTR_POS] = "2147483647",
 	[TRANSPORT_VAR_ABS_CTR_POS] = "2147483647",
-        [TRANSPORT_VAR_LAST_CHANGE] = "<Event xmlns=\"urn:schemas-upnp-org:metadata-1-0/AVT/\"/>",
+    [TRANSPORT_VAR_LAST_CHANGE] = "<Event xmlns=\"" TRANSPORT_EVENT_XML_NS "\"/>",


Nice, this is better.

hzeller · 2013-11-11T06:01:48Z

Sorry, that looks like a lot of comments, but I am kinda used to detailed code-reviews in my day job. Mostly things are ok, often I just have suggestions for future changes.

In general: I like it, nice job! I haven't looked too much into detail in the open-home protocol implementation (what actions are implemented, what variables) - I supposed you did that by spec, so I wasn't looking too much in to cross-verifying that. I played around with BubbleUPnP, and that worked :)

I do have some comments regarding the variable notification changes. I like that asynchronous notification scheme with a separate thread, it could help reducing some latency (I have never seen this myself, but this is good nevertheless). There are some problems with the handover between different threads though: it is possible that sometimes a change notification is missed as the notify_collector can be changed by multiple threads in a race condition ... and only one wins. This is potentially benign. But problematic is the access of the variable contents in different threads. Unless I am mistaken, the variables content is read from within the notification thread without a locked mutex that also protects the variable content writing. This certainly can lead to corrupt variable reads.

Can you fix these things ? I think then we're ready to merge this into the main branch.

Thank you,
Henner.

ademenev · 2013-11-13T14:38:07Z

I agree that there is a possible race condition. I think the changes related to notify_mutex I propose can solve this

ademenev · 2013-11-13T14:39:28Z

@hzeller , I have now made some modifications you proposed - but only those which are trivial. Will take some time this weekend to do the rest

hzeller · 2013-11-22T14:39:50Z

Can you already submit and push your changes so far, so that I can have a look ?
(I am now on vacation right now, so I might have some time to look at things)

ademenev · 2013-11-22T16:22:56Z

I have just finished a couple of changes, will commit soon

service-level mutex was only used to protect access to variable containers, and notifications thread did not protect access to containers The mutex was moved to variable container, and is now used in all places where variable containers are accessed. Also, the notification code was improved to make sure variable changes are not missed

that is the right place where variable names belong, and this allows to not pass complete service descriptor to variable container constructor

distinguish SENDEVENT_YES from SENDEVENT_LASTCHANGE

hzeller · 2013-12-11T15:27:04Z

I am back from vacation now. I ended up travelling so much, that my internet access was actually not that great so couldn't do much of a review.
I'll no revisit your pull request ASAP.

triplem · 2014-01-08T20:12:38Z

Any progress on this one?

triplem · 2014-01-17T21:57:41Z

Currently testing this pull request. Looks like the events if the song changed are not send correctly to the UPnP services (e.g. UPnP-display).

Well, I wander if this works for gmrender in the original version as well anyway....

christiscarborough · 2014-02-05T22:54:24Z

Shame that this kind of went quiet. You can get OpenHome services with the current code using the bubbleupnp-server wrapper (see http://www.coraline.org/non-fiction/raspi-upnp-renderer for details of how I did it), but a native implementation would probably work better.

hzeller · 2014-02-05T22:55:58Z

On 5 February 2014 14:54, christiscarborough [email protected]:

Shame that this kind of went quiet. You can get OpenHome services with the
current code using the bubbleupnp-server wrapper (see
http://www.coraline.org/non-fiction/raspi-upnp-renderer for details of
how I did it), but a native implementation would probably work better.

I am very sorry for the delay, things went kind of crazy here and I am busy
each weekend. But I have it very high on my TODO list to finish the
review...

-h

christiscarborough · 2014-02-05T22:58:29Z

No worries. Glad to see it may still be on the cards.

whyman · 2017-09-12T19:05:39Z

Any news?

sqlfairy · 2019-10-06T08:59:29Z

This looks perfect for a project I'm working on. Tantalizingly close... Any chance of a merge?

Edit: Nevermind. I see this has been forked already.

hzeller · 2019-10-22T04:37:06Z

There were a lot of changes needed and the review fizzled out eventually as this was touching so much code that testing without access to a control-point was a gamble.

Anyway, want to pick this up again. To do so, it needs to be tested and compared against the specification. This means

It needs to be testable somehow. Is there an open source OpenHome control point or some Android app that this can be tested with ?
specifiction needed. Is there a link to the OpenHome specification somewhere ?

christiscarborough · 2019-10-22T08:34:28Z

On 22 Oct 19 5:37 am, Henner Zeller wrote: There were a lot of changes needed and the review fizzled out eventually as this was touching so much code that testing without access to a control-point was a gamble. Anyway, want to pick this up again. To do so, it needs to be tested and compared against the specification. This means * It needs to be testable somehow. Is there an open source OpenHome control point or some Android app that this can be tested with ? * specifiction needed. Is there a link to the OpenHome specification somewhere ? —

I'm not aware of a current open source OpenHome control point. Lynn Kazoo is a free install on Windows, Android, Ipad or Mac but obviously you can't poke around in the source. They had a previous open source app called Kinsky, which was open source, but they seem to have taken it down completely. There is a docker vm here. https://github.com/DoomHammer/kinsky-docker. No idea if that contains the source or not. bubbleupnp-server (java app) on Android implements an OpenHome wrapper around other UPNP renderers, and may be a useful source of understanding, but not sure of the licence. OpenHome spec is at http://wiki.openhome.org/wiki/OhMedia. Hope this helps. Christi

Andrey Demenev added 30 commits October 20, 2013 09:39

move MIME type registration to a separate program module

6a71712

add orginal copyright statement to reused code

044a132

initial implementation of OpenHome Playlist service

1434cdd

this does not create an OpenHome renderer yet, but can be controlled with an UPnP device and service analyzer, e.g. UPnP Inspector

allow NULL argument for output_gstreamer_set_next_uri

8c6f66c

stop playback when current playlist item is deleted

2441f7d

move playlist handling utils to separate module

3d0255c

add utilities for sharing track metadata changes between modules

172f2a2

add stream details callback to shared_metadata

ad56432

make output modules provide shared metadata

193a4ad

notify metadata listeners about stream details changes from gstreamer…

52d5002

… output module

ooopps ...

4c7366f

initial implementation of OpenHome Info service

6a7b202

move time update thread out of upnp_transport module

6c5a9f8

initial implementation of OpenHone Time service

454104a

stop notifications flood

f2be8c3

initial implementation of OpenHome Product service

7ed9369

add option to select UPnP AV or OpenHome mode

c147582

include definition of memset()

0292047

improve state variable eventing

5ef0cf4

OpenHome services do not use LastChange for eventing

OpenHome seems to use big-endian for IDs list

57ef9b6

working OpenHome renderer! (I hope)

38c7822

do not restart playback when next track is required, and there is non…

afad10d

…e (same for previous) This has to be revisited when properly implementing shuffle and repeat

There are different namespaces for control and transport service

c5f129d

Manually merged upstream commit b59259c, I am at a point where automatic merge would not work

Qualify namespace choices

43f23f3

Manually merged upstream commit fd8f9c7

Include 'channel' attribute in volume-related event variables

492d4a8

Manually merged upstream commit 238a3c6

Add 'Loudness' to the set of attributes that need 'channel' qualifier

a16663b

Manually merged upstream commit 90f9b9b

ignore non-eventable variable when building LastChange

09dabd4

When using LastChange, not all modified variables must be sent in event. We use sendevents flag to filter out unwanted variables. See: hzeller@47da829 hzeller@ed0f527

do not send non-evened variables on initial sync

119923e

See: hzeller@47da829 hzeller@ed0f527

clean up evented variables

ea634d2

moderate OpenHome playlist upodates

7255dae

hzeller reviewed Nov 11, 2013
View reviewed changes

Andrey Demenev added 5 commits November 14, 2013 01:07

stop playback before changing track

24bc5cc

add TODO for VariableContainer_new() arguments

4e99464

add TODO for resolving modules cross-dependecies

3319068

clean up comments

28ae891

copy-paste is a BAD THING

a2fd33b

Andrey Demenev added 5 commits November 23, 2013 03:45

primitive events moderation

e1269d6

in service descriptor, move var names to metadata

fa65efd

that is the right place where variable names belong, and this allows to not pass complete service descriptor to variable container constructor

remove oudated TODO

b2a0aa2

more clear names for variable eventablity

ad75b67

distinguish SENDEVENT_YES from SENDEVENT_LASTCHANGE

hzeller mentioned this pull request Oct 22, 2019

Openhome support #186

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenHome services #45

OpenHome services #45

ademenev commented Oct 29, 2013

hzeller Nov 11, 2013

hzeller commented Nov 11, 2013

ademenev commented Nov 13, 2013

ademenev commented Nov 13, 2013

hzeller commented Nov 22, 2013

ademenev commented Nov 22, 2013

hzeller commented Dec 11, 2013

triplem commented Jan 8, 2014

triplem commented Jan 17, 2014

christiscarborough commented Feb 5, 2014

hzeller commented Feb 5, 2014

christiscarborough commented Feb 5, 2014

whyman commented Sep 12, 2017

sqlfairy commented Oct 6, 2019 •

edited

Loading

hzeller commented Oct 22, 2019

christiscarborough commented Oct 22, 2019 via email

OpenHome services #45

Are you sure you want to change the base?

OpenHome services #45

Conversation

ademenev commented Oct 29, 2013

hzeller Nov 11, 2013

Choose a reason for hiding this comment

hzeller commented Nov 11, 2013

ademenev commented Nov 13, 2013

ademenev commented Nov 13, 2013

hzeller commented Nov 22, 2013

ademenev commented Nov 22, 2013

hzeller commented Dec 11, 2013

triplem commented Jan 8, 2014

triplem commented Jan 17, 2014

christiscarborough commented Feb 5, 2014

hzeller commented Feb 5, 2014

christiscarborough commented Feb 5, 2014

whyman commented Sep 12, 2017

sqlfairy commented Oct 6, 2019 • edited Loading

hzeller commented Oct 22, 2019

christiscarborough commented Oct 22, 2019 via email

sqlfairy commented Oct 6, 2019 •

edited

Loading