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

[ie/ArteTV] Separate forced subtitle tracks #9945

Merged
merged 9 commits into from
May 22, 2024

Conversation

vtexier
Copy link
Contributor

@vtexier vtexier commented May 17, 2024

Description of your pull request and other information

Add support for partial (forced) captions in ArteTV extractor.

Without :

Language Type
fr srt, srt
fr-acc srt
de srt

With the support :

Language Type
fr-forced srt
fr srt
fr-acc srt
de srt

Otherwise you might get the partial (forced) as default (quite empty file translating only text appearing on screen), instead of the full captions.

ADD DESCRIPTION HERE

Wrong subtitles downloaded

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • [] 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?

@seproDev seproDev added the site-enhancement Feature request for some website label May 17, 2024
yt_dlp/extractor/arte.py Outdated Show resolved Hide resolved
@@ -1,6 +1,7 @@
import re

from .common import InfoExtractor
from .. import join_nonempty
Copy link
Member

Choose a reason for hiding this comment

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

import it from ..utils

Comment on lines 62 to 74
'note': 'age-restricted',
'url': 'https://www.arte.tv/de/videos/006785-000-A/the-element-of-crime/',
'info_dict': {
'id': '006785-000-A',
'description': 'md5:c2f94fdfefc8a280e4dab68ab96ab0ba',
'title': 'The Element of Crime',
'timestamp': 1696111200,
'duration': 5849,
'thumbnail': 'https://api-cdn.arte.tv/img/v2/image/q82dTTfyuCXupPsGxXsd7B/940x530',
'upload_date': '20230930',
'ext': 'mp4',
},
}, {
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove this test unless you can add another age-restricted test video

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you tell me why ? This test fails as the video is no longer available on the website. May be I can comment it if you really want to keep it as a reminder...

Copy link
Member

Choose a reason for hiding this comment

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

Tests are added to test specific code paths generally. If a test no longer functions, that path is no longer tested, and the test needs to be replaced. Its left in there as a reminder to be replaced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So can I comment it, as a reminder, to avoid breaking the tests before to be able to execute my own test on captions ?

Copy link
Member

@bashonly bashonly May 18, 2024

Choose a reason for hiding this comment

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

restore the test and add 'skip': '404 Not Found' to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works great! I put the skipped test at the end of the tests, otherwise my test was skipped.

'thumbnail': 'https://api-cdn.arte.tv/img/v2/image/3rR6PLzfbigSkkeHtkCZNF/940x530',
'duration': 7599,
'title': 'La loi de Téhéran',
'alt_title': None,
Copy link
Member

Choose a reason for hiding this comment

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

If the correct expected value is None, it doesn't need to be in the test. If not, put the correct value here and let the test fail

Suggested change
'alt_title': None,

@Grub4K
Copy link
Member

Grub4K commented May 18, 2024

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

I'm assuming the patch is yours and you just checked the wrong checkbox, otherwise please note your evidence for it being in the public domain

Copy link
Member

@bashonly bashonly left a comment

Choose a reason for hiding this comment

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

LGTM

Note: in the future, please do not force push

@bashonly
Copy link
Member

On second thought, maybe a more appropriate suffix would be -forced? Since this PR is labeling translated forced subtitles, and since that's how they are labeled in the m3u8

Fixes #9242

Does this PR somehow also fix the "wrong language" problem described in that issue?

@vtexier
Copy link
Contributor Author

vtexier commented May 19, 2024

On second thought, maybe a more appropriate suffix would be -forced? Since this PR is labeling translated forced subtitles, and since that's how they are labeled in the m3u8

You are right I will change the name to "forced" instead of "partial".

Fixes #9242

Does this PR somehow also fix the "wrong language" problem described in that issue?

No, sorry I have read somewhere that the code for "-acc" subtitle was linked with this issue but now I don't see where I have read that. Forget it.

@bashonly bashonly added the pending-review PR needs a review label May 19, 2024
@seproDev seproDev changed the title Arte subtitles partial [ie/ArteTV] Separate forced subtitle tracks May 19, 2024
@bashonly bashonly self-assigned this May 22, 2024
@bashonly bashonly removed the pending-review PR needs a review label May 22, 2024
@bashonly bashonly merged commit 7b56749 into yt-dlp:master May 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
site-enhancement Feature request for some website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants