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

example usage of media callbacks #138

Merged
merged 4 commits into from Jun 4, 2020
Merged

example usage of media callbacks #138

merged 4 commits into from Jun 4, 2020

Conversation

albestro
Copy link
Contributor

@albestro albestro commented Jun 1, 2020

Close #88

I wrote a basic script trying to show how to use callbacks. I know it's not the most basic example, but I thought it would have been useful for the final user for better understanding the mechanism.

Looking forward for your feedback on how to refine/improve it.

#!/usr/bin/env python3

import argparse
import ctypes as ct
Copy link
Owner

Choose a reason for hiding this comment

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

This saves 4 bytes, but hampers readability. I think it would be nice to keep a plain import ctypes

import os

import vlc

Copy link
Owner

Choose a reason for hiding this comment

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

It would help to have some docstring here, explaining the overall goal of the example and describing in simple sentences how it works, and the APIs it uses.

@oaubert
Copy link
Owner

oaubert commented Jun 1, 2020

It is good to have some examples, especially in the callbacks area which is always a bit complex. The example looks fine to me, not overly complex but at the same time using a non trivial API.

@albestro albestro marked this pull request as ready for review June 3, 2020 16:54
@albestro
Copy link
Contributor Author

albestro commented Jun 3, 2020

Let me know if you feel it is enough or you would add something more specific ;)

I didn't add the copyright/license information, I don't know how you would like to manage it.

Copy link
Owner

@oaubert oaubert left a comment

Choose a reason for hiding this comment

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

There is still a typo + I added a suggestion for the license. If you can add the boilerplate, it will allow to merge the request as-is.

@@ -50,7 +75,7 @@ def get_data(self):
"""
It reads the current file in the list and returns the binary data
In this example it reads from file, but it could have downloaded data from an url
"""
i"""
Copy link
Owner

Choose a reason for hiding this comment

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

A typo here

@@ -1,7 +1,32 @@
#!/usr/bin/env python3

# Author: A.Invernizzi (@albestro on GitHub)
Copy link
Owner

Choose a reason for hiding this comment

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

For the license, there are currently 2 different types of licenses for examples (GPLv2+ or MIT), but the best option for examples may indeed be the more permissive MIT license (as in cocoavlc), so you can use cocoavlc as reference if it is OK for you.

@albestro albestro requested a review from oaubert June 4, 2020 04:55
Copy link
Owner

@oaubert oaubert left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. One last thing before the merge.

@@ -3,6 +3,28 @@
# Author: A.Invernizzi (@albestro on GitHub)
# Date: Jun 03, 2020

# MIT License <http://OpenSource.org/licenses/MIT>
#
# Copyright (C) 2017-2018 -- mrJean1 at Gmail dot com
Copy link
Owner

Choose a reason for hiding this comment

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

One last thing: could you change the copyright line to
# Copyright (c) 2020 -- A. Invernizzi, @albestro on github
or
# Copyright (c) 2020 -- the VideoLAN team
for a copyright assignment to the videolan project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry...completely missed it, just copy&paste. My fault.

@oaubert oaubert merged commit f79f4ef into oaubert:master Jun 4, 2020
@oaubert
Copy link
Owner

oaubert commented Jun 4, 2020

Thanks a lot for the contribution and the handling of feedback

@albestro albestro deleted the example/media_callbacks branch June 4, 2020 14:18
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.

is there any way to play video loaded to memory using python-vlc?
2 participants