Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Sonos binding: album cover art #2838

Merged
merged 1 commit into from
Feb 10, 2017
Merged

Sonos binding: album cover art #2838

merged 1 commit into from
Feb 10, 2017

Conversation

lolodomo
Copy link
Contributor

Signed-off-by: Laurent Garnier [email protected]

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 17, 2017

Two new channels have been added.

  • "currentalbumart" attached to an image item. This channell is currently only visible in Paper UI (look for "Camera image").
  • "currentalbumarturl" attached to a string item. The image can be seen in Basic UI and Classic UI (not HABdroid unfortunately) using a sitemap element of type Image with item parameter.

@watou
Copy link
Contributor

watou commented Jan 17, 2017

Are you using the Image widget with the mandatory url= parameter as well as the item= parameter?

@lolodomo
Copy link
Contributor Author

@watou: for the string item, yes I am using url= and item= parameters:
Image url="http://ausdroid.net/wp-content/uploads/2014/12/Sonos-Logo-640x324.jpg" item=SonosChambreAlbumArtUrl

@watou
Copy link
Contributor

watou commented Jan 17, 2017

So HABdroid isn't showing any image for you, or only showing the trippy Sonos logo?

@lolodomo
Copy link
Contributor Author

Just no image at all.

@lolodomo
Copy link
Contributor Author

As it is working in Basic UI and Classic UI, I suppose my item/sitemap declarations are correct.

@watou
Copy link
Contributor

watou commented Jan 17, 2017

I wonder if it's a problem with the proxy servlet, perhaps similar to what was reported here. Hard to tell from the app though, but on a browser you could open the Developer Tools and inspect the HTTP headers of the request/response to look for oddities.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 17, 2017

There is a possible enhancement I can propose that will simplify the code for any binding wanting to deal with image items: adding a new constructor for RawType that will take as parameter an URL. It will download the data from the URL and store it in the internal array of bytes.
So one part of my current code (downloading of the image) will be moved directly in the class RawType.
Any binding wanting to update such a channel will just have to run such a code:

URL url = new URL(...);
State newState = new RawType(url);

Let me know if you are interested and if you think that it is a good idea.

@watou
Copy link
Contributor

watou commented Jan 17, 2017

Not an argument against, but consider multi-megabyte images held in server memory, URLs that never stop streaming, 4xx/5xx or other error responses, etc., and whether this handling can or should be generalized, or better left to the ultimate consuming client.

@lolodomo
Copy link
Contributor Author

Ok, I can agree, that was just an idea I wanted to share with you. By the way, that is only few lines of code to download the image when using IOUtils.
So forget my last proposal.

Note that in the Sonos binding, the image will be download in memory only if the channel is linked. Everyone has the choice and users with not a lot of RAM could decide to rather use the URL advanced channel.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 17, 2017

@watou: I will try to inspect what are the headers provided by Sonos in the response. Your hypothesis is that HABdroid (or the proxy servlet ?) is relying on certain HTTP headers and less compatible than Chrome or Firefox browsers ?

@watou
Copy link
Contributor

watou commented Jan 17, 2017

Your hypothesis is that HABdroid is relying on certain HTTP headers

That depends on whether you can see images at all in your HABdroid. I've not looked at the HABdroid code, but it might be more restrictive in fetching/handling images that the Android System WebView.

do you think I should set some headers in my request ?

No; it sounds like a HABdroid deficiency. Can you see any images from other sources in HABdroid? In my setup I've always seen expected images from Image widgets.

I would be curious if you could test #2753 since it uses a more thorough proxy implementation, to see if that makes any difference. You would have to build it, disable the equivalent bundle and add it to addons.

@lolodomo
Copy link
Contributor Author

@watou: I will try with other Image elements just using the url parameter and let you know if it is at least working sometyimes.
And I will try to compile and use your new proxy servlet.

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo
Copy link
Contributor Author

I have now documented the two new channels (in README.md).

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 17, 2017

@watou : here is what I can find using Firefox (and Basic UI):
Request URL: http://192.168.1.xx:8080/proxy?sitemap=home.sitemap&widgetId=020200&t=1484686264750
Request headers:

Host: 192.168.1.xx:8080
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:50.0) Gecko/xxxxxxxx Firefox/50.0
Accept: */*
Accept-Language: fr,fr-FR;q=0.8,en-US;q=0.5,en;q=0.3
Accept-Encoding: gzip, deflate
Referer: http://192.168.1.xx:8080/basicui/app?w=0202&sitemap=home
Connection: keep-alive

Response headers:

Connection: close
Content-Length: 23656
Date: Tue, 17 Jan 2017 20:51:04 GMT
Server: Linux UPnP/1.0 Sonos/34.7-xxxxxx (ZPS5)

What we can notice is that there is no "Content-Type" in the response headers. Is it what could explain the no display in HABdroid ?

@lolodomo
Copy link
Contributor Author

Could it be the proxy servlet that suppress the Content-Type header ?

@watou
Copy link
Contributor

watou commented Jan 17, 2017

no "Content-Type" in the response headers. Is it what could explain the no display in HABdroid ?

Most definitely a likely suspect. The response headers should have a Content-Type. I think modern browsers are smart enough to derive the image type from the raw image data, while the app may not have this ability. This is another reason I'm wary of RawType being used for image data, as without the content type, it's relying too much on browser cleverness to figure out how to interpret the raw data.

I wonder if the Sonos people could make an effort to fix their firmware to return proper HTTP response headers for cover art? The proxy servlet would not be the place to augment the response headers to compensate for this lack of information. If the theory is founded, it's conceivable that HABdroid could possibly become as "smart" as browsers in deriving the image type from the raw data. Maybe.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 17, 2017

Very interesting, if I select the line-in input, the default image is used (the one in my url= parameter) and in this case I see the picture in HABdroid. I can see using the browser development tools that in this case there is a Content-Type header in the response. So the problem is clear, Sonos is not providing the Content-Type header in the response and this makes HABdroid not displaying the picture.
I will update the issue in HABdroid repo and we can forget this problem in this PR.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 17, 2017

This leads to one of my unanswered questions: what will happen for UI that will have an array of bytes (RawType) without knowing what image format it is ? It is apparently correctly handled by WEB browsers (Paper UI) but this will not be a difficulty for HADdroid ?

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 17, 2017

Just a last word, the cover art are displayed even in HABdroid if I play an album from my local library but it does not work when I use Tune-in or Spotify services.

@watou
Copy link
Contributor

watou commented Jan 17, 2017

Excellent diagnosis, @lolodomo. To summarize: HABdroid ought to be made smarter (if possible) to sniff out image types from raw image data, when Content-Type isn't provided, like browsers do, but ideally the source of the image should provide the Content-Type and be delivered all the way to the client. Correct?

@watou
Copy link
Contributor

watou commented Jan 18, 2017

I have some doubts about my summary above. This line in the Android client shows that the image type is being figured out by the raw data after all. To me, #2734 (proxy servlet bug copying headers) looks more likely. Since I don't have the Sonos hardware you do, I can't test my theory. @lolodomo, could you try to build and test #2753 to see if it makes any difference? (I had to add a Directory to the Target Platform in the Eclipse IDE for OH2 that included org.eclipse.jetty.proxy_9.2.12.v20150709.jar in order to test my changes in #2753.) Or do a similar diagnosis to what @marcelrv did in #2734 to see if a blank line or other strangeness appears in the header section?

@lolodomo
Copy link
Contributor Author

@watou: I compiled your branch but unfortunately I can't find your jetty proxy jar file. In my target directory, I can find only org.eclipse.smarthome.ui-0.9.0-SNAPSHOT.jar. Is it the jar file to put in my addons folder ?

@watou
Copy link
Contributor

watou commented Jan 18, 2017

I did my testing for the PR in the IDE, but you ought to be able to add this JAR to you addons folder as well as use the org.eclipse.smarthome.ui-0.9.0-SNAPSHOT.jar you built. It can be quite a challenge to test changes to SmartHome under OH2, but there might be shortcuts I just don't know yet. 🙂

@lolodomo
Copy link
Contributor Author

@kgoderis: could you please review my change ?

@lolodomo
Copy link
Contributor Author

@watou : unfortunately, I cannot start your bundle. After uninstallaing the default bundle and trying to start the new one, I got this error:

openhab> bundle:list | grep UI
137 | Active    |  80 | 0.9.0.b3              | Eclipse SmartHome UI Icons
170 | Active    |  80 | 2.0.0.RC1             | openHAB Dashboard UI
182 | Active    |  80 | 0.9.0.b3              | Eclipse SmartHome Basic UI, Fragments: 193
184 | Active    |  80 | 0.9.0.b3              | Eclipse SmartHome Paper UI, Fragments: 195
193 | Resolved  |  80 | 2.0.0.RC1             | openHAB Basic UI Fragment, Hosts: 182
195 | Resolved  |  80 | 2.0.0.RC1             | openHAB Paper UI Theme Fragment, Hosts: 184
201 | Active    |  80 | 0.9.0.b3              | Eclipse SmartHome WebApp UI
202 | Active    |  80 | 2.0.0.RC1             | openHAB Classic UI Fragment
216 | Installed |  80 | 0.9.0.201701181851    | Eclipse SmartHome UI
openhab> bundle:start 216
Error executing command: Error executing command on bundles:
        Error starting bundle 216: Could not resolve module: org.eclipse.smarthome.ui [216]
  Unresolved requirement: Import-Package: org.eclipse.jetty.proxy

I could try later in Eclipse IDE.

@watou
Copy link
Contributor

watou commented Jan 21, 2017

Did you try

openhab> bundle:install https://dl.dropboxusercontent.com/u/4286376/org.eclipse.jetty.proxy_9.2.12.v20150709.jar

?

If that still doesn't help resolve the packages, yes please try in Eclipse IDE when you can, and remember to add it to the target platform there. SmartHome still uses Jetty 9.2.9 iirc, but OH2 uses 9.2.12. The PR is compatible with both.

@lolodomo
Copy link
Contributor Author

No I tried to install the bundle org.eclipse.smarthome.ui I compiled from your sources.
As I was not able to find how to re-install the default bundle, I was then constrained to re-install fully RC1. So I will try later with your jar but I don't want to break again immediately my production openHAB server.
If I install your jar, shall I uninstall a running bundle ?

@watou
Copy link
Contributor

watou commented Jan 21, 2017

If I install your jar, shall I uninstall a running bundle ?

I'm no expert in this, but I think you can stop the installed bundle and install and start the one you compiled, which has the dependency on the Jetty proxy bundle as above. Uninstalling the feature is probably a bad idea as you will lose others in the process. Testing on a test server install or via the Eclipse IDE is probably wisest. Sorry I don't have the definitive steps to test it. Back when I made a living programming, OSGi had only just been born. 😄

@sja
Copy link
Contributor

sja commented Feb 2, 2017

@watou

openhab> bundle:install https://dl.dropboxusercontent.com/u/4286376/

I don't know if this dropbox link really just spit out the binary file or does a redirect. Maybe, bundle:install cannot handle that. You can download the file manually and put it into $OH_HOME/conf/html/ and do a bundle:install http://localhost:8080/static/org.eclipse.jetty...

I'd really appreciate that change being merged.

Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works nicely, many thanks!

@kaikreuzer kaikreuzer merged commit b9bbf0b into eclipse-archived:master Feb 10, 2017
@lolodomo lolodomo deleted the sonos_albumart branch February 10, 2017 12:05
@kaikreuzer kaikreuzer modified the milestone: 0.9.0 Jun 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants