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] Extract formats from multiple DASH manifests (Closes #6093) #6097

Merged
merged 6 commits into from
Jun 29, 2015

Conversation

dstftw
Copy link
Collaborator

@dstftw dstftw commented Jun 26, 2015

DASH manifest pointed by dashmpd from the video webpage and one pointed by get_video_info may
be different (namely different itag set) - some itags are missing from DASH manifest pointed by
webpage's dashmpd, some - from DASH manifest pointed by get_video_info's dashmpd).
The general idea is to take a union of itags of both DASH manifests (for example video with such
'manifest behavior' see #6093).

DASH manifest pointed by dashmpd from the video webpage and one pointed by get_video_info may
be different (namely different itag set) - some itags are missing from DASH manifest pointed by
webpage's dashmpd, some - from DASH manifest pointed by get_video_info's dashmpd).
The general idea is to take a union of itags of both DASH manifests (for example video with such
'manifest behavior' see ytdl-org/youtube-dl#6093).
@jaimeMF
Copy link
Collaborator

jaimeMF commented Jun 26, 2015

I would prefer to avoid doing an additional network request (potentially 4) by default, it probably makes directly playing the urls (for example with mpv) a bit slower.

@dstftw
Copy link
Collaborator Author

dstftw commented Jun 26, 2015

In most of the cases there would be only one additional network request. Otherwise, we may loose some formats or even the bestvideo if it happens to be served by get_video_info's manifest.

# Remove the formats we found through non-DASH, they
# contain less info and it can be wrong, because we use
# fixed values (for example the resolution). See
# https://github.com/rg3/youtube-dl/issues/5774 for an
# example.
dash_keys = set(df['format_id'] for df in dash_formats)
dash_keys = set(df['format_id'] for df in dash_formats.values())
Copy link
Collaborator

Choose a reason for hiding this comment

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

The keys of the dash_formats dict are the format_id fields themselves, so you could directly set(dash_formats) or set(dash_formats.keys()) to make it clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in d80265c.

@yan12125
Copy link
Collaborator

Would it be better to skip the additional requests if --youtube-skip-dash-manifest given? Of course the documentation needs updated if you're going to do that. In case of mpv, the default format is now best due to mpv-player/mpv#1867. mpv, as well as other players relying youtube-dl, can reduce some network traffic at startup by specifying --youtube-skip-dash-manifest if they are not interested in video/audio-only formats.

And, I hope there's a test case for this PR.

@dstftw
Copy link
Collaborator Author

dstftw commented Jun 27, 2015

Since no DASH manifests downloaded when --youtube-skip-dash-manifest is specified, do you mean not trying to download get_video_info when extraction from the webpage succeeded? Fixing documentation in this case is not enough I guess since no one usually read it and option name (--youtube-skip-dash-manifest) clearly states what should it do. Probably some new generic option like --reduce-traffic should be introduced which would also be useful for another extractors.

@yan12125
Copy link
Collaborator

do you mean not trying to download get_video_info when extraction from the webpage succeeded?

Yes. Sorry that the original statement is not clear enough.

In this PR the additional get_video_info requests are for extracting more DASH urls. When those URLs are not used, I think there's no need to keep the requests. --youtube-skip-dash-manifest can be interpreted as: skipping requests required for getting valid DASH manifests.

@dstftw
Copy link
Collaborator Author

dstftw commented Jun 27, 2015

Fixed in 0a3cf9a.

@jaimeMF
Copy link
Collaborator

jaimeMF commented Jun 27, 2015

Looks good.

@dstftw
Copy link
Collaborator Author

dstftw commented Jun 28, 2015

I'd merge this if nobody minds.

@yan12125
Copy link
Collaborator

I think this PR can be merged.

dstftw added a commit that referenced this pull request Jun 29, 2015
[youtube] Extract formats from multiple DASH manifests (Closes #6093)
@dstftw dstftw merged commit c608ee4 into ytdl-org:master Jun 29, 2015
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.

4 participants