-
Notifications
You must be signed in to change notification settings - Fork 385
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
Video Player #149
Video Player #149
Conversation
# Conflicts: # tagstudio/src/qt/ts_qt.py
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.
Love the functionality! And sorry for all the code changes in the meantime, there was a recent formatting refactor that probably messed with stuff pretty bad. Thank you for your patience!
Overall I think this is going in a good direction, there's just a few things I've run into or would like to see done:
- When clicking on a new video file, the preview size is always incorrect until I manually grab the resize bar to "refresh" the panel (pictured below)
- Rarely I'm encountering an issue with vertical videos where the only about the middle part of the video is updating during playback, while the rest of the frame area only updates when the panel size changes. This may be related to the first issue.
- The video playback can be kind of intensive sometimes and slows down the program. This is more so an issue stemming from having to stream a potentially high bitrate video file from disk while the program runs. Because of this, I would really like to see an autoplay toggle to stop videos from automatically playing in the preview window. Maybe this could be included in the right-click context menu for the video preview.
- And lastly I'd like to see the mute/unmute status carry over from video to video instead of always defaulting to mute. This doesn't need to persist between sessions, but I think it makes sense for each next video to remember your audio preference.
Thank you again for working on this and getting a pretty cool feature going!
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.
Fixed bugs and added autoplay toggle.
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.
Awesome!! The persistent volume settings seem to work fine, but the autoplay toggle seems to be inconsistent. If I toggle autoplay off followed by clicking on another entry and then clicking back to the first video, then it will autoplay regardless of the setting.
Other than that, just a couple general housekeeping things:
- Remove or comment out the testing log messages (under mouse, over mouse, etc.). Ideally the ffmpeg logs would also be removed, but I'm not even sure if it's possible to turn those off, so I wouldn't worry about those.
- On startup I get the following warning logs:
QGraphicsScene::addItem: item has already been added to this scene
QGraphicsProxyWidget::setWidget: cannot embed widget 0x27f44083a30 which is not a toplevel widget, and is not a child of an embedded widget
QGraphicsProxyWidget::setWidget: cannot embed widget 0x27f44083d90 which is not a toplevel widget, and is not a child of an embedded widget
The first warning seems to stem from line 85 in video_player.py
, however I'm not sure about the other two warnings. I would suggest investigating the causes of these.
I'm also still having this same autoplay issue where it has inconsistency following the setting. |
I can't seem to recreate the autoplay issue. It is working just fine on my end after multiple tries. |
Here's a clip of me recreating the issue on my end: 2024-05-10.12-57-17-1.mp4It usually takes the form of the video autoplaying regardless of the setting when clicking off then back to the file. It seems to also be affected by whether or not the last file clicked on was also a video. |
I did checkout the branch to see why is the CI failing and I noticed one unrelated bug:
|
you can check this commit how to fix the mypy issues: 8e62147 |
radius=12, | ||
fill=(0, 0, 0, 0), | ||
) | ||
mask = mask.getchannel("A").toqpixmap() |
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.
mask
has originally type PIL.Image
, but here it's changed to QPixmap
, so it would be really better to use different variable for one of these, so the types wont clash
# return super().resizeEvent(event)\ | ||
|
||
def inputMethodEvent(self, event: QInputMethodEvent) -> None: | ||
return |
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.
is this intentional?
The play/pause indicator not updating after autoplaying seems to be mostly fixed, except in the case of when the video autoplays while the user's mouse is hovered over the indicator. |
# Conflicts: # tagstudio/src/qt/widgets/preview_panel.py
# Conflicts: # tagstudio/src/qt/widgets/preview_panel.py
It appears one of the commits from main fixed the autoplay issue.
This seems like a nice feature to add to this project, looking forward to it! |
@DrRetro2033 Thank you again for all your work on this! |
Did a redo on the video player, as it was in GitHub conflict hell. So it should work now, plus I even added some rounded corners to it as well.