-
Notifications
You must be signed in to change notification settings - Fork 95
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
Qt VideoEditor #745
Qt VideoEditor #745
Conversation
Sorry this problem was fixed when I built with the latest version of pyqt5 instead of pyside2. pyqt5 had to be installed from pip tho...... there were some QAudio attribute errors when using the edm version of pyqt5. going off this, there was a comment suggesting
|
FEAT: QT VideoEditor pulling frames
Some quick drive-by comments: on Linux (Ubuntu 18.04), the player refused to play video with EDM-installed PyQt5 and PySide2, with the error message 'no service found for - "org.qt-project.qt.mediaplayer"'. Installing pyqt5 from PyPI, as Jack suggested, made the video playable, but |
Some updates: @mdsmarte and I looked into running the video editor on Windows and Linux, and here are some findings. On Linux, the EDM-installed PyQt5 does not have the multimedia plugin and fails to play video. Using PyQt5 from PyPI instead works. As reported above, playback is very choppy both with local and remote videos. Commenting out the following line makes playback smooth again (comparable to playing the video in a native media player): traitsui/traitsui/qt4/video_editor.py Line 343 in 2b8620c
In Windows 10, I see the same kind of choppiness, with the same workaround. Playback works out of the box with both the EDM and the PyPI version of PyQt5. I did have to install the necessary codecs. |
@aaronayres35 Weren't you seeing intermittent failures around the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corranwebster I'm happy to finish this PR but I have several questions first.
# Signal handlers ------------------------------------------------------- | ||
|
||
def _state_changed_emitted(self, state): | ||
self.state = reversed_state_map[state] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be wrapped by a self.updating_value()
context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly - again, don't have the context loaded into my head, so it you think it is needed, do it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving this alone for now. It's not as straightforward as position
was.
state='state', | ||
position='position', | ||
duration='duration', | ||
video_error='error', | ||
media_status='status', | ||
buffer='buffer', | ||
muted='muted', | ||
volume='volume', | ||
playback_rate='playback_rate', | ||
image_fun='image_fun' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice not to require users to provide traits for all of this editor state. How do the trait declarations in VideoEditor
need to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This editor was designed to be low-level and makes heavy use of the sync_name
metadata, behaviour is explained here:
Lines 594 to 617 in 7f3dbaf
Initializes and synchronizes (as needed) editor traits with the | |
value of corresponding factory traits. The name of the factory | |
trait and the editor trait must match and the factory trait needs | |
to have ``sync_value`` metadata set. The strategy followed is: | |
- for each factory trait with ``sync_value`` metadata: | |
1. if the value is a :class:`ContextValue` instance then | |
call :meth:`sync_value` with the ``name`` from the | |
context value. | |
2. if the trait has ``sync_name`` metadata, look at the | |
referenced trait value and if it is a non-empty string | |
then use this value as the name of the value in the | |
context. | |
3. otherwise initialize the current value of the factory | |
trait to the corresponding value of the editor. | |
- synchronization mode in cases 1 and 2 is taken from the | |
``sync_value`` metadata of the editor trait first and then | |
the ``sync_value`` metadata of the factory trait if that is | |
empty. | |
- if the value is a container type, then the `is_list` metadata | |
is set to |
In particular users of the code are responsible for setting up any play/pause buttons, muting etc. If they are left empty, then the default values from the Editor will be used, I think, so we should make sure that those are sensible.
If we want to allow constant values (eg. setting muted
to True
and not synchronising), then we might need to have muted
and muted_name
traits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they are left empty, then the default values from the Editor will be used
This is true, but since the default values are empty strings it causes a TraitError
if nothing is passed.
I don't understand the mechanisms in TraitsUI well enough to change this [even after some experimentation], so I'm going to leave it alone in this PR.
Yes, feel free to run with this as needed (time permitting), making whatever changes you think make sense - it may make sense to strip out the image transform stuff, particularly if it affects performance. |
This is ready for review if someone would like to take a look |
Ah yeah I was, although they weren't on this specific test IIRC. Issue was #1551 which was closed by #1553 (although that may have jumped the gun and perhaps it needs to be re-opened) |
Hmm... Yeah. Speculative fixes can be risky. |
If there is more time available (which sounds like no), I had thought about having both video player and image transformed video player coded up, and if the user does not pass an image transformation function, it will fall back to the regular video player implementation. |
I'm pretty sure that's already implemented. |
@jwiggins can you please update the PR description - which probably is out-of-date. |
I'm not sure what should be changed in the description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM with a bunch of comments.
I guess the video editor isn't ready for prime time yet so we will keep it out of traitsui.api
for now. When we do decide to include it in the api
module though, we will probably want to also include the video editor-specific traits defined in the factory module.
# Editor interface | ||
# ------------------------------------------------------------------------ | ||
|
||
def init(self, parent): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're passing in the parent
but we are never using it. Not sure if we need to be though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be a common pattern in TraitsUI Qt code...
traitsui/qt4/video_editor.py
Outdated
self.media_player.setVideoOutput(self.control) | ||
|
||
def update_to_functional(self): | ||
self.control = ImageWidget(image_fun=self.image_fun) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be setting widget
here like we are with VideoSurface
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you're asking. VideoSurface
takes a widget
argument, which is equal to self.control
in this method.
Update : This doesn't work as expected with pyside2 either I LGTM'ed too early. I ran the example on windows and it isnt working as expected. win + cp36 + pyqt5 5.14.2-4 and qt 5.12.6-10
Note that I also see the following on the command line when i run the example
|
@rahulporuri I believe you need to install codecs on Windows in order for Qt Multimedia to work properly. |
One last comment that is not necessary: We had an issue in the past where our vp9 formatted files were not playing. An example file: It might have been that our file did not actually have an extension |
Good suggestion @JackDra. I'll do that here for now and we can move the info to the appropriate place later. Most of this info was collected by @mdsmarte, but was attached to a private issue. Windows: MacOS: Linux: |
@rahulporuri If you have no further objections, I'll merge this today. |
Please go ahead.
We can move this information into the user documentation separately. @jwiggins are you hoping for a new traitsui release soon? |
UPDATE : Please ignore. This is the whole point of the custom
Rotated video![well-that-is-unfornatute](https://user-images.githubusercontent.com/1926457/115021371-6968bc80-9eab-11eb-9e6b-3d6ca21392a6.gif) |
|
The rotation is the expected behavior. It's showing that users can manipulate the stream via the |
thanks a lot @jwiggins for pushing this across the finish line. please go ahead and merge. |
Thanks everyone, this is a nice addition to the library. |
@jwiggins ^ . We are planning on making a release early Feb. We can make one sooner if that's useful. |
Talk to @mdsmarte and/or @ffishburn. Their customer project is an intended user of this feature. |
This is a viewer-only Qt
VideoEditor
. This only presents the video (provided as a URL), not any associated controls like play buttons, position sliders, etc. but you can bind traits to player state and then build a custom interface around them - the example gives a good idea of how to do this.There are some glitches on MacOS retina displays which appear to be a problem with the underlying QtMultimedia implementation.
There is no Wx implementation. There are no tests yet.