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

Replace VLC with OMXPlayer #19

Merged
merged 7 commits into from
Oct 25, 2022
Merged

Replace VLC with OMXPlayer #19

merged 7 commits into from
Oct 25, 2022

Conversation

KaneBetter
Copy link
Collaborator

@KaneBetter KaneBetter commented Oct 21, 2022

Close #11
Fix #16

  • Added pvm_omx.py
  • Added build script for OMXPlayer
  • Added TODO list in the code

Future scope of work:

  • revise README.md with current implementation
  • finish all TODO list

Copy link
Owner

@omarcostahamido omarcostahamido left a comment

Choose a reason for hiding this comment

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

it seems to me that you are very much aware that some work is still needed, not only to fix the several todo items that you wrote on the code (thanks), but also to make it even be a thing that appears on the readme - which, I will stress that again, is very important!
If you all decide it is better to merge in order to split work after that, go for it.
But I want to see some readme updates asap. Doesn't have to be the final thing (it would be hard to do so). But something is better than nothing. It can always be improved later.

Copy link
Owner

@omarcostahamido omarcostahamido left a comment

Choose a reason for hiding this comment

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

I remember everyone agreed to deprecate VLC and move on to fully adopt OMXplayer.
In that sense I propose this PR to already include the deletion of these files doc_vlc_-I_rc.txt doc_vlc_-h_--advanced.txt and pvm_alt.py, as well as possibly not to create an additional pvm_omx.py but instead make that the new pvm.py
Additionally, README.md needs to be updated accordingly 😃

edit: remove also python-vlc from the requirements.txt
Edited OP so that it will close #11 at the same time.

@KaneBetter
Copy link
Collaborator Author

Remove the content about VLC and rename pvm_omx

Copy link
Owner

@omarcostahamido omarcostahamido left a comment

Choose a reason for hiding this comment

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

@KaneBetter thx for the updates.

Again, I'm ok with it being merged if others also agree.

Also, let's try to clear this temporary TODO list on the readme asap, and turn that into issues to be assigned and addressed 😉

@@ -3,6 +3,12 @@ Pi Video Machine - a scalable, synchronized, and networked-controlled, raspberry


### { This is a work in progress }

- TODO: rename the variable
Copy link
Owner

Choose a reason for hiding this comment

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

@KaneBetter what variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, I see what you did now. You just transcribed all the todo comment lines into the readme.

- TODO: rename the variable
- TODO: rewrite logging
- TODO: rewrite logic between commands
- TODO: Create another python file to control two display
Copy link
Owner

Choose a reason for hiding this comment

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

this sounds more like a latter version of the project - the end goal (last milestone) -, rather than an alternative version?
things to decide when we get to that point include:

  • is it using 2 UDP servers on different ports?
  • is it possible to detect with rpi device has 1 or 2 screens connected?

of course, I am not saying that you shouldn't start working on it, I am just saying that it might turn out to be the final version of the project, and not an alternative python script 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we need to think about the use case in more detail in the later milestone.

@KaneBetter KaneBetter merged commit 706338e into main Oct 25, 2022
@KaneBetter KaneBetter deleted the UseOmxplayer branch October 25, 2022 19:44
omarcostahamido added a commit that referenced this pull request Oct 25, 2022
* Replace VLC with OMXPlayer (#19)

* Used Omxplayer in system 10
* Add shell script for build omxplayer
* Add set rate feature
* Added logging format and TODO list
* Remove vlc and change README.md

* Lib folder (#23)

- this fixes #21 
- users will only have to open `pvm.maxproj` now

* move max files into `lib` folder

* Create pvm.maxproj

* Update README.md

- added reference to `pvm.maxproj`
- moved long explanation of the max control interface to the structure section.

Co-authored-by: Kane <[email protected]>
KaneBetter added a commit that referenced this pull request Oct 27, 2022
* Used Omxplayer in system 10
* Add shell script for build omxplayer
* Add set rate feature
* Added logging format and TODO list
* Remove vlc and change README.md
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.

Looking for an alternative media player Investigate buffer deadlock prevented issues.
2 participants