Navigation Menu

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

Jump to bottom

Adding OMX Player as experimental option for ES #135

Merged
merged 2 commits into from May 28, 2017
Merged

Adding OMX Player as experimental option for ES #135

merged 2 commits into from May 28, 2017

Conversation

pjft
Copy link
Collaborator

@pjft pjft commented May 19, 2017

---- Changes summary ----

  • Added OMX Player as an option to ES (Only available on the RPi. Default: disabled)
  • Generalized "VideoComponent" class, and created two child classes - one for VLC, one for OMX. In the future it's easy to just remove the OMX one, and/or create a new one for another video renderer we choose to use (ffmpeg? Something else?), so these are easy to revert. The bulk of this work was done by @fieldofcows - he is the git author of that commit in this PR, I just squashed it, and added my fixes and changes separately.
  • Added option to mute videos.

OMX Player has better performance on the Pi than VLC. @fieldofcows did a great work adding it to a separate branch, earlier in the year.

Since then, I ended up picking it up and improving it slightly - fixing some minor annoyance and a couple of leftover issues - and since then I have mostly been maintaining a version of ES that's pretty much the RetroPie main one (frequently rebased), but that has OMX Player as an option, and screensaver as well. I tried to reach out to @fieldofcows for the past 2 months, but didn't hear anything from him.

That version has been extensively used for the past few months both by me as well as by several other forum users (Dominus, TMNTurtlGuy, Scannigan come to mind, but a few others stated that were using it as well), who prefer OMX Player as a default given its better performance on the Pi (avoiding overheating issues in some cases, and allowing for better video quality), so I'd like to bring it over as an explicitly experimental option under a "Video Settings" menu.

It's only available on the Pi - VLC is still the default for anything else. It's easy to revert/remove in the future, as it's a separate independent class.

OMX Player cannot be the default on the Pi because of theming issues, in particular z-indexing. HW accelerated video players, both OMXPlayer and a compiled version of VLC are rendering video on top of everything else.

Still, while there are themes in development that use overlays on top of videos, the majority of the actual themes that are out there do not yet have overlays on videos, so this is a valid option to have in case the user doesn't want to use VLC to get better performance.

I squashed @fieldofcows ' commits into a single one of him, and added mine separately, so that full accurate attribution can be had. Hopefully this is alright.

Following this one I'll submit the screensaver one as well.

This is fully rebased with @jdrassa's z-index changes from yesterday.

Hope this suits. Thanks.

@Darknior
Copy link

You are real ... i'm 100% agree with you :D
We must have OMX on Pi, it is the only player that read perfectly all the videos, with no glitch.
The only one that not burn the Pi processor !!!
With an option to add or remove it, it is perfect.

The only thing i don't understand was, is the new Z-Index option can fix the overlays on top of videos problem?

#130 (comment)

@pjft
Copy link
Collaborator Author

pjft commented May 19, 2017

Not with OMX Player, no. The thing is that EmulationStation uses SDL for rendering, which is hardcoded at a specific layer - 10000, I believe.

You can read more details in this post, where I kind of laid out all the research I had been doing as well:

https://retropie.org.uk/forum/post/62614

The z-index option handles which objects are rendered on top of which, but that's all within the 10000 SDL layer. While VLC renders videos to a texture in ES - so it can be rendered via the ES pipeline - OMX Player with hardware acceleration renders videos separately, unfortunately as a window on top of ES.

As such, for the time being, OMX Player videos are permanently on top of ES, in a separate layer.

I may in the future test whether rendering video through OMX into a texture would result in better performance (than VLC) while being able to render overlays, but reports and research provided mixed results. Even VLC compiled with HW acceleration seemed to start rendering on top of the current layer, according to fieldofcows in the threads, so... we'll see.

Baby steps.

But I agree: in the long run, I'd be very much in favor of settling for a single video player that would be hardware accelerated. But for the time being, this is a good compromise I hope. And it's fairly easy to undo.

Thanks!

@joolswills
Copy link
Member

Thanks. If anyone has time to test this it would be appreciated thanks.

@pjft
Copy link
Collaborator Author

pjft commented May 22, 2017

Thanks.

A quick question: I believe at one time you might have mentioned something about us linking a hardware accelerated version of ffmpeg somewhere? Can you point me in the right direction?

While we may have OMX as an option, I'd like to see what could be a more longer-term solution. Or at least get that out of my system :)

@joolswills
Copy link
Member

the ffmpeg / vlc in Jessie doesn't have it so you would need to build it all (and then there would likely need to be code changes to enable the acceleration when using the vlc library etc).

@pjft
Copy link
Collaborator Author

pjft commented May 22, 2017

I see. I'll leave that battle for later then, you did mention that hopefully in the short to medium term we'd get a hardware accelerated version of VLC with the OS so we'll revisit then.

Thanks.

@Darknior
Copy link

I really want to test this build to help.
But is it possible to know how i can do it ? I must compile ES myself like i do with the fieldofcow build ?
Now i'm in Retropie 4.2.? i can't compile ES from source, with RetropiSetup, it always crash :(
But i can update from source all the emulators without any problem ... and some friends have the same problem :(

@pjft
Copy link
Collaborator Author

pjft commented May 23, 2017

Hi @Darknior . Thanks for the reply.

There have been some comments around ES crashing at compilation from within RetroPie Setup, but it is the common understanding that if you exit EmulationStation and open RetroPie-Setup separately that it should work. Please check whether that's not the case.

That being said, to test this, these steps should help:

1 - Exit to the terminal (quit ES)
2 - Either via terminal or via SSH, run the following (the first command is just to create a new folder, you can name it whatever you want):

mkdir ES-OMX
cd ES-OMX
git clone https://github.com/pjft/EmulationStation.git
git checkout RetroPie-OMX-Player
git submodule update --init
cmake .
make

And then after if finishes compiling, just run

./emulationstation

And you should have the new option for the experimental player in a new menu.

Let me know if this doesn't work - I'm mostly writing from memory.

Thanks!

@Darknior
Copy link

Thanks, you are real for ES, Using RetroPie-Setup separately working fine for me :)

I also compile your version for test and it is excellent i love it.
For my part i must restart ES if i change the video player, if not i don't have videos.
After restart i have videos with OMX, fast and smoother, without any glitch in videos, a real dream :p

Thanks!

@pjft
Copy link
Collaborator Author

pjft commented May 23, 2017 via email

@Darknior
Copy link

It's ok it works fine like you write :D
Thanks

@meleu
Copy link

meleu commented May 25, 2017 •
edited

I've tested on my raspi1 overclocked to High, the results are impressive. I've recorded a video to let you guys see:
https://youtu.be/w-uvnI_o7u8

The first 30 sec is the pjft RetroPie-OMX-Player branch.
From 31 sec on is the official RetroPie ES binary as in 25-may-2017.

UPDATE: I've added some subtitles on the youtube video. So, enable it when watching.

@pjft
Copy link
Collaborator Author

pjft commented May 25, 2017 via email

@meleu
Copy link

meleu commented May 25, 2017

I just updated my branch to tackle that, so if you plan on keeping that build around, do update!

I'll do it as soon as I learn how to cross-compile. It took almost 4 hours to compile on my raspi1! 😅

  • Paulo Tavares

Off-topic: It sounds like a Brazilian name. Você é brasuca também?

@pjft
Copy link
Collaborator Author

pjft commented May 25, 2017

Português;)

I knew you were Brazilian from a few threads in the forum, where you were talking about... Was it rollerblading?

Anyway, glad to help!

@pjft
Copy link
Collaborator Author

pjft commented May 26, 2017

Just squashed the screensaver fix to the latest commit, so it's ready to merge in case it gets approved.

Would you want me to squash both @fieldofcows' and my commits though? I'd rather keep them separate as it gives more credit - and sense of authorship - to his work, but I'll accept any recommendation here.

Thanks.

@jrassa
Copy link
Collaborator

jrassa commented May 26, 2017

@pjft Is OMX Player installed by default or will we need to update the install script as well to ensure it is available?

@joolswills
Copy link
Member

im ok with the commits split.

we can add omxplayer as a dependency in retropie-setup - it's installed by default on our prebuilt image (can be used for the splashscreen).

@pjft
Copy link
Collaborator Author

pjft commented May 26, 2017 via email

@meleu
Copy link

meleu commented May 26, 2017

@joolswills

it's installed by default on our prebuilt image

Now it's explained why it ran on my RetroPie/raspi1 without needing to install anything else...

@pjft
Copy link
Collaborator Author

pjft commented May 26, 2017 via email

@meleu
Copy link

meleu commented May 26, 2017 •
edited

I vote for ENABLE OMX VIDEO PLAYER.

Question: even if my hardware is powerful is there any advantage in using VLC rather than the faster OMX?

I've tested the pjft's branch on RetroPie/x86/Core i7/RAM=8GB and noticed that the OMX player loads the video a little bit faster. It's a small difference, but it's noticeable. I think that if OMX video player proves to be stable, it should be the only option.

@pjft
Copy link
Collaborator Author

pjft commented May 26, 2017 via email

@meleu
Copy link

meleu commented May 26, 2017

Pretty enlightening answer, thank you.

@jrassa
Copy link
Collaborator

jrassa commented May 27, 2017

I don't know if they are considered finalized, but off hand I know Nismo's OldRoom theme and the CRT theme are layering images on top of the video.

I wouldn't have any objections to making OMX the default player, but there may be other theme features down the line that it may not support. For example, I have been doing some experimenting with adding support for rotating theme elements. I am not sure if anyone would want to rotate the video, but it would be possible.

@pjft
Copy link
Collaborator Author

pjft commented May 27, 2017 via email

@pjft
Copy link
Collaborator Author

pjft commented May 27, 2017 •
edited

Addenda: of course, as I said, that's all "for now". I'd love to explore rendering OMX player video to Texture and see if performance holds up, as well as whether there are any other problems.

But that's not a short term effort as it'd need a bit of research and learning on my end. And I can't promise I'd succeed, as @fieldofcows already did a lot of that research and came up empty, so my initial expectations are set quite low.

But yes, this isn't necessarily final, though it's probably as good as it'll get for a while.

@TMNTturtleguy
Copy link

I have been using @pjft's OMX player build for several months and it works really well. The videos play very smooth and they play at a much lower temperature. Once i started using the OMX player i have not gone back. I have tested this PR branch and it works extremely well and as expected. I suggest we leave the option to turn the feature on and off. I know several theme makers like the overlays, and several individual modify their own systems with different overlays as well. I also like the renaming to Enable OMX player, i would suggest we add a note like this Enable OMX Performance Video Player. Or some other way to describe what its purpose is. This is a great addition to ES and a huge improvement on performance and temperature. Nice Work @pjft.

@joolswills
Copy link
Member

I'm happy to merge if @pjft is. I don't mind if it's on by default either, is that the case?

@joolswills
Copy link
Member

joolswills commented May 28, 2017 •
edited

The PR needs rebasing actually due to recent changes. Missed that

@hex007
Copy link

hex007 commented May 28, 2017

I agree with @meleu on this.

@pjft pjft closed this May 28, 2017
fieldofcows and others added 2 commits May 28, 2017 21:26
@fieldofcows @pjft
Adding experimental OMX video player component. Squashed from fieldof…
34ea9ca 
…cows' work (by pjft).
@pjft
Fixes and tweaks to OMXPlayer work, by pjft
029e8bd 
- Correct handling of zombie processes left in memory
- Add options to mute video
- Fix resizing to work with theme refactorings introduced by jdrassa and zigurana
@pjft pjft reopened this May 28, 2017
@pjft
Copy link
Collaborator Author

pjft commented May 28, 2017

Sorry about re-opening and closing - my brute-force rebasing approach causes Git to auto-close the pull-request. My bad.

I'm just recompiling once again to confirm everything is good to go (i.e. that there are no compilation errors).

The option is currently titled "USE OMX VIDEO PLAYER (HW ACCELERATED)" (need to check if it fits the menu), and is OFF by default.

I wouldn't be shocked if OMX Player is by default ON, only because neither the CRT nor the OldRoom themes are currently released, and as such there are no completed themes that use overlays on top of videos at the moment. But, as I said, I do not have a strong opinion.

But if that is to be the case, I'd rather have the option be called "USE VLC PLAYER (SUPPORTS THEME OVERLAYS, SLOWER)" or something, and be off by default.

Let me know your thoughts and we'll wrap this today.

@TMNTturtleguy
Copy link

Beautifully stated! I like the reasoning and the labeling of the VLC player. I think everyone has convinced me that OMX as default is the way to go. It is the only thing thing I use, so it makes me happy!

@joolswills
Copy link
Member

Bear in mind on a PC, vlc should be default and the setting should be hidden (if this isn't already the case)

@meleu
Copy link

meleu commented May 28, 2017

@joolswills
I think it should be an option on a PC too.

#135 (comment)

@pjft
Copy link
Collaborator Author

pjft commented May 28, 2017 via email

@pjft
Copy link
Collaborator Author

pjft commented May 28, 2017

@meleu: OMX Player only exists on the Pi.

If for some reason you're seeing the option on anything other than the Pi, it will be using VLC.

Are you seeing such an option, though?

@meleu
Copy link

meleu commented May 28, 2017

I've got it. My bad, sorry.

@meleu
Copy link

meleu commented May 28, 2017 •
edited

Are you seeing such an option, though?

I've just compiled and checked. The option is gone! 👍

@pjft
Copy link
Collaborator Author

pjft commented May 28, 2017

Just checked. It compiles, and the options are contained within ifdef RPI statements so that it only uses OMX and only shows the OMX option on the Pi.

Current status: the option is currently titled "USE OMX VIDEO PLAYER (HW ACCELERATED)" (need to check if it fits the menu), and is OFF by default.

If you'd like it to be different, just say so - or feel free to edit it on your end (Settings.cpp sets the default, and GuiMenu.cpp has the option text), otherwise merge away.

Best.

@joolswills
Copy link
Member

Thanks

@joolswills joolswills merged commit bc68e0a into RetroPie:master May 28, 2017
@pjft
Copy link
Collaborator Author

pjft commented May 28, 2017

Yay, thanks!

Next stop: video screensaver.

@TMNTturtleguy
Copy link

YES PLEASE!

@Darknior
Copy link

Perfect, all i read here is fine for me :)
Next step Video Screensaver ... so excellent :)

I have some other idées to upgrade skinning concept @pjft ... to show the titleshot at the same place of the video, 1 sec before she start to make the scrolling smother, and the skin more beautifully.
And some other ... i must create a new thread in the forum to explain them and ask you if it is possible to add them ? Thanks

@pjft
Copy link
Collaborator Author

pjft commented May 29, 2017 via email

@TMNTturtleguy
Copy link

@pjft, just noticed something with the screensaver function I wanted to share with you. when testing out a new system I created with additional art for an update to my theme I used a master gamelist supplied by another user. I only loaded 8 roms that I own physical copies of but was to lazy to create a gamelist with only those games and the other user had proved all of the art properly named for the gamelist. What I did do however is onlynload the 8 vidoos onto the pi that correspond to the 8 games. The screensaver ran until it hit a game on the gamelist that said it had a video, but the video did not exist. The screen went black and video never reappeared. I checked the error log and it was filled with all the videos from the gamelist that I didn't have video for. So....users who run master game lists will run into issues using the screensaver function. Let me know if this makes sense. Thanks

@pjft
Copy link
Collaborator Author

pjft commented May 29, 2017 via email

@hex007
Copy link

hex007 commented May 29, 2017

@pjft Audio doesnt work if using USB soundcard.

@zigurana
Copy link

@hex007: this is a (lengthy) discussion on the addition of a new video player. How is your issue related?
Please open a support topic on the forum to get help on your issue. If that shows there is a real problem, then please open a new PR at the relevant repository.

@joolswills
Copy link
Member

joolswills commented May 31, 2017 •
edited

I think I would like to move the settings around a bit. As on x86 there is a video player settings menu with just one item in it. I was thinking:

  • put video player audio setting in the Audio Settings.
  • put the omxplayer setting in Other Settings.

thoughts ?

@pjft
Copy link
Collaborator Author

pjft commented May 31, 2017 via email

@joolswills
Copy link
Member

Can leave it for now. Not a biggy.

@pjft
Copy link
Collaborator Author

pjft commented May 31, 2017 via email

@hex007
Copy link

hex007 commented Jun 1, 2017 •
edited

Submitted PR #152 corresponding to requested changes

fabricecaruso referenced this pull request in fabricecaruso/old-EmulationStation-fork---see-batocera-emulationstation-instead Oct 29, 2019
@nadenislamarre
Merge pull request jrassa#135 from lbrpdx/master
e786c74 
Fix compilation (memcpy) under Ubuntu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@pjft @Darknior @joolswills @meleu @jrassa @TMNTturtleguy @hex007 @zigurana @fieldofcows