Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Partial fix for "begin" options not working as intended #159

Closed
wants to merge 2 commits into from
Closed

Partial fix for "begin" options not working as intended #159

wants to merge 2 commits into from

Conversation

TinyMan
Copy link
Contributor

@TinyMan TinyMan commented May 18, 2017

This PR aims to partially fix #129. I'm not sure whether this belongs in ytdl or if it should be on the user side, so feel free to reject it if you think so.

When begin parameter is used, it tries to download the "index" of the file + the codec metadata. After that it searches the the index for the position of the wanted begin time. Then it resume the download with the range parameter.

I've seen it work for multiple videos but not all of them. Index is not present / not encoded the same way in every video. For now it works with opus encoded audio (filter audioonly), but I haven't found any video with audio + video track that have this index.

When using begin option:

  • When possible, build index based on format.index that indicates the byte location of the index. Then download the video starting at the offset looked up in the index
  • if not possible, fallback to begin parameter
    Additions:
  • util.parseRanges which parse the ranges of a format (if any)
  • buildIndex that build index given a buffer and a range
  • the index builder could support more format in the future, have to investigate further

TinyMan added 2 commits May 17, 2017 19:40
… begin parameter

When using begin option:
* When possible, build index based on format.index that indicates the byte location of the index. Then download the video starting at the offset looked up in the index
* if not possible, fallback to begin parameter
Additions:
* util.parseRanges which parse the ranges of a format (if any)
* buildIndex that build index given a buffer and a range
* the index builder could support more format in the future, have to investigate further
@fent
Copy link
Owner

fent commented May 22, 2017

Hmm, the more I think about it, the more I believe at least the core of this functionality should be placed outside of ytdl-core. Not necessarily by users of ytdl-core, but it could be a module that ytdl-core depends on.

There are potentially other media downloaders out there that could benefit from this same feature, that's one reason to keep this separated. ytdl-core should be isolated to YouTube.

I'm still not sure. It's partly in the same reasoning of why I haven't gotten around to fixing #100, because I'm not sure if it should be fixed in ytdl-core, or outside.

@TinyMan
Copy link
Contributor Author

TinyMan commented May 23, 2017

I agree with your thoughts. I also believe that a separated module could be better. However, I cannot figure out the scope of such a package. I cannot choose between:

  1. a youtube-streamer package that use this technique and ytdl at its core to stream/seek/pause youtube videos
  2. something like 1. but that allows to stream any video/audio (a very very simple implementation of HTML5 media element)
  3. a more generic package that decodes the audio file structure and could bee used to seek the correct location. This kind of package already exists https://www.npmjs.com/package/ebml. It is used for example by discordie. Other dependent modules are also interesting.

@fent
Copy link
Owner

fent commented May 24, 2017

I was leaning more towards 3. A package that, given the index, it parses it. The part about downloading the index would still be implemented by this ytdl-core as in this PR.

@TinyMan
Copy link
Contributor Author

TinyMan commented May 28, 2017

After some research I think seeking should only be implemented in the player. Because when downloading only a portion of the file the data becomes corrupted unless we're doing something with it and expect specific data (ex: converting / playing it directly and we requested that portion of the file). But when downloading to store in a file it would require to rewrite metadata and I believe you don't want that, correct me if I'm wrong.

So unless you're planning on providing a "stream service" which provide the ability to seek / pause it's up to the user to request the correct ranges (for example with a stream service that decodes the file on the fly and allow to request only specific ranges)

@fent
Copy link
Owner

fent commented May 29, 2017

If we think about it from that perspective it does make sense. A player would not only use ytdl-core, but could potentially use other websites or sources for media. It would make more sense to "seek" with the player, than with each individual media source.

However, what would we do with the begin option that YouTube itself has? It seems like a bonus feature we can keep, but not completely rely on.

@TinyMan
Copy link
Contributor Author

TinyMan commented May 29, 2017

Yeah the begin option is nice and all, but YouTube doesn't really support it ... As you said we can't rely on it.

I don't know how you discovered it and what is the original purpose of this option on YouTube's side, but right nowI don't see anything we can do to fix it except emulating it with range requests and metadata rewriting.

@TinyMan
Copy link
Contributor Author

TinyMan commented Jun 19, 2017

I finally made a player that does basically the same as this PR but it's cleaner: https://github.com/TinyMan/node-jamy

For now it supports seeking only for webm files (only tested with audioonly filter of ytdl). Other formats works but no seeking.
It's still WIP but I'll try to add more format, more test coverage and docs. Though it is stable enough to be used.
I aim to fully support ytdl-core as a media source. To anyone reading this don't hesitate to contribute with issues/PR.

I'll close this PR as it's not an issue for ytdl to fix

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"begin" option doesn't affect when the video starts
2 participants