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

Code Updates for using libvlc (2.1.1), fixes #33 #48

Merged
merged 5 commits into from
Mar 15, 2017

Conversation

csoni111
Copy link
Member

@csoni111 csoni111 commented Mar 8, 2017

This pr is in reference to issue #33 on github and issue #2014 on amahi issue tracker.

There have been a lot of changes in the libvlc library since it was last used in this project. I have tried to make all the necessary changes required to make it work.

The following changes have been made:

  • Moved maven url for libvlc from buildscript repositories to dependencies repositories in build.gradle
  • New method for initializing LibVlc with some args added to it
  • New MediaPlayer class added
  • Replaced setSurfaceSize method previously given in libvlc.IVideoPlayer and implemented in VideoService by onNewVideoLayout given in IVLCVout.OnNewVideoLayoutListener and implemented in ServerFileVideoActivity
  • Added new code implementation for scaling the surface view size according to the video's dimensions
  • Removed Event Listeners from VideoService and implemented in ServerFileVideoActivity as provided in new MediaPlayer.EventListener interface
  • Removed BusEvents and related classes, as they were no longer required

@megabitdragon
Copy link
Member

I've tried this. The app builds fine but it crashes when trying to play movies. Here is the error for reference.

@csoni111
Copy link
Member Author

csoni111 commented Mar 9, 2017

Please Make a Clean Build and the error will be resolved.

You are getting the error because previously in ServerFileVideoActivity class there was an injected instance for serverClient field but in the updated code, it has been removed. The Dagger library used in the project makes separate classes for all the injected dependencies on building the project. You might be having previously built classes like this one ServerFileVideoActivity$$InjectAdapter.java in the build folder. Cleaning the build folder will resolve the error.

View.OnSystemUiVisibilityChangeListener
{
@Inject
ServerClient serverClient;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just for reference: here ^ is the instance injection that was removed. @megabitdragon

@megabitdragon
Copy link
Member

The clean build did the job. There are however some issues with it. Some might be vlc related and I'll take a look at those but there are others as well (e.g.: the controls don't show up when you tap screen, movie restarts when changing orientation). Good job so far.

@csoni111
Copy link
Member Author

csoni111 commented Mar 9, 2017

I will try to resolve the errors

@megabitdragon megabitdragon self-requested a review March 10, 2017 16:36
@csoni111
Copy link
Member Author

csoni111 commented Mar 11, 2017

@megabitdragon I have tried to resolve some of the issues that you mentioned:

  • Orientation change issue has been resolved.
  • For controls not showing up after fullscreen, I am facing an odd bug:
    If I remove this toast message from line#155 in my last commit, the controls dont show up but if this toast is displayed then controls work as expected. I am not sure why this is happening.

I have another solution for this, in my another pr #71 I have added a FullScreenHelper.java class to make any activity go full screen and display controls on screen touch event. That one uses a different approach than this one to toggle fullscreen. You may test it in that pr in the ImageViewer Activity.
So after that gets merged we can rebase and use that class for making this video activity go fullscreen.

@csoni111
Copy link
Member Author

Looks like toggling video controls is a long standing bug. I just found it here #1683 referenced two years ago.

- fix: surface width and height becoming zero bug
- feat: different surface video scaling types added
- TODO added in code
- orientation change restarts video
- controls not showing after fullscreen
- app crashes in some cases
@megabitdragon
Copy link
Member

Good work on this. The movie restarting when changing orientation is fixed. I see what you mean about the toast and video controls. I will take a look at it as well. Meanwhile I merged PR #71 and rebased the vlc2 branch. This might create some issues with your commits since the references were changed.

@csoni111
Copy link
Member Author

No issues I have rebased my vlc2 branch accordingly and now I can use that FullScreenHelper class to resolve that video controls issue. Hopefully now it will be resolved!

@csoni111
Copy link
Member Author

@megabitdragon the video controls toggling issue has now been fixed in my last commit. So till now following bugs have been fixed as per amahi bug tracker: #2014, #1683, #1556, #1416.

@csoni111
Copy link
Member Author

Bug #1446: MediaController Leaks Window After Video Stop and some sync issue between toolbar and media controls have now been resolved.

@megabitdragon
Copy link
Member

I tried it it works as expected. One small issue I noticed is that when I change orientation in the emulator, the video only shows up in a corner. It works fine on device.
device-2017-03-13-195923

@megabitdragon megabitdragon merged commit 2623002 into amahi:vlc2 Mar 15, 2017
@cpg
Copy link
Member

cpg commented Mar 15, 2017

Hi guys, i decided to try this and after a full cleanup, it works quite well on all file formats I tried it on. Great job!

Should we merge it into master?

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.

3 participants