Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom album art size #121

Merged
merged 26 commits into from
Jul 19, 2021

Conversation

kjk2010
Copy link
Contributor

@kjk2010 kjk2010 commented May 23, 2021

This PR adds a new API method for requesting custom album art image sizes in JPEG format. This method is used by the keypad controllers I have been developing, but could also be used for other clients.

It currently only supports local input and Internet Radio album art. I've been unsuccessful in using Pandora on my dev Pi 2, and the current Spotify client doesn't appear to include any metadata or album art information.

The included rca_inputs.jpg file is converted from the original SVG file. This was done because the 'pillow' library doesn't support converting from SVG.

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2021

Codecov Report

Merging #121 (bf294e4) into master (ea7d2e2) will increase coverage by 0.13%.
The diff coverage is 60.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   59.57%   59.70%   +0.13%     
==========================================
  Files          11       11              
  Lines        2031     2065      +34     
==========================================
+ Hits         1210     1233      +23     
- Misses        821      832      +11     
Flag Coverage Δ
unittests 59.70% <60.39%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
amplipi/streams.py 48.45% <42.85%> (-0.11%) ⬇️
amplipi/app.py 84.66% <60.00%> (-2.84%) ⬇️
amplipi/ctrl.py 77.82% <100.00%> (+0.24%) ⬆️
amplipi/models.py 93.75% <100.00%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea7d2e2...bf294e4. Read the comment docs.

@linknum23
Copy link
Contributor

I really like how simple this addition looks, nice work!

I will spend more time checking this out after merging in #116, since that PR is a little bit of a tear-up for the flask infrastructure. (There will need to be a couple of minor changes to the API plumbing once that is merged, I can make those to make this easier to merge)

I can also adding Pandora image conversion, we use that a lot over here.

In the meantime I'm trying to think of the best way to add a simple automated test for this addition to the API, do you have any ideas?

@kjk2010
Copy link
Contributor Author

kjk2010 commented May 25, 2021

For tests, I'm thinking a GET request on /api/streams/image/0/120 (should be the rca_local.jpg binary image, Content-Type: image/jpg) and /api/streams/image/{validStreamId}/120 (should be a stream's binary image, also Content-Type: image/jpg).

Thanks!

@linknum23
Copy link
Contributor

I started working on integrating this in, but got pulled in a couple of different ways and have not been able to finish up testing this.

What I got stuck on was how the analog inputs are handled on what is essentially a stream input and this seemed like something that might break in future changes. I know we had discussed how to handle this in the past but I was not able to find the discussion. Did the discussion get deleted?

@kjk2010
Copy link
Contributor Author

kjk2010 commented Jul 1, 2021

There was some additional discussion of this feature on the POE Keypad discussion: #77 (comment)

I'd like for the analog input images to eventually be custom configured so users can upload their own images for different inputs, and possibly even be set to call an external API in the event that the analog source device can provide the metadata and/or album art.

In the mean time, I was just having this PR send the rca_inputs.jpg file for analog inputs.

@linknum23
Copy link
Contributor

linknum23 commented Jul 7, 2021

I have a protoype of this working in my branch. Mind if I force push it into your branch? I could also make a separate PR if that makes more sense.

@linknum23 linknum23 force-pushed the add_custom_albumart_size branch from 1324875 to 0088d3f Compare July 7, 2021 20:16
@linknum23
Copy link
Contributor

Ooops, looks like i forced pushed to the wrong origin, sorry about that.

I changed the endpoint to be /api/sources/{sid}/image/{height} and made sources hold the metadata of what is currently playing in them.

I know this adds a couple of changes on your side, so I will try to make the changes to the touchscreen code tomorrow and see how it all works together before merging this is. Best case we both get to merge PRs tomorrow.

Feel free to check it out and see if there are any changes you think make sense.

@linknum23 linknum23 requested review from micronova-jb and Lohrer July 7, 2021 20:25
@kjk2010
Copy link
Contributor Author

kjk2010 commented Jul 7, 2021 via email

amplipi/models.py Outdated Show resolved Hide resolved
Copy link
Contributor

@micronova-jb micronova-jb left a comment

Choose a reason for hiding this comment

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

Everything looks great! Once I get answers to my comments specifically, I am good to approve the changes.

Copy link
Collaborator

@Lohrer Lohrer left a comment

Choose a reason for hiding this comment

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

We'll definitely have to update the display once the 'state' is fixed.

amplipi/app.py Outdated Show resolved Hide resolved
amplipi/app.py Show resolved Hide resolved
amplipi/ctrl.py Outdated Show resolved Hide resolved
amplipi/models.py Outdated Show resolved Hide resolved
amplipi/models.py Outdated Show resolved Hide resolved
amplipi/streams.py Show resolved Hide resolved
@linknum23
Copy link
Contributor

This is looking good to go! Album art should work now for any source configuration.

@kjk2010
Copy link
Contributor Author

kjk2010 commented Jul 13, 2021 via email

@linknum23 linknum23 force-pushed the add_custom_albumart_size branch from bf294e4 to f8c01a3 Compare July 19, 2021 14:54
@linknum23 linknum23 merged commit df12681 into micro-nova:master Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants