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 an Audio display class #4302

Merged
merged 14 commits into from Oct 23, 2013
Merged

Add an Audio display class #4302

merged 14 commits into from Oct 23, 2013

Conversation

davidosterberg
Copy link
Contributor

This is a first attempt at addressing issue #4241.
In fact it is my first attempt at contributing back so please bear with me for a bit :)

This is a first attempt at addressing issue 4241.
@davidosterberg
Copy link
Contributor Author

Hmm. I clearly have to normalize the data without using numpy. That I can do.

import base64
import struct
from io import BytesIO
import wave
Copy link
Member

Choose a reason for hiding this comment

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

Let's put this import inside the methods that use it - just in case someone has built Python without this module.

@davidosterberg
Copy link
Contributor Author

I have tested this functionality on several platforms and there are some serious problems with browser compatibility. The state of my testing is that

  • Android's Chrome browser doesn't seem to support embedding data in the URI for the audio control.
  • The WAV format doesn't work on Internet Explorer 9
  • Only MP3 works in Safari on iOS 7
  • Almost everything works on desktop Chrome, Firefox and Safari.

Should I investigate what needs to be done to have better support on mobile?

@mgaitan
Copy link
Contributor

mgaitan commented Oct 1, 2013

I'm not aware of what are the policies for the usage of extra js resources, but if it would be possible, this is a great option:

https://github.com/happyworm/jplayer/

also It's available through a Cdn for "on demand" (async) load.

http://cdnjs.com/libraries/jplayer/

@ellisonbg
Copy link
Member

The main targets for the notebook are the desktop versions of Chrome, FF
and Safari and IE 10, so I am not too worried about the others at this
point. I don't think it is worth adding an external dep for this.

On Tue, Oct 1, 2013 at 1:18 PM, Martín Gaitán notifications@github.comwrote:

I'm not aware of what are the policies for the usage of extra js
resources, but if it would be possible, this is a great option:

https://github.com/happyworm/jplayer/

also It's available through a Cdn for "on demand" (async) load.

http://cdnjs.com/libraries/jplayer/


Reply to this email directly or view it on GitHubhttps://github.com//pull/4302#issuecomment-25485574
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@davidosterberg
Copy link
Contributor Author

I have noted that performance is an issue for longer signals. For instance, just normalizing a 60 second long high fidelity signal (44100 fps) takes about 8 seconds on my machine. This goes down to 500 milliseconds if numpy is used. What do you think about doing something like this?

    try:
        import numpy as np
        data = np.array(data,dtype=float)
        scaled = list(np.int16(data/np.max(np.abs(data))*32767))
    except:
        maxabsvalue = float(max(map(abs,data)))
        scaled = map(lambda x: int(x/maxabsvalue*32767), data)

@ahmadia
Copy link
Contributor

ahmadia commented Oct 2, 2013

That's a good idea David :) It's a little more Pythonic to do two things:

  • create a scale_func that either uses NumPy or doesn't, based on whether
    NumPy is available:
  • explicitly catch an ImportError, unless you have reason to believe that
    there are situations where NumPy might be available and still fail.

(Awesome looking first contribution by the way)

On Wed, Oct 2, 2013 at 3:44 PM, David Österberg notifications@github.comwrote:

I have noted that performance is an issue for longer signals. For
instance, just normalizing a 60 second long high fidelity signal (44100
fps) takes about 8 seconds on my machine. This goes down to 500
milliseconds if numpy is used. What do you think about doing something like
this?

try:
    import numpy as np
    data = np.array(data,dtype=float)
    maxabsvalue = np.max(np.abs(data))
    scaled = list(np.int16(data/np.max(np.abs(data))*32767))
except:
    maxabsvalue = float(max(map(abs,data)))
    scaled = map(lambda x: int(x/maxabsvalue*32767), data)


Reply to this email directly or view it on GitHubhttps://github.com//pull/4302#issuecomment-25570429
.

@takluyver
Copy link
Member

I'm wary of having a fallback that's significantly slower than the primary method, because it leads people to rely on something that becomes unreasonably slow when somebody runs it without the supporting tools. I'd be inclined to make numpy required for passing in raw data - the docstring already says that it expects a numpy array.

In short: I'd prefer a clear failure explaining that it needs numpy to an unexplained hang of several seconds.

@davidosterberg
Copy link
Contributor Author

Hmm. I think it is polite not to require numpy given that it isn't a hard dependency of IPython. I concede that if we do support a list of numbers as input we should say so in the docstring otherwise there is no point.

So what if we always use the fallback if the user passes in a list and attempt to use numpy when a numpy array is passed. I think it is OK if it is slower in the rare edgecase where the user passes in a numpy array but doesn't have the numpy module.

@takluyver
Copy link
Member

By the sounds of it, it's impractically slow without numpy, and I doubt anyone would try to create raw sound data without numpy. But yes, it's an acceptable compromise to allow lists but always use the slow normalisation for them, because that way the same notebook will always be about equally slow for the author and the user.

There's no way to create a numpy array if you don't have the numpy module, thankfully. You might be able to prevent its import, but that's just one of many things that we'd consider 'being wilfully awkward', so we don't need to plan for that.

@minrk
Copy link
Member

minrk commented Oct 2, 2013

I think it's a reasonable expectation that if someone is constructing an array of data to be encoded into a wav, it is a numpy array. They can always encode and pass wav data themselves, if they don't have numpy.

@ellisonbg
Copy link
Member

Here is what I would recommend:

  • Use numpy if it is installed.
  • Otherwise use a list. But check to make sure the list is not over some
    reasonable length. If it is, raise a suitable exception with a message...

On Wed, Oct 2, 2013 at 2:39 PM, Min RK notifications@github.com wrote:

I think it's a reasonable expectation that if someone is constructing an
array of data to be encoded into a wav, it is a numpy array. They can
always encode and pass wav data themselves, if they don't have numpy.


Reply to this email directly or view it on GitHubhttps://github.com//pull/4302#issuecomment-25579440
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@davidosterberg
Copy link
Contributor Author

I've added a simple example in the rich display example notebook. Also, I have installed python 3.3 and tested my code. Lo and behold, my code worked! I guess I can finally switch to python3 now.

I'm still wondering about this fallback performance stuff. After testing a bit I have realized that the generic normalizing method is actually only very slow if it is run on a numpy array. Hence if the pure python normalization is only used as a fallback it is only about 2x slower than the numpy version. It is not impractically slow for any data that you'd wanna embed in the notebook, in my opinion. I am fine either way. As @minrk said, most people will have numpy. @ellisonbg @takluyver what do you think?

I don't think it is necessary to have a limit on the list length as @ellisonbg suggested. I tested embedding a 20 minute 44100 Hz signal without problem. That is an array of length 53e6. I think the user knows it will take a few seconds to embed that.

When that question is resolved and unless there are other things I think this is ready to be reviewed.

@takluyver
Copy link
Member

If it's only ~2x slower, I'm happy to have the fallback.

@minrk
Copy link
Member

minrk commented Oct 13, 2013

It's not so much the performance that bothers me, it's adding extra code to handle a case that I don't expect to really be an issue. But if it's already there and the performance isn't too bad, then I don't see any great cost in leaving it.

If we do encounter any issues with it, I would be inclined to remove the support rather than fix it.

@takluyver
Copy link
Member

Is there anything else we want to do with this before merging it?

@ellisonbg
Copy link
Member

I think it is ready to go. I tested the examples and it works as expected. @davidosterberg anything else you want to do on this before we merge?

@davidosterberg
Copy link
Contributor Author

I am happy with the class.

The example notebook could be improved. Now it links to a rather arbitrary wav file from the web. Also, I did not commit the cell output in the notebook. So someone should run the notebook so nbviewer shows the right thing. Should I do that?

@ellisonbg
Copy link
Member

Yes, why don't you embed the audio in the cell so it works on nbviewer and
then I will merge. Thanks for working on this.

On Wed, Oct 23, 2013 at 1:18 PM, David Österberg
notifications@github.comwrote:

I am happy with the class.

The example notebook could be improved. Now it links to a rather arbitrary
wav file from the web. Also, I did not commit the cell output in the
notebook. So someone should run the notebook so nbviewer shows the right
thing. Should I do that?


Reply to this email directly or view it on GitHubhttps://github.com//pull/4302#issuecomment-26941845
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@takluyver
Copy link
Member

Great, I'll merge this. I'm trying to land Python-based pull requests so I can start a branch to remove 2to3 from our Python 3 support.

takluyver added a commit that referenced this pull request Oct 23, 2013
@takluyver takluyver merged commit 0609662 into ipython:master Oct 23, 2013
@Carreau Carreau mentioned this pull request Nov 1, 2013
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@nils-werner
Copy link

I'm happy to see my initial attempt was picked up and brought to a state in which it could get merged 👍

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.

None yet

7 participants