search

hzeller / gmrender-resurrect

Resource efficient UPnP/DLNA renderer, optimal for Raspberry Pi, CuBox or a general MediaServer. Fork of GMediaRenderer to add some features to make it usable.
GNU General Public License v2.0
841 stars 204 forks source link

Support for libupnp v1.8.x & v1.6.x #190

Closed mill1000 closed 4 years ago

mill1000 commented 4 years ago

Adds support for libupnp-1.8.x while maintaining support for libupnp-1.6.x via upnp_compat.h by stealing macros from libupnp-1.6.26.

Supersedes and closes #173 & closes #170

Haven't run many tests beyond compilation so leaving this as a draft PR.

Compiled successfully against

Also close #176 & close #148

ashkitten commented 4 years ago

i know this is still a draft pr, but it seems that of the 3 current prs to support libupnp 1.8, this and #173 both do not correctly update playing status in the client. #170 does seem to do that properly, but i don't know the code well enough to say what it's doing differently so i hope my feedback helps

mill1000 commented 4 years ago

Good to know. Thanks. I'll be able to test the changes properly tonight.

Any chance you could share what version of libupnp you compiled against?

On Sat, Oct 19, 2019, 11:41 AM ash lea notifications@github.com wrote:

i know this is still a draft pr, but it seems that of the 3 current prs to support libupnp 1.8, this and #173 https://github.com/hzeller/gmrender-resurrect/pull/173 both do not correctly update playing status in the client. #170 https://github.com/hzeller/gmrender-resurrect/pull/170 does seem to do that properly, but i don't know the code well enough to say what it's doing differently so i hope my feedback helps

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hzeller/gmrender-resurrect/pull/190?email_source=notifications&email_token=ABU2STUAXUFPZRVSZZ6EKEDQPNBDPA5CNFSM4JCM4ZM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBXYC3A#issuecomment-544178540, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABU2STQ2D3FTOYQ4EECBZHDQPNBDPANCNFSM4JCM4ZMQ .

ashkitten commented 4 years ago

i'm using 1.8.4, but the issue also happens with 1.6.21 using this pr

hzeller commented 4 years ago

Thanks, I'll have a look at this later. There were a couple of upnp 1.8 PRs before, but from what I recall they were all looking like quick hacks 'just to get it work', so I had some local modifications sitting in my client to get this more properly merged.

I'll have a look at your PR later tonight; it sounds like you addressed that it is possible to compile with different versions of upnp, which is good (because there are still many devices out there that have the old version installed sadly, so compatiblity mode is good).

mill1000 commented 4 years ago

@ashkitten Thanks. I did a quick diff between PR #170 & #173. I had assumed there implemented the v1.8.x support the same way. This does not appear to be true.

What client are you using and how did you test?

ashkitten commented 4 years ago

i'm using bubbleupnp on android, tested playing a song from my phone's internal storage. it started playing on my computer, but didn't show as playing on my phone

mill1000 commented 4 years ago

Ok. I turned on ACTION_LOGGING and it looks like some of the UPNP actions are getting dropped.

mill1000 commented 4 years ago

@ashkitten I think I've fixed the issue. Let me know if its working for you.

ashkitten commented 4 years ago

yes, it seems to have worked. thank you!

mill1000 commented 4 years ago

Add some additional macros to eliminate warnings due to changes in the callback prototypes

hzeller commented 4 years ago

Nice! the pull request is still marked as draft, are you intending more changes ?

mill1000 commented 4 years ago

I've made some commits with regards to formatting within upnp_device. Some of the lines breaks seems extraneous, and for my eyes make the code harder to read.

However, if this is your preferred style, I will revert these changes

hzeller commented 4 years ago

I try to keep things within 80 character limit. I've inherited in this project the tab-indentation, so this makes things a little bit long, and means some lines have to be broken.

mill1000 commented 4 years ago

Ah, I'm more of a 120 guys I guess.

It's your project, shall I reformat to 80?

hzeller commented 4 years ago

Yes, for now, please keep it at 80 if possible.

(I have some code-reorg changes in mind, and at that time I'll change it to 2 or 4 spaces.)

mill1000 commented 4 years ago

Ok. Just one or two lines now that are a hair over 80.

hzeller commented 4 years ago

Thanks Tucker! Merged, and with it closed a bunch of partial pull-requests and filed compile bugs.