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

Implement Video and Audio widgets #2162

Merged

Conversation

@martinRenou
Copy link
Member

@martinRenou martinRenou commented Aug 2, 2018

Implement a Video widget, at first I wanted to synchronize the progress of the video between views, but it turned out that it's quite difficult to do it. There is a timeupdate event on HTML video elements, but it's not triggered often enough to synchronize the views.

Edit: Also implement an Audio widget. Image, Video and Audio widgets have a common base class _Media

@martinRenou
Copy link
Member Author

@martinRenou martinRenou commented Aug 2, 2018

Loading

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Aug 2, 2018

Lgtm. For the tests to pass you will need to update the spec file as per the test error message.

Loading

@jasongrout jasongrout added this to the Minor release milestone Aug 2, 2018
@martinRenou
Copy link
Member Author

@martinRenou martinRenou commented Aug 3, 2018

I also plan to make an Audio widget and it might be better to implement a base class named Media for widgets like Image, Video and Audio. The code will be quite the same for those three widgets.

Loading

@martinRenou martinRenou changed the title Implement Video widget Implement Video and Audio widgets Aug 3, 2018
@martinRenou martinRenou force-pushed the implement_video_widget branch from facf5b6 to 81b616b Aug 3, 2018
@martinRenou martinRenou force-pushed the implement_video_widget branch from 81b616b to cbb67f3 Aug 3, 2018
@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Aug 3, 2018

This looks good to merge.

Leaving it open for a bit so that jason or other people can comment.

Loading

@maartenbreddels
Copy link
Member

@maartenbreddels maartenbreddels commented Aug 3, 2018

Looking good!
I guess the Image unittests covers basically everything?
The only open issue I have is that a video can have multiple urls, for instance, if it does not support webm, it can take a mp4 file. Should we support this?

Loading

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Aug 3, 2018

Using multiple source tags would require providing different data for each case. That would completely modify the model to be a lot more complex (like a dict: format: data)... Not sure that it would be good for usability.

Loading

@martinRenou
Copy link
Member Author

@martinRenou martinRenou commented Aug 3, 2018

Also @maartenbreddels we may want to implement something like Video.from_download("bla.mp4") ?

Loading

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Aug 3, 2018

After the in-person discussion with @maartenbreddels and @martinRenou we decided to not add the possibility to have extra sources for the video widgets.

I think that eventually, we will add a sources attribute to media in ipywidgets 8, that will apply to

  • video
  • audio
  • and picture, which should maybe underlie the Image widget?

Loading

@maartenbreddels
Copy link
Member

@maartenbreddels maartenbreddels commented Aug 3, 2018

For Python2 and 3 I use this:

try:
    from urllib import urlopen  # py2
except ImportError:
    from urllib.request import urlopen  # py3
value = urlopen(url).read()

Except that Python2 doesn't give an exception on a 404, and will happily feed you the 404 page. For Python 3 you get a proper exception. For the convenience of this, I suggest we add a from_download method.

Loading

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Aug 3, 2018

We discussed further on the means to download the media

  • to the kernel
  • to the client

and APIs.

This should probably be another PR.

Merging as per agreement with @jasongrout and @maartenbreddels.

Loading

@SylvainCorlay SylvainCorlay merged commit 2ce067a into jupyter-widgets:master Aug 3, 2018
1 check passed
Loading
@SylvainCorlay SylvainCorlay removed this from the Minor release milestone Aug 7, 2018
@SylvainCorlay SylvainCorlay added this to the 7.4 milestone Aug 7, 2018
@lock lock bot added the resolved-locked label May 21, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants