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

Fix External subtitle not loading #153

Merged
merged 33 commits into from
May 29, 2020
Merged

Fix External subtitle not loading #153

merged 33 commits into from
May 29, 2020

Conversation

arshubham
Copy link
Collaborator

@arshubham arshubham commented Oct 20, 2019

Set subtitle directly to pipeline, bypassing clutter-gst. A workaround till clutter-gst is fixed. Timeout is needed since pipeline is reset by setting state to Gst.State.NULL and setting progress just after Gst.State.PLAYING has no effect. A better solution for this is welcome.

Fixes #32 , Fixes #63 , Fixes #57

@arshubham arshubham requested review from a team and ryonakano October 20, 2019 19:39
@arshubham arshubham marked this pull request as ready for review October 20, 2019 19:40
src/Window.vala Outdated Show resolved Hide resolved
@ryonakano
Copy link
Contributor

I wonder why ClutterGst.Playback.set_subtitle_uri does not work here, while it works when the app automatically detects a subtitle in the same directory when opening a video file from Files.

Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
@arshubham
Copy link
Collaborator Author

arshubham commented Oct 21, 2019

@ryonakano I believe this is because set_subtitle_uri just sets pipeline.set ("suburi", uri, null) without changing the state first. Here I just copied the logic of autoload_subtitle and it worked perfectly.

Edit: Build is failing due to error in npm i -g @elementaryos/houston

Copy link
Contributor

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

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

Screenshot from 2019-10-22 09-57-30

Now an external subtitle is loaded immediately by dragging and dropping it whether the video is paused or playing, but it's not reflected to the UI; the FileChooserButton external_subtitle_file keeps showing "None".

ryonakano
ryonakano previously approved these changes Oct 22, 2019
Copy link
Contributor

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

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

Seem to work.

@tintou tintou removed their request for review January 15, 2020 10:58
@jeremypw
Copy link
Collaborator

jeremypw commented May 17, 2020

This seems to work well for one film but there is still that crash on starting a new film :-(. I notice that the subtitle file uri is lost if the app is closed then reopened, although it saves the currently playing media. It would be nice to save ands restore the corresponding subtitle uri when they work - but that can be left to another PR. @arshubham are you still working on fixing this PR?

@jeremypw
Copy link
Collaborator

There are a number of issues reported about problems playing a second video for some people. This PR may have exacerbated an underlying problem. I'll have a look.

@jeremypw jeremypw requested a review from ryonakano May 18, 2020 18:43
@jeremypw
Copy link
Collaborator

@ryonakano I have made a number of improvements to this PR. If you are able to give it some testing that would be great. Thanks.

@ryonakano
Copy link
Contributor

Hmm, the issue @arshubham mentioned seems to be still reproducable for me (not happen in master) if I don't pause the first video.

@jeremypw
Copy link
Collaborator

jeremypw commented May 23, 2020

Hmm, I though I tested that - but now I find that with the test .avi video I am using Videos does not crash but becomes unresponsive if i do not pause (OK if video paused before going back). But with another .mkv video I do get a crash. I will look into it further - at least I can now reproduce a crash. Thanks for testing!

…pausing

* Do not navigate back before player finishes stopping
* Ensure pipeline reset before playing a file
* Avoid multiple ended signal from player
@jeremypw
Copy link
Collaborator

@ryonakano The last commit fixes the crashes and hangs I was experiencing. Please re-test when you have time :-)

@jeremypw
Copy link
Collaborator

Like many Gtk apps this contains signal cascades resulting in hard to diagnose races. It is important that a signal handler ends before triggering another signal - else an Idle loop has to be used.

Copy link
Contributor

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

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

I've tested with a mkv and mp4 file of Elephants Dream (mentioned in #32 (comment)) and the issue that external subtitle is not applied seems to be fixed for me. Playing another video in this way still crashes the app, but it should be another issue like #200.

@jeremypw
Copy link
Collaborator

@ryonakano Thanks for testing! So, it looks like my latest changes did not fix the crashing for you then? It seemed to make it pretty stable for me (I may put up a screen capture). But if this crash is also occurring with master then its not a regression and this PR is at least enabling external subtitles to be shown so it may be worth merging anyway.

@danrabbit What do you think?

@jeremypw
Copy link
Collaborator

Demonstration of playing external subtitles and switching videos with this PR.

Screen record from 2020-05-24 18 13 12

@jeremypw
Copy link
Collaborator

It looks like Videos crashes on resume video on machines with an Intel video card, not with an NVidia one. This occurs in master too so is not a regression from this PR.

@jeremypw jeremypw merged commit cce9b64 into master May 29, 2020
@jeremypw jeremypw deleted the subtitle-fix branch May 29, 2020 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants