gmrender-resurrect
gmrender-resurrect copied to clipboard
hzeller
OpenHome services
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
Wonderful, thanks for your contribution!
The changes are touching quite a few places and I want to review the change carefully, so I'll probably do the final merge on the weekend when I have some time. Expect some questions for clarification on this thread. Also, I might ask for little changes before the merge (but in general, your code looks high quality from what I can tell looking at some of your submits).
In the meantime, I'll try to patch it into a local client and play around with it.
What is a good UPnP controller to test this out ? I usually use BubbleUPnP on Android as a controller, can that handle the playlists ?
Oh, can you merge with the current HEAD of my master branch, so that I have a clean, mergeable state ? I did some minor changes in the last couple of days, but I think mostly in the README.md, so shouldn't be conflicting.
(mmh, though it looks like, you did the merge. Maybe github is just confused)
I still have to figure out how to use git and github. Possibly my manual merges created conflicts, so automatic merge is not possible now
I use BubbleUPnP and Kinsky for testing, both can control OpenHome renderers. I like BubbluUPnP more.
So from my initial merge attempt (that failed), it looks like there are whiltespace changes. e.g. in variable-container.h, the original file has 2 tabs and one 4 spaces for the stuff below UPnPLastChangeCollector_new(variable_container_t *variable_container, const char *event_xml_namespac, struct upnp_device *upnp_device, const char *service_id);
.. while in the new file, there are only 2 tabs. In general, your editor seems to only generate tabs here, no tabs+spaces, so a lot of indentations are off. Is it possible to change your editor to change this ? Right now, there is a mix of TABs (always 8 wide) and spaces to 'fill' the rest that is not divisible by 8.
(in general, these tabs+space should all be replaced by spaces; this is partially inherited code; I was holding off though changing that so far because that causes nasty merge conflicts).
(If it is hard to change now, don't worry about it now, I'll have a look at the code anyway first)
Of course I can change the indentation now and update the pull request, but I am afraid that will just create even more conflicts. So possibly you have to make manual merge for now. Once it is done, I can do the work of tidying up the whitespace stuff. I for sure will continue to work on the code, and having uniform editor setup is important.
Sounds good. So after this merge, we should probably do a cleanup once that replaces all TABs with spaces and go from there. I am fine with manual merging now.
Another question: do you have a link to the OpenHome specs that you used as reference for the implementation ?
http://www.openhome.org/wiki/Av:Developer - then follow links for description of services http://docs.linn.co.uk/wiki/index.php/Developer:Davaar - basically the same, but with additional pieces of useful info (for example, openhome.org does not state that IdArray is big endian)
Unfortunately not yet - stuff came up last weekend, so I didn't have any time. This weekend the Sunday should be free to work on the review.
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.
I agree that there is a possible race condition. I think the changes related to notify_mutex I propose can solve this
@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
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)
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.
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....
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.
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
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.
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 ?
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