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

[youtube] Show if video is embeddable in info #27747

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pukkandan
Copy link
Contributor

@pukkandan pukkandan commented Jan 9, 2021

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly
  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])
  • Use Preview tab to see how your pull request will actually look like

Before submitting a pull request make sure you have:

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New extractor
  • New feature

Description of your pull request and other information

Adds a new key playable_in_embed in youtube.py infodict
Closes #27730

@dstftw
Copy link
Collaborator

dstftw commented Jan 13, 2021

  1. Any new meta field must be documented in extractor/common.
  2. In order to keep metadata consistent and predictable we only add general purpose meta fields which may be applicable to wide range of extractors.

@pukkandan
Copy link
Contributor Author

Any new meta field must be documented in extractor/common.

I can add the documentation, but if it is not going to be merged anyway since it is not "general purpose", then I am not going to bother with it.

@dstftw
Copy link
Collaborator

dstftw commented Jan 13, 2021

First of, it's not quite clear what's this meta field is about - allowing/not allowing to play in embedded players another sites or what? If so it may be considered generic as many video platforms have such option (e.g. vimeo, livestream).

@pukkandan
Copy link
Contributor Author

done

@remitamine
Copy link
Collaborator

in the case of vimeo, limelight, brightcove, etc..., embed might be restricted by certain conditions.
as an example for vimeo videos, the config would contain embed_permission with values documented in the API documentation.
so in the case of a vimeo that has embed_permission=whitelist, setting the playable_in_embed value to True would give the wrong impression that the video can be embed everywhere, on the other hand setting the value to False is also wrong the video can embeded in certain websites, and for value None, the possibility to embed is known but can't be represented.

@pukkandan
Copy link
Contributor Author

pukkandan commented Jan 14, 2021

@remitamine What values do you suggest? Should any string be allowed in addition to True/False?

I am just trying to implement this in youtube.com. I am not really familiar with the other extractors

@remitamine
Copy link
Collaborator

What values do you suggest? Should any string be allowed in addition to True/False?

i'm pointing that using a boolean value might not be the right choice(how this would be implemented is up to you and what the review process will lead to).

I am just trying to implement this in youtube.com. I am not really familiar with the other extractors

a general purpose meta field should work with the majority of extractors and not only for a specific extractor(trying to avoid the need in the future to add another fields specific to every extractor which will make it more difficult for third parties to consume the info dict).

@pukkandan
Copy link
Contributor Author

I have changed the description to

Whether this video is allowed to play in embedded players on other sites. Can be True (=always allowed), False (=never allowed), None (=unknown), or a string specifying the criteria for embeddability (Eg: 'whitelist').

I dont know what else to change it to. If you think it should be needs further modification, please make a review commit

@kr4ssi
Copy link
Contributor

kr4ssi commented Feb 22, 2021

Can it get merged?

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.

[Youtube] Show if video is embeddable in info
4 participants